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