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

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 5/22/25 18:01, Reinette Chatre wrote:
> 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.

Sure. Will update the comment.
With Tony's changes(
https://lore.kernel.org/lkml/20250429003359.375508-3-tony.luck@intel.com/),
the resctrl_is_mbm_event() is not required in here. It is already there.

I will have to update the comment only on mbm_cntr_get().  Will do.

>>
>> 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)

Sure.

> 
>> +
>>  "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.

Will add the comment.

> 
>> 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
> 
> 

-- 
Thanks
Babu Moger