Hi Reinette, On 5/22/25 18:01, Reinette Chatre wrote: > Hi Babu, > > On 5/15/25 3:52 PM, Babu Moger wrote: >> In mbm_cntr_assign mode, the hardware counter should be assigned to read >> the MBM events. >> >> Report 'Unassigned' in case the user attempts to read the event without >> assigning a hardware counter. >> >> Export resctrl_is_mbm_event() and mbm_cntr_get() to allow usage from other >> functions within fs/resctrl. > > Please clarify that these two functions are exposed differently, resctrl_is_mbm_event() > is added to include/linux/resctrl.h (also note similar change in > https://lore.kernel.org/lkml/20250429003359.375508-3-tony.luck@xxxxxxxxx/) > so not just exposed to fs/resctrl but instead to resctrl fs as well as > arch code > while mbm_cntr_get() remains internal to resctrl fs by being added to > fs/resctrl/internal.h. Sure. Will update the comment. With Tony's changes( https://lore.kernel.org/lkml/20250429003359.375508-3-tony.luck@xxxxxxxxx/), the resctrl_is_mbm_event() is not required in here. It is already there. I will have to update the comment only on mbm_cntr_get(). Will do. >> >> Signed-off-by: Babu Moger <babu.moger@xxxxxxx> >> --- > > >> --- >> Documentation/filesystems/resctrl.rst | 8 ++++++++ >> fs/resctrl/ctrlmondata.c | 14 ++++++++++++++ >> fs/resctrl/internal.h | 2 ++ >> fs/resctrl/monitor.c | 4 ++-- >> fs/resctrl/rdtgroup.c | 2 +- >> include/linux/resctrl.h | 1 + >> 6 files changed, 28 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst >> index 2bfad43aac9c..5cf2d742f04c 100644 >> --- a/Documentation/filesystems/resctrl.rst >> +++ b/Documentation/filesystems/resctrl.rst >> @@ -430,6 +430,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_cntr_assign mode offers "num_mbm_cntrs" number of counters >> + and allows users to assign a counter to mon_hw_id, event pair 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 counters are not reset >> + during this period. An MBM event returns 'Unassigned' when the event >> + does not have a hardware counter assigned. > > (please rework based on "event" vs "group" assignment ... not intending > that "group" assignment be documented but the "event" assignment needs > to be accurate for "group" assignment to be a simple extension) Sure. > >> + >> "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 6ed2dfd4dbbd..f6b8ad24b0b5 100644 >> --- a/fs/resctrl/ctrlmondata.c >> +++ b/fs/resctrl/ctrlmondata.c >> @@ -643,6 +643,18 @@ 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_cntr_assign 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); >> } >> >> @@ -652,6 +664,8 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg) >> 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); >> > > It may be unexpected that this is treated as "-ENOENT" but the function returns > success. This can be addressed with a comment when comparing the return codes to > other hardware return codes. Will add the comment. > >> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h >> index 64ddc107fcab..0dfd2efe68fc 100644 >> --- a/fs/resctrl/internal.h >> +++ b/fs/resctrl/internal.h >> @@ -381,6 +381,8 @@ int resctrl_assign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d, >> struct rdtgroup *rdtgrp, enum resctrl_event_id evtid); >> int resctrl_unassign_cntr_event(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); >> >> #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 fbc938bd3b23..c98a61bde179 100644 >> --- a/fs/resctrl/monitor.c >> +++ b/fs/resctrl/monitor.c >> @@ -956,8 +956,8 @@ static void resctrl_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d >> * mbm_cntr_get() - Return the cntr_id for the matching evtid and rdtgrp in >> * cntr_cfg array. >> */ >> -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; >> >> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c >> index f192b2736a77..72317a5adee2 100644 >> --- a/fs/resctrl/rdtgroup.c >> +++ b/fs/resctrl/rdtgroup.c >> @@ -127,7 +127,7 @@ static bool resctrl_is_mbm_enabled(void) >> resctrl_arch_is_mbm_local_enabled()); >> } >> >> -static bool resctrl_is_mbm_event(int e) >> +bool resctrl_is_mbm_event(int e) >> { >> return (e >= QOS_L3_MBM_TOTAL_EVENT_ID && >> e <= QOS_L3_MBM_LOCAL_EVENT_ID); >> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h >> index 59a4fe60ab46..f78b6064230c 100644 >> --- a/include/linux/resctrl.h >> +++ b/include/linux/resctrl.h >> @@ -441,6 +441,7 @@ static inline u32 resctrl_get_config_index(u32 closid, >> } >> } >> >> +bool resctrl_is_mbm_event(int e); >> bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level l); >> int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable); >> > > Reinette > > -- Thanks Babu Moger
