Message-ID: <b4b14670-9cb0-4f65-abd5-39db996e8da9@intel.com>
Date: Tue, 24 Jun 2025 21:14:15 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: 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 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@....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

Powered by blists - more mailing lists