[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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