Re: [PATCH v13 15/27] x86/resctrl: Report 'Unassigned' for MBM events in mbm_cntr_assign mode

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 5/15/25 3:52 PM, Babu Moger wrote:
> In mbm_cntr_assign mode, the hardware counter should be assigned to read
> the MBM events.
> 
> Report 'Unassigned' in case the user attempts to read the event without
> assigning a hardware counter.
> 
> Export resctrl_is_mbm_event() and mbm_cntr_get() to allow usage from other
> functions within fs/resctrl.

Please clarify that these two functions are exposed differently, resctrl_is_mbm_event()
is added to include/linux/resctrl.h (also note similar change in 
https://lore.kernel.org/lkml/20250429003359.375508-3-tony.luck@intel.com/)
so not just exposed to fs/resctrl but instead to resctrl fs as well as
arch code
while mbm_cntr_get() remains internal to resctrl fs by being added to
fs/resctrl/internal.h.


> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---


> ---
>  Documentation/filesystems/resctrl.rst |  8 ++++++++
>  fs/resctrl/ctrlmondata.c              | 14 ++++++++++++++
>  fs/resctrl/internal.h                 |  2 ++
>  fs/resctrl/monitor.c                  |  4 ++--
>  fs/resctrl/rdtgroup.c                 |  2 +-
>  include/linux/resctrl.h               |  1 +
>  6 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 2bfad43aac9c..5cf2d742f04c 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -430,6 +430,14 @@ When monitoring is enabled all MON groups will also contain:
>  	for the L3 cache they occupy). These are named "mon_sub_L3_YY"
>  	where "YY" is the node number.
>  
> +	The mbm_cntr_assign mode offers "num_mbm_cntrs" number of counters
> +	and allows users to assign a counter to mon_hw_id, event pair enabling
> +	bandwidth monitoring for as long as the counter remains assigned.
> +	The hardware will continue tracking the assigned mon_hw_id until
> +	the user manually unassigns it, ensuring that counters are not reset
> +	during this period. An MBM event returns 'Unassigned' when the event
> +	does not have a hardware counter assigned.

(please rework based on "event" vs "group" assignment ... not intending
that "group" assignment be documented but the "event" assignment needs
to be accurate for "group" assignment to be a simple extension)

> +
>  "mon_hw_id":
>  	Available only with debug option. The identifier used by hardware
>  	for the monitor group. On x86 this is the RMID.
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index 6ed2dfd4dbbd..f6b8ad24b0b5 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -643,6 +643,18 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  			goto out;
>  		}
>  		d = container_of(hdr, struct rdt_mon_domain, hdr);
> +
> +		/*
> +		 * Report 'Unassigned' if mbm_cntr_assign mode is enabled and
> +		 * counter is unassigned.
> +		 */
> +		if (resctrl_arch_mbm_cntr_assign_enabled(r) &&
> +		    resctrl_is_mbm_event(evtid) &&
> +		    (mbm_cntr_get(r, d, rdtgrp, evtid) < 0)) {
> +			rr.err = -ENOENT;
> +			goto checkresult;
> +		}
> +
>  		mon_event_read(&rr, r, d, rdtgrp, &d->hdr.cpu_mask, evtid, false);
>  	}
>  
> @@ -652,6 +664,8 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  		seq_puts(m, "Error\n");
>  	else if (rr.err == -EINVAL)
>  		seq_puts(m, "Unavailable\n");
> +	else if (rr.err == -ENOENT)
> +		seq_puts(m, "Unassigned\n");
>  	else
>  		seq_printf(m, "%llu\n", rr.val);
>  

It may be unexpected that this is treated as "-ENOENT" but the function returns
success. This can be addressed with a comment when comparing the return codes to
other hardware return codes.

> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
> index 64ddc107fcab..0dfd2efe68fc 100644
> --- a/fs/resctrl/internal.h
> +++ b/fs/resctrl/internal.h
> @@ -381,6 +381,8 @@ int resctrl_assign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d,
>  			      struct rdtgroup *rdtgrp, enum resctrl_event_id evtid);
>  int resctrl_unassign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d,
>  				struct rdtgroup *rdtgrp, enum resctrl_event_id evtid);
> +int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
> +		 struct rdtgroup *rdtgrp, enum resctrl_event_id evtid);
>  
>  #ifdef CONFIG_RESCTRL_FS_PSEUDO_LOCK
>  int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index fbc938bd3b23..c98a61bde179 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -956,8 +956,8 @@ static void resctrl_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d
>   * mbm_cntr_get() - Return the cntr_id for the matching evtid and rdtgrp in
>   *		    cntr_cfg array.
>   */
> -static int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
> -			struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
> +int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
> +		 struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
>  {
>  	int cntr_id;
>  
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index f192b2736a77..72317a5adee2 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -127,7 +127,7 @@ static bool resctrl_is_mbm_enabled(void)
>  		resctrl_arch_is_mbm_local_enabled());
>  }
>  
> -static bool resctrl_is_mbm_event(int e)
> +bool resctrl_is_mbm_event(int e)
>  {
>  	return (e >= QOS_L3_MBM_TOTAL_EVENT_ID &&
>  		e <= QOS_L3_MBM_LOCAL_EVENT_ID);
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 59a4fe60ab46..f78b6064230c 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -441,6 +441,7 @@ static inline u32 resctrl_get_config_index(u32 closid,
>  	}
>  }
>  
> +bool resctrl_is_mbm_event(int e);
>  bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l);
>  int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable);
>  

Reinette