Re: [PATCH v14 20/32] fs/resctrl: Report 'Unassigned' for MBM events in mbm_event mode

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

Hi Reinette,

On 6/24/2025 11:14 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/13/25 2:05 PM, Babu Moger wrote:
>> When "mbm_event" mode is enabled, a hardware counter must be assigned to
> 
> "When the "mbm_event" counter assignment mode is enabled ..."

Sure.

> 
>> read the event.
>>
>> Report 'Unassigned' in case the user attempts to read the event without
>> assigning a hardware counter.
>>
>> Export mbm_cntr_get() to allow usage from other functions within
> 
> "Export" can be a loaded term in the Linux kernel. Perhaps:
> "Export mbm_cntr_get() ... " -> "Declare mbm_cntr_get() in fs/resctrl/internal.h ..."
> 

Sure.

>> fs/resctrl.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> ...
> 
>> ---
>>   Documentation/filesystems/resctrl.rst |  8 ++++++++
>>   fs/resctrl/ctrlmondata.c              | 19 ++++++++++++++++++-
>>   fs/resctrl/internal.h                 |  2 ++
>>   fs/resctrl/monitor.c                  |  4 ++--
>>   4 files changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
>> index 8a2050098091..18de335e1ff8 100644
>> --- a/Documentation/filesystems/resctrl.rst
>> +++ b/Documentation/filesystems/resctrl.rst
>> @@ -434,6 +434,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_event" mode offers "num_mbm_cntrs" number of counters and
> 
> "The "mbm_event" mode" -> "The "mbm_event" counter assignment mode"?

Sure.

> 
>> +	allows users to assign counter IDs to mon_hw_id, event pairs enabling
> 
> "users to assign counter IDs" -> "users to assign counters"
> 

Sure.

>> +	bandwidth monitoring for as long as the counter remains assigned. The
>> +	hardware will continue tracking the assigned mon_hw_id until the user
> 
> "assigned mon_hw_id" -> "assigned counter"?
> 

Sure.

>> +	manually unassigns it, ensuring that event data is not reset during this
>> +	period. An MBM event returns 'Unassigned' when the event does not have
>> +	a hardware counter assigned.
>> +
>>   "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 ad7ffc6acf13..8a182f506877 100644
>> --- a/fs/resctrl/ctrlmondata.c
>> +++ b/fs/resctrl/ctrlmondata.c
>> @@ -648,15 +648,32 @@ 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_event" 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;
>> +		}
>> +
> 
> When looking at this snippet in combination with patch #22 that adds the support for
> reading counters the flow does not look ideal. While above adds a check whether
> this is dealing with counters, it only does so to check if a counter is *not* assigned.
> I cannot see *any* other check by resctrl whether it is dealing with counters while
> it lumps all information into parameters to resctrl_arch_reset_rmid() and
> resctrl_arch_rmid_read(), needing to provide "dummy" parameters when not all information
> is relevant, and leaving the arch to need to determine if it is
> dealing with counters and then use provided parameters based on that information.
> 
> I think it will be simpler for resctrl to determine if a counter or RMID needs to be
> read and then call appropriate arch API for each and provide only necessary information
> to support that call.
> 
> I think this can be accomplished with following changes:
> - drop above snippet from rdtgroup_mondata_show() (this will be done in mon_event_read())
> - introduce new rmid_read::is_cntr that is a boolean that is true if it is a counter
>    that should be read.
> - mon_event_read() initializes rmid_read::is_cntr and returns with rmid_read::err
>    set if a counter should be read but no counter is assigned (above snippet). The
>    added benefit of doing this in mon_event_read() is that if a counter is not
>    assigned on new monitor group create or domain add then the mon_add_all_files()->mon_event_read()
>    will return immediately with this error instead of trying to read the unassigned
>    counter.
> - __mon_event_count() should *only* attempt to initialize the counter ID (call mbm_cntr_get)
>    if rmid_read::is_cntr is true.
> - Introduce two new arch calls (naming TBD):
>    resctrl_arch_cntr_read() and resctrl_arch_reset_cntr() that will respectively read
>    and reset the counter.

It may be necessary to restructure resctrl_arch_cntr_read(), as there is 
some shared logic that applies to both resctrl_arch_rmid_read() and 
resctrl_arch_cntr_read().


> - __mon_event_count() calls appropriate API based on rmid_read::is_cntr.
> 
> What do you think?

Sounds good to me—this seems like a much cleaner approach. I’ll start 
making the changes on Monday(out of the office tomorrow). I’ll let you 
know if I run into any issues. I might post snippet if necessary.

Thanks
Babu