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