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

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

Hi Reinette,

On 10/12/26 21:16, Reinette Chatre wrote:
> 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?

It is just the default group. Not the monitoring group under 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.

ok.

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

Ok.
Thanks
Babu