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
