Message-ID: <f80f254c-af8b-4d7a-96cc-e8aa569b6d45@amd.com>
Date: Mon, 30 Jun 2025 15:58:25 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
 "corbet@....net" <corbet@....net>, "tony.luck@...el.com"
 <tony.luck@...el.com>, "Dave.Martin@....com" <Dave.Martin@....com>,
 "james.morse@....com" <james.morse@....com>,
 "tglx@...utronix.de" <tglx@...utronix.de>,
 "mingo@...hat.com" <mingo@...hat.com>, "bp@...en8.de" <bp@...en8.de>,
 "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
Cc: "x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
 "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
 "rostedt@...dmis.org" <rostedt@...dmis.org>,
 "paulmck@...nel.org" <paulmck@...nel.org>,
 "thuth@...hat.com" <thuth@...hat.com>, "ardb@...nel.org" <ardb@...nel.org>,
 "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
 "seanjc@...gle.com" <seanjc@...gle.com>,
 "Lendacky, Thomas" <Thomas.Lendacky@....com>,
 "pawan.kumar.gupta@...ux.intel.com" <pawan.kumar.gupta@...ux.intel.com>,
 "Shukla, Manali" <Manali.Shukla@....com>, "Yuan, Perry"
 <Perry.Yuan@....com>, "kai.huang@...el.com" <kai.huang@...el.com>,
 "peterz@...radead.org" <peterz@...radead.org>,
 "xiaoyao.li@...el.com" <xiaoyao.li@...el.com>,
 "kan.liang@...ux.intel.com" <kan.liang@...ux.intel.com>,
 "Limonciello, Mario" <Mario.Limonciello@....com>,
 "xin3.li@...el.com" <xin3.li@...el.com>,
 "Shenoy, Gautham Ranjal" <gautham.shenoy@....com>,
 "xin@...or.com" <xin@...or.com>,
 "chang.seok.bae@...el.com" <chang.seok.bae@...el.com>,
 "fenghuay@...dia.com" <fenghuay@...dia.com>,
 "peternewman@...gle.com" <peternewman@...gle.com>,
 "maciej.wieczor-retman@...el.com" <maciej.wieczor-retman@...el.com>,
 "eranian@...gle.com" <eranian@...gle.com>,
 "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v14 21/32] fs/resctrl: Pass entire struct rdtgroup rather
 than passing individual members

Hi Reinette,

On 6/30/25 10:44, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/30/25 6:57 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 6/24/2025 11:18 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 6/13/25 2:05 PM, Babu Moger wrote:
>>>> Reading the monitoring data requires RMID, CLOSID, and event ID, among
>>>> other parameters. These are passed individually, resulting in architecture
>>>
>>> It is not clear how "event ID" and "other parameters" are relevant to this
>>> change since (in this context) it is only RMID and CLOSID that can be
>>> found in rdtgroup.
>>>
>>>> specific function calls.
>>>
>>> Could you please elaborate what you meant with: "These are passed individually,
>>> resulting in architecture specific function calls."?
>>
>> Rephrased the whole changelog.
>>
>> "fs/resctrl: Pass the full rdtgroup structure instead of individual RMID
>> and CLOSID
> 
> nit, can be simplified to:
> 	fs/resctrl: Pass struct rdtgroup instead of individual members

sure.

> 
>>
>> The functions resctrl_arch_reset_rmid() and resctrl_arch_rmid_read()
> 
> (No need to say "function" when using ().)
> 
> But wait ... this now changes to different functions from what the original
> patch touched and even more so it changes _arch_ functions that should not
> have access to struct rdtgroup. This new changelog does not seem to document
> the original patch but something new that has not yet been posted.

No. patch has not changed.

> 
>> require several parameters, including RMID and CLOSID. Currently, RMID and
>> CLOSID are passed individually, even though they are available within the
>> rdtgroup structure.
>>
>> Refactor the code to pass a pointer to struct rdtgroup instead of
>> individual members in preparation for this requirement.
> 
> "this requirement" .. what requirement are you referring to?
> There is no requirement that individual members of a struct cannot be passed
> as separate parameters and there is no problem doing so.
> 
>>>From "Changelog" in Documentation/process/maintainer-tip.rst:
> "A good structure is to explain the context, the problem and the solution in
>  separate paragraphs and this order."
> 
> This new changelog has structure of "context, solution, problem".
> 
>>
>> Additionally, when "mbm_event" counter assignment mode is enabled, a
> 
> This seems to be primary motivation since passing struct rdtgroup will
> simplify the code (when I consider the original patch, not what this new
> changelog implies) ... but if this change is indeed to the arch API as the
> context suggest then passing the individual members is the right thing to
> do because arch code should not access struct rdtgroup.

Again.  patch did not change.
> 
>> counter ID is required to read the event. The counter ID is obtained
>> through mbm_cntr_get(), which expects a struct rdtgroup pointer."
> 
> This is even stranger. mbm_cntr_get() is private to resctrl fs while
> the new changelog describes how the arch functions resctrl_arch_reset_rmid()
> and resctrl_arch_rmid_read() need struct rdtgroup to call mbm_cntr_get()?
> 
> Reinette
> 
> 

Patch is same.. I am having trouble with changelog. ):

How does this look?

"fs/resctrl: Pass struct rdtgroup instead of individual members

Reading monitoring data for a resctrl group requires both the RMID and
CLOSID. These parameters are passed to functions like __mon_event_count(),
mbm_bw_count(), mbm_update_one_event(), and mbm_update(), where they are
ultimately used to retrieve event data.

When "mbm_event" counter assignment mode is enabled, a counter ID is
required to read the event. The counter ID is obtained through
mbm_cntr_get(), which expects a struct rdtgroup pointer.

Passing the pointer to the full rdtgroup structure simplifies access to
these parameters. Refactor the code to pass a pointer to struct rdtgroup
instead of individual members in preparation for this requirement."

-- 
Thanks
Babu Moger

Powered by blists - more mailing lists