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

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

Hi Reinette,

On 10/6/26 12:51, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/2/26 2:26 PM, Babu Moger wrote:
>> 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 mbm_assign_on_mkdir for subsequent mkdir, but
>> does not assign counters to groups that already exist, including the
>> default group created at mount. The counters of those groups return
>> "Unassigned" until the user assigns counters by hand.
> 
> Above summarizes the context and then below follows with the change,
> what makes this deserve a "Fixes:" tag? What is the issue needing fixing?
> 
> 
> Consider, for example:
> 
>        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 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.
> 

Looks good. Thanks

>>
>> Enable mbm_assign_on_mkdir and assign counters, while there are some
>> available, to existing CTRL_MON and MON groups so the switch matches
>> mkdir auto-assignment. An event left without a counter reads
>> "Unassigned".
>>
>> 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
>> Cc: stable@vger.kernel.org
> 
> tip tag order places "Cc:" after author's SoB.

ack.

> 
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> 
>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index 73413cb128ea..fca0734bf346 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -1300,8 +1300,7 @@ 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 MBM events.
> 
> This patch adds another caller of rdtgroup_assign_cntrs() and above thus correctly
> updates the function comments to not just mention the original caller ...
> 
>>    *
>>    * 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
> 
> ... but the detailed function comments trimmed here still considers original
> caller as the only use case and uses original caller to justify why it does not
> need to return an error.
> 
How about this?

/*
  * rdtgroup_assign_cntrs() - Assign counters to MBM events.
  *
  * 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 return failure.
  * Users have the option to modify the counter assignments later if needed.
  */

Thanks
Ban