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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e21ad30b-b6d0-476b-8090-f58a20264fe6@intel.com>
Date: Thu, 26 Jun 2025 08:05:05 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: "Moger, Babu" <bmoger@....com>, Babu Moger <babu.moger@....com>,
	<corbet@....net>, <tony.luck@...el.com>, <Dave.Martin@....com>,
	<james.morse@....com>, <tglx@...utronix.de>, <mingo@...hat.com>,
	<bp@...en8.de>, <dave.hansen@...ux.intel.com>
CC: <x86@...nel.org>, <hpa@...or.com>, <akpm@...ux-foundation.org>,
	<rostedt@...dmis.org>, <paulmck@...nel.org>, <thuth@...hat.com>,
	<ardb@...nel.org>, <gregkh@...uxfoundation.org>, <seanjc@...gle.com>,
	<thomas.lendacky@....com>, <pawan.kumar.gupta@...ux.intel.com>,
	<manali.shukla@....com>, <perry.yuan@....com>, <kai.huang@...el.com>,
	<peterz@...radead.org>, <xiaoyao.li@...el.com>, <kan.liang@...ux.intel.com>,
	<mario.limonciello@....com>, <xin3.li@...el.com>, <gautham.shenoy@....com>,
	<xin@...or.com>, <chang.seok.bae@...el.com>, <fenghuay@...dia.com>,
	<peternewman@...gle.com>, <maciej.wieczor-retman@...el.com>,
	<eranian@...gle.com>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v14 13/32] fs/resctrl: Introduce mbm_cntr_cfg to track
 assignable counters per domain

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.

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