Re: [PATCH v2 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event

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

Hi Reinette,


On 9/11/26 17:12, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/4/26 11:06 AM, Babu Moger wrote:
>> resctrl_mbm_assign_mode_write() frees all counters and sets
> 
> The changelog is easier to read if it documents what the code does
> instead of documenting the function names. In its current form the reader needs
> to stop at the first word of this changelog, go to the source code,
> figure out when resctrl_mbm_assign_mode_write() is called, and then be able
> to return to changelog to further try and understand the change.

Ack.

> 
> Consider an alternative like:
> 	When the user enables counter assignment mode by writing "mbm_event"
> 	to /sys/fs/resctrl/info/L3_MON/mbm_assign_mode resctrl resets all
> 	monitoring state and sets ...
> 

Sure.

>> mbm_assign_on_mkdir for subsequent mkdir, but does not assign counters to
>> groups that already exist, including the default group created at mount.
>> Those events then read "Unassigned" until the user assigns counters by
> 
> "Those events"? No mention of "events" before this.

Will change.

> 
>> hand.
>>
>> Enable mbm_assign_on_mkdir and assign counters to existing CTRL_MON and MON
>> groups with resctrl_assign_cntrs_allrdtgrp() so the switch matches mkdir
> 
> no need to mention the function name, this can be seen from the patch. Just mention
> what the change achieves. Looks like "with resctrl_assign_cntrs_allrdtgrp()" can
> just be dropped.

ok.

> 
>> auto-assignment. Groups left without a counter still read "Unassigned".
> 
> "Groups" -> "An event ..."?
> "still read" -> "reads"?

Sure.

> 
> This implies that counters are assigned to all groups but that may not be possible.
> I am not sure what would be best text here. How about something like:
> 
> 	Enable mbm_assign_on_mkdir and assign counters, while there are some
> 	available, to existing ...
> 

Sure.

>>
>> Update Documentation/filesystems/resctrl.rst to describe this.
>>
>> Fixes: 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v2: New patch.
>>      This patch addresses the Sashiko comment about documentation issue where
>>      counters are not assigned automatically when mode is switched to mbm_event.
>>      https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com
> 
> Sounds like this is needed:
> 
> 	Reported-by: Sashiko <sashiko-bot@kernel.org>
> 	Closes: https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com

Sure. Will add.

> 
> Is this a stable candidate?

Yes. Will add CC to stable.

> 
>>      In fact, it exposed a real issue. When switching to mbm_event mode, existing
>>      monitoring groups should be assigned counters whenever counters are available.
>>      This provides a smooth transition between modes and aligns the behavior with
>>      the existing auto-assignment mechanism.
>> ---
>>   Documentation/filesystems/resctrl.rst |  7 +++++--
>>   fs/resctrl/monitor.c                  | 30 ++++++++++++++++++++++++---
>>   2 files changed, 32 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
>> index e4b66af55ffb..79feeb1dc296 100644
>> --- a/Documentation/filesystems/resctrl.rst
>> +++ b/Documentation/filesystems/resctrl.rst
>> @@ -371,8 +371,11 @@ with the following files:
>>   	of counters available is described in the "num_mbm_cntrs" file. Changing the
>>   	mode may cause all counters on the resource to reset.
>>   
>> -	Moving to mbm_event counter assignment mode requires users to assign the counters
>> -	to the events. Otherwise, the MBM event counters will return 'Unassigned' when read.
>> +	Moving to mbm_event counter assignment mode enables "mbm_assign_on_mkdir" and
>> +	assigns counters to the events of all existing groups, including the default
> 
> "all existing groups" -> "all existing monitoring groups"

Sure.

> 
>> +	group, for as long as counters remain available. Events left without a counter
> 
> "for as long as" implies duration. Perhaps "while counters remain available"?

ok.

> 
>> +	will return 'Unassigned' when read until the user assigns one using
>> +	"mbm_L3_assignments".
> 
> hmmm ... guiding users to read each event and use the return value to learn whether
> a counter is assigned or not seems inefficient. How about replacing last sentence with
> something similar to the "mbm_assign_on_mkdir" doc:
> 
> 	Consult "mbm_L3_assignments" after switching to "mbm_event" mode for
> 	counter assignment states of all monitoring groups.

Sure.

> 
>>   
>>   	The mode is beneficial for AMD platforms that support more CTRL_MON
>>   	and MON groups than available hardware counters. By default, this
>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index 73413cb128ea..61463741b91b 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -1326,6 +1326,27 @@ void rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp)
>>   					   &mon_event_all[QOS_L3_MBM_LOCAL_EVENT_ID]);
>>   }
>>   
>> +/*
>> + * resctrl_assign_cntrs_allrdtgrp() - Assign counters to the MBM events of every
>> + *				      existing group. Called when "mbm_event" mode
>> + *				      is enabled.
> 
> Please do not include caller information in function comments. This does not age well
> and this patch clearly demonstrates this: rdtgroup_assign_cntrs()'s comments
> read "Called when a new group is created.", after this patch those comments are no
> longer accurate and thus also needs to change as part of this patch.
> 

Sure. Will change it.

>> + *
>> + * Groups created while in "default" mode have no counter assigned, including the
>> + * default group created when resctrl is mounted. Assign counters to them so that
>> + * enabling the mode leaves the same assignments that mkdir would have made.
> 
> Above comment belongs in caller.

ok.

> 
>> + */
>> +static void resctrl_assign_cntrs_allrdtgrp(void)
>> +{
>> +	struct rdtgroup *prgrp, *crgrp;
>> +
>> +	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
>> +		rdtgroup_assign_cntrs(prgrp);
>> +
>> +		list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list)
>> +			rdtgroup_assign_cntrs(crgrp);
>> +	}
>> +}
> 
> I think sashiko's feedback about needing to test for pseudo-locked groups need not
> be followed since there is no overlap between systems supporting assigned counters
> and those that support pseudo-locking.

Sure. Thanks

> 
>> +
>>   /*
>>    * rdtgroup_free_unassign_cntr() - Unassign and reset the counter ID configuration
>>    * for the event pointed to by @mevt within the domain @d and resctrl group @rdtgrp.
>> @@ -1599,9 +1620,6 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf,
>>   									   (READS_TO_LOCAL_MEM |
>>   									    READS_TO_LOCAL_S_MEM |
>>   									    NON_TEMP_WRITE_TO_LOCAL_MEM);
>> -		/* Enable auto assignment when switching to "mbm_event" mode */
>> -		if (enable)
>> -			r->mon.mbm_assign_on_mkdir = true;
>>   		/*
>>   		 * Reset all the non-achitectural RMID state and assignable counters.
>>   		 */
>> @@ -1609,6 +1627,12 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf,
>>   			mbm_cntr_free_all(r, d);
>>   			resctrl_reset_rmid_all(r, d);
>>   		}
>> +
> 
> Comment within the block can be dropped and instead a new comment can be placed here.
> Something like:
> 	/*
> 	 * Counters were freed above, so both new groups (via mkdir) and the
> 	 * groups that already exist need assignments.
> 	 */

Sure.
Thanks
Babu