Re: [PATCH v14 21/32] fs/resctrl: Pass entire struct rdtgroup rather than passing individual members

["Moger, Babu" <babu.moger@amd.com>]

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