When "mbm_event" mode is enabled, a hardware counter must be assigned to
read the event.

Report 'Unassigned' in case the user attempts to read the event without
assigning a hardware counter.

Export mbm_cntr_get() to allow usage from other functions within
fs/resctrl.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v14: Updated the changelog.
     Added the code comment for "-ENOENT" when counter is read without assignement.
     Removed the references to resctrl_is_mbm_event().

v13: Minor commitlog and user doc update.
     Resolved conflicts caused by the recent FS/ARCH code restructure.
     The monitor.c/rdtgroup.c files have been split between the FS and ARCH directories.

v12: Updated the documentation for more clarity.

v11: Domain can be NULL with SNC support so moved the unassign check in
     rdtgroup_mondata_show().

v10: Moved the code to check the assign state inside mon_event_read().
     Fixed few text comments.

v9: Used is_mbm_event() to check the event type.
    Minor user documentation update.

v8: Used MBM_EVENT_ARRAY_INDEX to get the index for the MBM event.
    Documentation update to make the text generic.

v7: Moved the documentation under "mon_data".
    Updated the text little bit.

v6: Added more explaination in the resctrl.rst
    Added checks to detect "Unassigned" before reading RMID.

v5: New patch.
---
 Documentation/filesystems/resctrl.rst |  8 ++++++++
 fs/resctrl/ctrlmondata.c              | 19 ++++++++++++++++++-
 fs/resctrl/internal.h                 |  2 ++
 fs/resctrl/monitor.c                  |  4 ++--
 4 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
index 8a2050098091..18de335e1ff8 100644
--- a/Documentation/filesystems/resctrl.rst
+++ b/Documentation/filesystems/resctrl.rst
@@ -434,6 +434,14 @@ When monitoring is enabled all MON groups will also contain:
 	for the L3 cache they occupy). These are named "mon_sub_L3_YY"
 	where "YY" is the node number.
 
+	The "mbm_event" mode offers "num_mbm_cntrs" number of counters and
+	allows users to assign counter IDs to mon_hw_id, event pairs enabling
+	bandwidth monitoring for as long as the counter remains assigned. The
+	hardware will continue tracking the assigned mon_hw_id until the user
+	manually unassigns it, ensuring that event data is not reset during this
+	period. An MBM event returns 'Unassigned' when the event does not have
+	a hardware counter assigned.
+
 "mon_hw_id":
 	Available only with debug option. The identifier used by hardware
 	for the monitor group. On x86 this is the RMID.
diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
index ad7ffc6acf13..8a182f506877 100644
--- a/fs/resctrl/ctrlmondata.c
+++ b/fs/resctrl/ctrlmondata.c
@@ -648,15 +648,32 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
 			goto out;
 		}
 		d = container_of(hdr, struct rdt_mon_domain, hdr);
+
+		/*
+		 * Report 'Unassigned' if "mbm_event" mode is enabled and counter
+		 * is unassigned.
+		 */
+		if (resctrl_arch_mbm_cntr_assign_enabled(r) &&
+		    resctrl_is_mbm_event(evtid) &&
+		    (mbm_cntr_get(r, d, rdtgrp, evtid) < 0)) {
+			rr.err = -ENOENT;
+			goto checkresult;
+		}
+
 		mon_event_read(&rr, r, d, rdtgrp, &d->hdr.cpu_mask, evtid, false);
 	}
 
 checkresult:
-
+	/*
+	 * -ENOENT is a special case, set only when "mbm_event" mode is enabled
+	 * and no counter has been assigned.
+	 */
 	if (rr.err == -EIO)
 		seq_puts(m, "Error\n");
 	else if (rr.err == -EINVAL)
 		seq_puts(m, "Unavailable\n");
+	else if (rr.err == -ENOENT)
+		seq_puts(m, "Unassigned\n");
 	else
 		seq_printf(m, "%llu\n", rr.val);
 
diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
index 4496c359ac22..4a7130018aa1 100644
--- a/fs/resctrl/internal.h
+++ b/fs/resctrl/internal.h
@@ -390,6 +390,8 @@ int resctrl_assign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d,
 			      struct rdtgroup *rdtgrp, struct mon_evt *mevt);
 void resctrl_unassign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d,
 				 struct rdtgroup *rdtgrp, struct mon_evt *mevt);
+int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
+		 struct rdtgroup *rdtgrp, enum resctrl_event_id evtid);
 
 #ifdef CONFIG_RESCTRL_FS_PSEUDO_LOCK
 int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
index f2636aea6545..cf7f6a22ea51 100644
--- a/fs/resctrl/monitor.c
+++ b/fs/resctrl/monitor.c
@@ -977,8 +977,8 @@ static void resctrl_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d
  * Return:
  * Valid counter ID on success, or -ENOENT on failure.
  */
