Re: [PATCH v14 13/32] fs/resctrl: Introduce mbm_cntr_cfg to track assignable counters per domain

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

Hi Reinette,

On 6/26/25 10:05, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/25/25 6:31 PM, Moger, Babu wrote:
>> On 6/24/2025 6:31 PM, Reinette Chatre wrote:
>>> On 6/13/25 2:04 PM, Babu Moger wrote:
> 
>>>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>>>> index f078ef24a8ad..468a4ebabc64 100644
>>>> --- a/include/linux/resctrl.h
>>>> +++ b/include/linux/resctrl.h
>>>> @@ -156,6 +156,22 @@ struct rdt_ctrl_domain {
>>>>       u32                *mbps_val;
>>>>   };
>>>>   +/**
>>>> + * struct mbm_cntr_cfg - Assignable counter configuration.
>>>> + * @evtid:        MBM event to which the counter is assigned. Only valid
>>>> + *            if @rdtgroup is not NULL.
>>>> + * @evt_cfg:        Event configuration created using the READS_TO_LOCAL_MEM,
>>>> + *            READS_TO_REMOTE_MEM, etc. bits that represent the memory
>>>> + *            transactions being counted.
>>>> + * @rdtgrp:        resctrl group assigned to the counter. NULL if the
>>>> + *            counter is free.
>>>> + */
>>>> +struct mbm_cntr_cfg {
>>>> +    enum resctrl_event_id    evtid;
>>>> +    u32            evt_cfg;
>>>
>>> It is not clear to me why the event configuration needs to be duplicated
>>> between mbm_cntr_cfg::evt_cfg and mon_evt::evt_cfg (done in patch #16).
>>> I think there should be only one "source of truth" and mon_evt::evt_cfg
>>> seems most appropriate since then it can be shared with BMEC.
>>>
>>> It also seems unnecessary to make so many copies of the event configuration
>>> if it can just be determined from the event ID.
>>>
>>> Looking ahead at how this is used, for example in event_filter_write()
>>> introduced in patch #25:
>>>     ret = resctrl_process_configs(buf, &evt_cfg);
>>>     if (!ret && mevt->evt_cfg != evt_cfg) {
>>>         mevt->evt_cfg = evt_cfg;
>>>         resctrl_assign_cntr_allrdtgrp(r, mevt);
>>>     }
>>>
>>> After user provides new event configuration the mon_evt::evt_cfg is
>>> updated. Since there is this initial check to determine if counters need
>>> to be updated I think it is unnecessary to have a second copy of mbm_cntr_cfg::evt_cfg
>>> that needs to be checked again. The functions called by resctrl_assign_cntr_allrdtgrp(r, mevt)
>>> should just update the counters without any additional comparison.
>>>
>>> For example, rdtgroup_assign_cntr() can be simplified to:
>>>     rdtgroup_assign_cntr() {
>>>         ...
>>>         list_for_each_entry(d, &r->mon_domains, hdr.list) {
>>>             cntr_id = mbm_cntr_get(r, d, rdtgrp, mevt->evtid);
>>>             if (cntr_id >= 0)
>>>                 resctrl_arch_config_cntr(r, d, mevt->evtid, rdtgrp->mon.rmid,
>>>                              rdtgrp->closid, cntr_id, true);
>>>         }
>>>     }
>>>
>>>
>>
>> Actually, this interaction works as intended.
>>
>> It serves as an optimization for cases where the user repeatedly tries to assign the same event to a group. Since we have no way of knowing whether the event is up-to-date, this mechanism helps us avoid unnecessary MSR writes.
>>
>> For example:
>> mbm_L3_assignments_write() → resctrl_assign_cntr_event() → resctrl_alloc_config_cntr() → resctrl_config_cntr() → resctrl_arch_config_cntr()
>>
>>
>> resctrl_alloc_config_cntr()
>>
>> {
>> ..
>>
>> /*
>>  * Skip reconfiguration if the event setup is current; otherwise,
>>  * update and apply the new configuration to the domain.
>>  */
>>  if (mevt->evt_cfg != d->cntr_cfg[cntr_id].evt_cfg) {
>>      d->cntr_cfg[cntr_id].evt_cfg = mevt->evt_cfg;
>>      resctrl_config_cntr(r, d, mevt->evtid, rdtgrp->mon.rmid,
>>                                 rdtgrp->closid, cntr_id, true);
>>    }
>> }
> 
> This ties in with the feedback to patch #18 where this snippet is
> introduced. Please see 
> https://lore.kernel.org/lkml/77ce3646-2213-4987-a438-a69f6d7c6cfd@intel.com/
> 
> It is not clear to me that this reconfiguration should be done, if the
> counter is assigned to a group then it should be up to date, no? If there
> was any change in configuration after assignment then event_filter_write()
> will ensure that all resource groups are updated.

Yes. That is the good point. We can do that. I think we started this code
before we introduced event_filter_write().
> 
> If a user repeatedly assigns the same event to a group then mbm_cntr_get()
> will return a valid counter and resctrl_alloc_config_cntr() in above flow
> can just return success without doing a reconfigure.

Sure. We can do that. Will remove evt_cfg from struct mbm_cntr_cfg.

That for pointing this out.
-- 
Thanks
Babu Moger