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

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

Hi Reinette,

On 10/12/2026 1:44 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/9/26 4:44 PM, Babu Moger wrote:
>> When the user switches counter assignment mode by writing "mbm_event" to
>> /sys/fs/resctrl/info/L3_MON/mbm_assign_mode, resctrl frees all assignable
>> counters, resets per-domain RMID state, and enables counter auto-assignment
>> exposed to user space as mbm_assign_on_mkdir.
>>
>> Even though counter auto-assignment is enabled, groups that already exist
>> at the time of the switch, including the default group created at mount,
>> are not assigned a counter. All MBM events read "Unassigned" until the user
>> assigns a counter by hand.
>>
>> Walk every existing CTRL_MON and its MON children after the reset and
>> assign counters to their MBM events. Enabling "mbm_event" now leaves the
>> same per-group state that auto-assignment would have produced. There may be
> 
> nit: Related to more detailed comments later the above "that auto-assignment
> would have produced" may not always be accurate. If the patch behavior is
> maintained (more later) then one way around this may be to just instead say
> "that auto-assignment could have produced".

Ack.

> 
>> fewer available counters than MBM events across the existing groups; in
>> that case stop assignment when no counters remain. Events in the remaining
>> groups read "Unassigned", matching the behavior of creating a group when no
>> counters are available. Also revise the rdtgroup_assign_cntrs() function
>> comments to account for the new callers.
>>
>> Fixes: 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Closes: https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> Cc: stable@vger.kernel.org
>> ---
> 
> ...
> 
>> ---
>>   Documentation/filesystems/resctrl.rst |  7 ++--
>>   fs/resctrl/monitor.c                  | 47 ++++++++++++++++++++++-----
>>   2 files changed, 43 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
>> index b52795e03303..c8507580474a 100644
>> --- a/Documentation/filesystems/resctrl.rst
>> +++ b/Documentation/filesystems/resctrl.rst
>> @@ -370,8 +370,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 monitoring groups, including
>> +	the default group, while counters remain available. Consult
> 
> This also adds the "including the default group" caveat that I will refer to later.

ack

> 
>> +	"mbm_L3_assignments" after switching to "mbm_event" mode for counter
>> +	assignment states of all monitoring groups.
>>   
>>   	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 04e4ea612b7c..9e705eafe873 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -1302,14 +1302,17 @@ static int rdtgroup_assign_cntr_event(struct rdt_l3_mon_domain *d, struct rdtgro
>>   }
>>   
>>   /*
>> - * rdtgroup_assign_cntrs() - Assign counters to MBM events. Called when
>> - *			     a new group is created.
>> + * rdtgroup_assign_cntrs() - Assign counters to enabled MBM events of a
>> + *                         resource group
> 
> nit: please fix alignment. This patch inconsistently uses tabs vs spaces for this
> alignment. That is not major since this file itself is not consistent in this
> regard, but the file does consistently align the description irrespective of
> character used.

Sure.

> 
>>    *
>>    * Each group can accommodate two counters per domain: one for the total
>> - * event and one for the local event. Assignments may fail due to the limited
>> - * number of counters. However, it is not necessary to fail the group creation
>> - * and thus no failure is returned. Users have the option to modify the
>> - * counter assignments after the group has been created.
>> + * event and one for the local event. Assignments may fail due to the
>> + * limited number of counters.
>> + *
>> + * Intended to be called in auto assignment scenarios ("mbm_assign_on_mkdir"
>> + * enabled in "mbm_event" counter assignment mode) where counter assignment
>> + * is not required to succeed. Thus no failure is returned. Users can modify
>> + * the counter assignments afterwards if needed.
>>    */
>>   void rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp)
>>   {
>> @@ -1328,6 +1331,22 @@ 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.
>> + */
>> +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);
>> +	}
>> +}
>> +
>>   /*
>>    * 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.
>> @@ -1601,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.
>>   		 */
>> @@ -1611,6 +1627,19 @@ 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);
>>   		}
>> +
>> +		/*
>> +		 * Counters were freed above, so both new groups (via mkdir) and
>> +		 * the groups that already exist need assignments. Groups created
>> +		 * while in "default" mode have no counter assigned, including the
>> +		 * default group created when resctrl is mounted. Assign counters
> 
> This patch refers three times to how "default group" receives counters during auto assignment.
> 
> How important is it that the default group is assigned counters when switching to "mbm_event" mode?
> 
> This patch implies that auto assignment will always include the default group but that cannot be
> guaranteed since the default group is always at the end of the rdt_all_groups list. This means
> that when traversing rdt_all_groups in resctrl_assign_cntrs_allrdtgrp(), whether default group
> receives counters would depend on how many other CTRL_MON and MON groups there are.

Good point. I recreated the problem. It is not always guaranteed that 
the default group gets the counter with the current logic.

> 
> 
>> +		 * to them so that enabling the mode leaves the same assignments
>> +		 * that mkdir would have made.
> 
> Related to previous comment is that the changelog mentions "same per-group state that auto-assignment
> would have produced" that repeats a similat claim made above that states "same assignments that mkdir
> would have made". This behavior assumes that a CTRL_MON group is created with all its MON groups before
> the next CTRL_MON group is created *and* that enough counters are available for all groups. If
> there are not enough counters then the most recent CTRL_MON groups will receive counters first potentially
> leaving the first "mkdir" created CTRL_MON groups without counters.
> 
> It is not clear to me whether default group requires counters. Separately, I do not see whether resctrl
> can or should support the same ordering as what "mkdir" would have produced. If these are not actually
> requirements then the patch is fine but the comments, documentation change, and changelog need to change
> to match the code.
> 
> If the default group does require counters then we either need to change how groups
> are added to rdt_all_groups or how resctrl_assign_cntrs_allrdtgrp() traverses the list.


We have always treated the default group as a special case. I think we 
should ensure that the default group gets counters before any other 
group. How about adding logic to give the default group priority when 
assigning counters?

How about something like this?

diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
index 9e705eafe873..5f3711d78a19 100644
--- a/fs/resctrl/monitor.c
+++ b/fs/resctrl/monitor.c
@@ -1339,8 +1339,10 @@ static void resctrl_assign_cntrs_allrdtgrp(void)
  {
         struct rdtgroup *prgrp, *crgrp;

+       rdtgroup_assign_cntrs(&rdtgroup_default);
         list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
-               rdtgroup_assign_cntrs(prgrp);
+               if(prgrp != &rdtgroup_default)
+                       rdtgroup_assign_cntrs(prgrp);

                 list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, 
mon.crdtgrp_list)
                         rdtgroup_assign_cntrs(crgrp);


> 
>> +		 */
>> +		if (enable) {
>> +			r->mon.mbm_assign_on_mkdir = true;
>> +			resctrl_assign_cntrs_allrdtgrp();
>> +		}
>>   	}
>>   
>>   out_unlock:
> 
> Reinette
> 

Thanks
Babu