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@intel.com/)
> 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@intel.com/),
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@amd.com>
>> ---
>
>
>> ---
>> 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