Reading the monitoring data requires RMID, CLOSID, and event ID, among
other parameters. These are passed individually, resulting in architecture
specific function calls.

Passing the pointer to the full rdtgroup structure simplifies access to
these parameters.

Additionally, when "mbm_event" 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.

Refactor the code to pass a pointer to struct rdtgroup instead of
individual members in preparation for this requirement.

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v14: Few text update to commit log.

v13: New patch to pass the entire struct rdtgroup to __mon_event_count(),
     mbm_update(), and related functions.
---
 fs/resctrl/monitor.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
index cf7f6a22ea51..31e08d891db2 100644
--- a/fs/resctrl/monitor.c
+++ b/fs/resctrl/monitor.c
@@ -356,9 +356,11 @@ static struct mbm_state *get_mbm_state(struct rdt_mon_domain *d, u32 closid,
 	return state ? &state[idx] : NULL;
 }
 
-static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
+static int __mon_event_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
 {
 	int cpu = smp_processor_id();
+	u32 closid = rdtgrp->closid;
+	u32 rmid = rdtgrp->mon.rmid;
 	struct rdt_mon_domain *d;
 	struct cacheinfo *ci;
 	struct mbm_state *m;
@@ -429,9 +431,11 @@ static int __mon_event_count(u32 closid, u32 rmid, struct rmid_read *rr)
  * __mon_event_count() is compared with the chunks value from the previous
  * invocation. This must be called once per second to maintain values in MBps.
  */
-static void mbm_bw_count(u32 closid, u32 rmid, struct rmid_read *rr)
+static void mbm_bw_count(struct rdtgroup *rdtgrp, struct rmid_read *rr)
 {
 	u64 cur_bw, bytes, cur_bytes;
+	u32 closid = rdtgrp->closid;
+	u32 rmid = rdtgrp->mon.rmid;
 	struct mbm_state *m;
 
 	m = get_mbm_state(rr->d, closid, rmid, rr->evtid);
@@ -460,7 +464,7 @@ void mon_event_count(void *info)
 
 	rdtgrp = rr->rgrp;
 
-	ret = __mon_event_count(rdtgrp->closid, rdtgrp->mon.rmid, rr);
+	ret = __mon_event_count(rdtgrp, rr);
 
 	/*
 	 * For Ctrl groups read data from child monitor groups and
@@ -471,8 +475,7 @@ void mon_event_count(void *info)
 
 	if (rdtgrp->type == RDTCTRL_GROUP) {
 		list_for_each_entry(entry, head, mon.crdtgrp_list) {
-			if (__mon_event_count(entry->closid, entry->mon.rmid,
-					      rr) == 0)
+			if (__mon_event_count(entry, rr) == 0)
 				ret = 0;
 		}
 	}
@@ -603,7 +606,7 @@ static void update_mba_bw(struct rdtgroup *rgrp, struct rdt_mon_domain *dom_mbm)
 }
 
 static void mbm_update_one_event(struct rdt_resource *r, struct rdt_mon_domain *d,
-				 u32 closid, u32 rmid, enum resctrl_event_id evtid)
+				 struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
 {
 	struct rmid_read rr = {0};
 
@@ -617,30 +620,30 @@ static void mbm_update_one_event(struct rdt_resource *r, struct rdt_mon_domain *
 		return;
 	}
 
-	__mon_event_count(closid, rmid, &rr);
+	__mon_event_count(rdtgrp, &rr);
 
 	/*
 	 * If the software controller is enabled, compute the
 	 * bandwidth for this event id.
 	 */
 	if (is_mba_sc(NULL))
-		mbm_bw_count(closid, rmid, &rr);
+		mbm_bw_count(rdtgrp, &rr);
 
 	resctrl_arch_mon_ctx_free(rr.r, rr.evtid, rr.arch_mon_ctx);
 }
 
 static void mbm_update(struct rdt_resource *r, struct rdt_mon_domain *d,
-		       u32 closid, u32 rmid)
+		       struct rdtgroup *rdtgrp)
 {
 	/*
 	 * This is protected from concurrent reads from user as both
 	 * the user and overflow handler hold the global mutex.
 	 */
 	if (resctrl_is_mon_event_enabled(QOS_L3_MBM_TOTAL_EVENT_ID))
-		mbm_update_one_event(r, d, closid, rmid, QOS_L3_MBM_TOTAL_EVENT_ID);
+		mbm_update_one_event(r, d, rdtgrp, QOS_L3_MBM_TOTAL_EVENT_ID);
 
 	if (resctrl_is_mon_event_enabled(QOS_L3_MBM_LOCAL_EVENT_ID))
-		mbm_update_one_event(r, d, closid, rmid, QOS_L3_MBM_LOCAL_EVENT_ID);
+		mbm_update_one_event(r, d, rdtgrp, QOS_L3_MBM_LOCAL_EVENT_ID);
 }
 
 /*
@@ -713,11 +716,11 @@ void mbm_handle_overflow(struct work_struct *work)
 	d = container_of(work, struct rdt_mon_domain, mbm_over.work);
 
 	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
-		mbm_update(r, d, prgrp->closid, prgrp->mon.rmid);
+		mbm_update(r, d, prgrp);
 
 		head = &prgrp->mon.crdtgrp_list;
 		list_for_each_entry(crgrp, head, mon.crdtgrp_list)
-			mbm_update(r, d, crgrp->closid, crgrp->mon.rmid);
+			mbm_update(r, d, crgrp);
 
 		if (is_mba_sc(NULL))
 			update_mba_bw(prgrp, d);
-- 
2.34.1

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

> 
> Passing the pointer to the full rdtgroup structure simplifies access to
> these parameters.
> 
> Additionally, when "mbm_event" 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.
> 
> Refactor the code to pass a pointer to struct rdtgroup instead of
> individual members in preparation for this requirement.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

Reinette

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

The functions resctrl_arch_reset_rmid() and resctrl_arch_rmid_read()
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.

Additionally, 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."


Thanks
Babu

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

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

Hi Babu,

On 6/30/25 1:58 PM, Moger, Babu wrote:
> 
> 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."

This looks good. I made a few adjustments that result in below. What do you think?

  Reading monitoring data for a monitoring group requires both the RMID and
  CLOSID. The RMID and CLOSID are members of struct rdtgroup but passed
  separately to several functions involved in retrieving event data.

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

  Provide a pointer to the struct rdtgroup as parameter to functions
  involved in retrieving event data to simplify access to RMID, CLOSID,
  and counter ID.
 
Reinette

Hi Reinette,

On 6/30/2025 4:59 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/30/25 1:58 PM, Moger, Babu wrote:
>>
>> 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."
> 
> This looks good. I made a few adjustments that result in below. What do you think?

Looks good. Thanks

> 
>    Reading monitoring data for a monitoring group requires both the RMID and
>    CLOSID. The RMID and CLOSID are members of struct rdtgroup but passed
>    separately to several functions involved in retrieving event data.
> 
>    When "mbm_event" counter assignment mode is enabled, a counter ID is
>    required to read event data. The counter ID is obtained through
>    mbm_cntr_get(), which expects a struct rdtgroup pointer.
> 
>    Provide a pointer to the struct rdtgroup as parameter to functions
>    involved in retrieving event data to simplify access to RMID, CLOSID,
>    and counter ID.
>   
> Reinette
> 
> 

-Babu