[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e55460eb-0679-479a-94d0-fe564cc77b79@amd.com>
Date: Thu, 26 Jun 2025 20:34:04 -0500
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>,
Babu Moger <babu.moger@....com>, corbet@....net, tony.luck@...el.com,
Dave.Martin@....com, james.morse@....com, tglx@...utronix.de,
mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com
Cc: x86@...nel.org, hpa@...or.com, akpm@...ux-foundation.org,
rostedt@...dmis.org, paulmck@...nel.org, thuth@...hat.com, ardb@...nel.org,
gregkh@...uxfoundation.org, seanjc@...gle.com, thomas.lendacky@....com,
pawan.kumar.gupta@...ux.intel.com, manali.shukla@....com,
perry.yuan@....com, kai.huang@...el.com, peterz@...radead.org,
xiaoyao.li@...el.com, kan.liang@...ux.intel.com, mario.limonciello@....com,
xin3.li@...el.com, gautham.shenoy@....com, xin@...or.com,
chang.seok.bae@...el.com, fenghuay@...dia.com, peternewman@...gle.com,
maciej.wieczor-retman@...el.com, eranian@...gle.com,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v14 20/32] fs/resctrl: Report 'Unassigned' for MBM events
in mbm_event mode
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@....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
Powered by blists - more mailing lists