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