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

[Reinette Chatre <reinette.chatre@intel.com>]

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

> 
> 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.

> 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.

> 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