-static int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
-			struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
+int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
+		 struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
 {
 	int cntr_id;
 
-- 
2.34.1

Hi Babu,

On 6/13/25 2:05 PM, Babu Moger wrote:
> When "mbm_event" mode is enabled, a hardware counter must be assigned to

"When the "mbm_event" counter assignment mode is enabled ..."

> read the event.
> 
> Report 'Unassigned' in case the user attempts to read the event without
> assigning a hardware counter.
> 
> Export mbm_cntr_get() to allow usage from other functions within

"Export" can be a loaded term in the Linux kernel. Perhaps:
"Export mbm_cntr_get() ... " -> "Declare mbm_cntr_get() in fs/resctrl/internal.h ..."

> fs/resctrl.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> ---
>  Documentation/filesystems/resctrl.rst |  8 ++++++++
>  fs/resctrl/ctrlmondata.c              | 19 ++++++++++++++++++-
>  fs/resctrl/internal.h                 |  2 ++
>  fs/resctrl/monitor.c                  |  4 ++--
>  4 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 8a2050098091..18de335e1ff8 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -434,6 +434,14 @@ When monitoring is enabled all MON groups will also contain:
>  	for the L3 cache they occupy). These are named "mon_sub_L3_YY"
>  	where "YY" is the node number.
>  
> +	The "mbm_event" mode offers "num_mbm_cntrs" number of counters and

"The "mbm_event" mode" -> "The "mbm_event" counter assignment mode"?

> +	allows users to assign counter IDs to mon_hw_id, event pairs enabling

"users to assign counter IDs" -> "users to assign counters"

> +	bandwidth monitoring for as long as the counter remains assigned. The
> +	hardware will continue tracking the assigned mon_hw_id until the user

"assigned mon_hw_id" -> "assigned counter"?

> +	manually unassigns it, ensuring that event data is not reset during this
> +	period. An MBM event returns 'Unassigned' when the event does not have
> +	a hardware counter assigned.
> +
>  "mon_hw_id":
>  	Available only with debug option. The identifier used by hardware
>  	for the monitor group. On x86 this is the RMID.
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index ad7ffc6acf13..8a182f506877 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -648,15 +648,32 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>  			goto out;
>  		}
>  		d = container_of(hdr, struct rdt_mon_domain, hdr);
> +
> +		/*
> +		 * Report 'Unassigned' if "mbm_event" mode is enabled and counter
> +		 * is unassigned.
> +		 */
> +		if (resctrl_arch_mbm_cntr_assign_enabled(r) &&
> +		    resctrl_is_mbm_event(evtid) &&
> +		    (mbm_cntr_get(r, d, rdtgrp, evtid) < 0)) {
> +			rr.err = -ENOENT;
> +			goto checkresult;
> +		}
> +

When looking at this snippet in combination with patch #22 that adds the support for
reading counters the flow does not look ideal. While above adds a check whether
this is dealing with counters, it only does so to check if a counter is *not* assigned.
I cannot see *any* other check by resctrl whether it is dealing with counters while
it lumps all information into parameters to resctrl_arch_reset_rmid() and
resctrl_arch_rmid_read(), needing to provide "dummy" parameters when not all information
is relevant, and leaving the arch to need to determine if it is
dealing with counters and then use provided parameters based on that information.

I think it will be simpler for resctrl to determine if a counter or RMID needs to be
read and then call appropriate arch API for each and provide only necessary information
to support that call.

I think this can be accomplished with following changes:
- drop above snippet from rdtgroup_mondata_show() (this will be done in mon_event_read())
- introduce new rmid_read::is_cntr that is a boolean that is true if it is a counter
  that should be read.
- mon_event_read() initializes rmid_read::is_cntr and returns with rmid_read::err
  set if a counter should be read but no counter is assigned (above snippet). The
  added benefit of doing this in mon_event_read() is that if a counter is not
  assigned on new monitor group create or domain add then the mon_add_all_files()->mon_event_read()
  will return immediately with this error instead of trying to read the unassigned
  counter.
- __mon_event_count() should *only* attempt to initialize the counter ID (call mbm_cntr_get)
  if rmid_read::is_cntr is true. 
- Introduce two new arch calls (naming TBD):
  resctrl_arch_cntr_read() and resctrl_arch_reset_cntr() that will respectively read
  and reset the counter.
- __mon_event_count() calls appropriate API based on rmid_read::is_cntr.

What do you think?

Reinette

Hi Reinette,

On 6/24/2025 11:14 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/13/25 2:05 PM, Babu Moger wrote:
>> When "mbm_event" mode is enabled, a hardware counter must be assigned to
> 
> "When the "mbm_event" counter assignment mode is enabled ..."

Sure.

> 
>> read the event.
>>
>> Report 'Unassigned' in case the user attempts to read the event without
>> assigning a hardware counter.
>>
>> Export mbm_cntr_get() to allow usage from other functions within
> 
> "Export" can be a loaded term in the Linux kernel. Perhaps:
> "Export mbm_cntr_get() ... " -> "Declare mbm_cntr_get() in fs/resctrl/internal.h ..."
> 

