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

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

Hi Babu,

On 10/12/26 6:02 PM, Moger, Babu wrote:
> On 10/12/2026 1:44 PM, Reinette Chatre wrote:
>> On 10/9/26 4:44 PM, Babu Moger wrote:

...

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

In general I think the special treatment results from the default group
always being present. It is not clear to me that this implies that the default
group needs to have counters assigned when switching to mbm_event mode.


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

I do not find there to be a strong motivation for this assignment but it
should be fine to establish such expectation at this point. Just to confirm, you
explicitly only consider the default group and not also the monitoring groups
belonging to the default group?

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

ok. If you go this route, please add a small comment to highlight why the list
traversal is done in this unexpected way. If you do so, please take care with
potential duplicate comments between this function and its caller.

I do not have a preference about which groups need counters assigned when
switching to mbm_event mode. Please just ensure that the documentation (resctrl.rst,
changelog, code comments) match the behavior and explains why.

Thank you very much.

Reinette