[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <13297090-926c-4585-95ac-02a865ad1e49@intel.com>
Date: Thu, 20 Jun 2024 15:50:28 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: <babu.moger@....com>, <corbet@....net>, <fenghua.yu@...el.com>,
<tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
<dave.hansen@...ux.intel.com>
CC: <x86@...nel.org>, <hpa@...or.com>, <paulmck@...nel.org>,
<rdunlap@...radead.org>, <tj@...nel.org>, <peterz@...radead.org>,
<yanjiewtw@...il.com>, <kim.phillips@....com>, <lukas.bulwahn@...il.com>,
<seanjc@...gle.com>, <jmattson@...gle.com>, <leitao@...ian.org>,
<jpoimboe@...nel.org>, <rick.p.edgecombe@...el.com>,
<kirill.shutemov@...ux.intel.com>, <jithu.joseph@...el.com>,
<kai.huang@...el.com>, <kan.liang@...ux.intel.com>,
<daniel.sneddon@...ux.intel.com>, <pbonzini@...hat.com>,
<sandipan.das@....com>, <ilpo.jarvinen@...ux.intel.com>,
<peternewman@...gle.com>, <maciej.wieczor-retman@...el.com>,
<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<eranian@...gle.com>, <james.morse@....com>
Subject: Re: [PATCH v4 14/19] x86/resctrl: Add the interface to assign ABMC
counter
Hi Babu,
On 6/19/24 3:38 PM, Moger, Babu wrote:
> On 6/13/2024 8:48 PM, Reinette Chatre wrote:
>> On 5/24/24 5:23 AM, Babu Moger wrote:
>>> +int resctrl_arch_assign(struct rdt_domain *d, u32 evtid, u32 rmid,
>>> + u32 ctr_id, u32 closid, bool enable)
>>> +{
>>> + struct rdt_hw_domain *hw_dom = resctrl_to_arch_dom(d);
>>> + union l3_qos_abmc_cfg abmc_cfg = { 0 };
>>> + struct arch_mbm_state *arch_mbm;
>>> +
>>> + abmc_cfg.split.cfg_en = 1;
>>> + abmc_cfg.split.ctr_en = enable ? 1 : 0;
>>> + abmc_cfg.split.ctr_id = ctr_id;
>>> + abmc_cfg.split.bw_src = rmid;
>>> +
>>> + /*
>>> + * Read the event configuration from the domain and pass it as
>>> + * bw_type.
>>> + */
>>> + if (evtid == QOS_L3_MBM_TOTAL_EVENT_ID) {
>>> + abmc_cfg.split.bw_type = hw_dom->mbm_total_cfg;
>>> + arch_mbm = &hw_dom->arch_mbm_total[rmid];
>>> + } else {
>>> + abmc_cfg.split.bw_type = hw_dom->mbm_local_cfg;
>>> + arch_mbm = &hw_dom->arch_mbm_local[rmid];
>>> + }
>>> +
>>> + smp_call_function_any(&d->cpu_mask, rdtgroup_abmc_cfg, &abmc_cfg, 1);
>>> +
>>> + /* Reset the internal counters */
>>
>> "internal counters"? This needs a definition ... but since this is not
>> a new data structure the comment can be more specific about what is done
>> and why.
>
> How about?
> /* Reset internal mbm event counters. */
I think it is potentially confusing to introduce a new concept of
"internal counters". This is just the architectural state helping to
detect overflows. This commit has no mention of this but I assume that
the re-configuration of the counter will reset it, which explains
why it is required to reset the state maintained by software.
A similar comment to what can be found in mbm_config_write_domain()
would be useful. This does not have to be so elaborate, maybe just something
like (please feel free to improve):
Configuring a hardware counter resets it. Reset architectural
state to match.
Reinette
Powered by blists - more mailing lists