Sure.

>> fs/resctrl.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> ...
> 
>> ---
>>   Documentation/filesystems/resctrl.rst |  8 ++++++++
>>   fs/resctrl/ctrlmondata.c              | 19 ++++++++++++++++++-
>>   fs/resctrl/internal.h                 |  2 ++
>>   fs/resctrl/monitor.c                  |  4 ++--
>>   4 files changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
>> index 8a2050098091..18de335e1ff8 100644
>> --- a/Documentation/filesystems/resctrl.rst
>> +++ b/Documentation/filesystems/resctrl.rst
>> @@ -434,6 +434,14 @@ When monitoring is enabled all MON groups will also contain:
>>   	for the L3 cache they occupy). These are named "mon_sub_L3_YY"
>>   	where "YY" is the node number.
>>   
>> +	The "mbm_event" mode offers "num_mbm_cntrs" number of counters and
> 
> "The "mbm_event" mode" -> "The "mbm_event" counter assignment mode"?

Sure.

> 
>> +	allows users to assign counter IDs to mon_hw_id, event pairs enabling
> 
> "users to assign counter IDs" -> "users to assign counters"
> 

Sure.

>> +	bandwidth monitoring for as long as the counter remains assigned. The
>> +	hardware will continue tracking the assigned mon_hw_id until the user
> 
> "assigned mon_hw_id" -> "assigned counter"?
> 

Sure.

>> +	manually unassigns it, ensuring that event data is not reset during this
>> +	period. An MBM event returns 'Unassigned' when the event does not have
>> +	a hardware counter assigned.
>> +
>>   "mon_hw_id":
>>   	Available only with debug option. The identifier used by hardware
>>   	for the monitor group. On x86 this is the RMID.
>> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
>> index ad7ffc6acf13..8a182f506877 100644
>> --- a/fs/resctrl/ctrlmondata.c
>> +++ b/fs/resctrl/ctrlmondata.c
>> @@ -648,15 +648,32 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
>>   			goto out;
>>   		}
>>   		d = container_of(hdr, struct rdt_mon_domain, hdr);
>> +
>> +		/*
>> +		 * Report 'Unassigned' if "mbm_event" mode is enabled and counter
>> +		 * is unassigned.
>> +		 */
>> +		if (resctrl_arch_mbm_cntr_assign_enabled(r) &&
>> +		    resctrl_is_mbm_event(evtid) &&
>> +		    (mbm_cntr_get(r, d, rdtgrp, evtid) < 0)) {
>> +			rr.err = -ENOENT;
>> +			goto checkresult;
>> +		}
>> +
> 
> When looking at this snippet in combination with patch #22 that adds the support for
> reading counters the flow does not look ideal. While above adds a check whether
> this is dealing with counters, it only does so to check if a counter is *not* assigned.
> I cannot see *any* other check by resctrl whether it is dealing with counters while
> it lumps all information into parameters to resctrl_arch_reset_rmid() and
> resctrl_arch_rmid_read(), needing to provide "dummy" parameters when not all information
> is relevant, and leaving the arch to need to determine if it is
> dealing with counters and then use provided parameters based on that information.
> 
> I think it will be simpler for resctrl to determine if a counter or RMID needs to be
> read and then call appropriate arch API for each and provide only necessary information
> to support that call.
> 
> I think this can be accomplished with following changes:
> - drop above snippet from rdtgroup_mondata_show() (this will be done in mon_event_read())
> - introduce new rmid_read::is_cntr that is a boolean that is true if it is a counter
>    that should be read.
> - mon_event_read() initializes rmid_read::is_cntr and returns with rmid_read::err
>    set if a counter should be read but no counter is assigned (above snippet). The
>    added benefit of doing this in mon_event_read() is that if a counter is not
>    assigned on new monitor group create or domain add then the mon_add_all_files()->mon_event_read()
>    will return immediately with this error instead of trying to read the unassigned
>    counter.
> - __mon_event_count() should *only* attempt to initialize the counter ID (call mbm_cntr_get)
>    if rmid_read::is_cntr is true.
> - Introduce two new arch calls (naming TBD):
>    resctrl_arch_cntr_read() and resctrl_arch_reset_cntr() that will respectively read
>    and reset the counter.

It may be necessary to restructure resctrl_arch_cntr_read(), as there is 
some shared logic that applies to both resctrl_arch_rmid_read() and 
resctrl_arch_cntr_read().


> - __mon_event_count() calls appropriate API based on rmid_read::is_cntr.
> 
> What do you think?

Sounds good to me—this seems like a much cleaner approach. I’ll start 
making the changes on Monday(out of the office tomorrow). I’ll let you 
know if I run into any issues. I might post snippet if necessary.

Thanks
Babu