Message-ID: <c74dfbb4-5570-40e6-8598-5cceca87746e@intel.com>
Date: Wed, 9 Jul 2025 15:17:33 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Tony Luck <tony.luck@...el.com>, Fenghua Yu <fenghuay@...dia.com>, "Maciej
 Wieczor-Retman" <maciej.wieczor-retman@...el.com>, Peter Newman
	<peternewman@...gle.com>, James Morse <james.morse@....com>, Babu Moger
	<babu.moger@....com>, Drew Fustini <dfustini@...libre.com>, Dave Martin
	<Dave.Martin@....com>, Anil Keshavamurthy <anil.s.keshavamurthy@...el.com>,
	Chen Yu <yu.c.chen@...el.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v6 25/30] x86/resctrl: Handle number of RMIDs supported by
 telemetry resources

Hi Tony,

On 6/26/25 9:49 AM, Tony Luck wrote:
> There are now three meanings for "number of RMIDs":
> 
> 1) The number for legacy features enumerated by CPUID leaf 0xF. This
> is the maximum number of distinct values that can be loaded into the
> IA32_PQR_ASSOC MSR. Note that systems with Sub-NUMA Cluster mode enabled
> will force scaling down the CPUID enumerated value by the number of SNC
> nodes per L3-cache.
> 
> 2) The number of registers in MMIO space for each event. This
> is enumerated in the XML files and is the value initialized into
> event_group::num_rmids. This will be overwritten with a lower
> value if hardware does not support all these registers at the
> same time (see next case).
> 
> 3) The number of "h/w counters" (this isn't a strictly accurate
> description of how things work, but serves as a useful analogy that
> does describe the limitations) feeding to those MMIO registers. This
> is enumerated in telemetry_region::num_rmids returned from the call to
> intel_pmt_get_regions_by_feature()
> 
> Event groups with insufficient "h/w counter" to track all RMIDs are
> difficult for users to use, since the system may reassign "h/w counters"
> as any time. This means that users cannot reliably collect two consecutive

"as any time" -> "at any time"?

> event counts to compute the rate at which events are occurring.
> 
> Ignore such under-resourced event groups unless the user explicitly
> requests to enable them using the "rdt=" Linux boot argument.
> 
> Scan all enabled event groups and assign the RDT_RESOURCE_PERF_PKG
> resource "num_rmids" value to the smallest of these values to ensure
> that all resctrl groups have equal monitor capabilities.

The "to ensure that all resctrl groups ..." seems to describe the next patch?

> 
> N.B. Changed type of rdt_resource::num_rmids to u32 to match.

rdt_resource::num_rmids -> rdt_resource::num_rmid 

Please also check that existing code accommodates this changed type.
See for example,
	rdt_num_rmids_show() {
		...
		seq_printf(seq, "%d\n", r->num_rmid);
		...
	}

> 
> Signed-off-by: Tony Luck <tony.luck@...el.com>
> ---
>  include/linux/resctrl.h                 |  2 +-
>  arch/x86/kernel/cpu/resctrl/internal.h  |  4 ++++
>  arch/x86/kernel/cpu/resctrl/core.c      | 20 +++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 29 +++++++++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/monitor.c   |  2 ++
>  5 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index b9f2690bee1e..35ae24822493 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -288,7 +288,7 @@ struct rdt_resource {
>  	int			rid;
>  	bool			alloc_capable;
>  	bool			mon_capable;
> -	int			num_rmid;
> +	u32			num_rmid;
>  	enum resctrl_scope	ctrl_scope;
>  	enum resctrl_scope	mon_scope;
>  	struct resctrl_cache	cache;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index ee1c6204722e..11f25c225837 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -18,6 +18,8 @@
>  
>  #define RMID_VAL_UNAVAIL		BIT_ULL(62)
>  
> +extern int rdt_num_system_rmids;
> +
>  /*
>   * With the above fields in use 62 bits remain in MSR_IA32_QM_CTR for
>   * data to be returned. The counter width is discovered from the hardware
> @@ -171,6 +173,8 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
>  
>  bool rdt_is_software_feature_enabled(char *option);
>  
> +bool rdt_is_software_feature_force_enabled(char *name);
> +
>  bool intel_aet_get_events(void);
>  void __exit intel_aet_exit(void);
>  int intel_aet_read_event(int domid, int rmid, enum resctrl_event_id evtid,
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index f9f3bc58290e..7fe4e8111773 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -895,6 +895,26 @@ bool rdt_is_software_feature_enabled(char *name)
>  	return ret;
>  }
>  
> +/*
> + * Similar to rdt_is_software_feature_enabled() but the test is whether

This is just too similar and makes this code quite confusing ... (more below)

> + * the user has force enabled the feature on the kernel command line.
> + */
> +bool rdt_is_software_feature_force_enabled(char *name)
> +{
> +	struct rdt_options *o;
> +	bool ret = false;
> +
> +	for (o = rdt_options; o < &rdt_options[NUM_RDT_OPTIONS]; o++) {
> +		if (!strcmp(name, o->name)) {
> +			if (o->force_on)
> +				ret = true;
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  bool resctrl_arch_is_evt_configurable(enum resctrl_event_id evt)
>  {
>  	if (!rdt_cpu_has(X86_FEATURE_BMEC))
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index 1d2511984156..1d9edd409883 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -15,6 +15,7 @@
>  #include <linux/cpu.h>
>  #include <linux/intel_vsec.h>
>  #include <linux/io.h>
> +#include <linux/minmax.h>
>  #include <linux/resctrl.h>
>  #include <linux/slab.h>
>  
> @@ -55,6 +56,9 @@ struct pmt_event {
>   *			telemetry regions.
>   * @pkginfo:		Per-package MMIO addresses of telemetry regions belonging to this group.
>   * @guid:		Unique number per XML description file.
> + * @num_rmids:		Number of RMIDS supported by this group. Adjusted downwards
> + *			if enumeration from intel_pmt_get_regions_by_feature() indicates
> + *			fewer RMIDs can be tracked simultaneously.
>   * @mmio_size:		Number of bytes of MMIO registers for this group.
>   * @num_events:		Number of events in this group.
>   * @evts:		Array of event descriptors.
> @@ -67,6 +71,7 @@ struct event_group {
>  
>  	/* Remaining fields initialized from XML file. */
>  	u32				guid;
> +	u32				num_rmids;
>  	size_t				mmio_size;
>  	int				num_events;
>  	struct pmt_event		evts[] __counted_by(num_events);
> @@ -82,6 +87,7 @@ struct event_group {
>  static struct event_group energy_0x26696143 = {
>  	.name		= "energy",
>  	.guid		= 0x26696143,
> +	.num_rmids	= 576,
>  	.mmio_size	= XML_MMIO_SIZE(576, 2, 3),
>  	.num_events	= 2,
>  	.evts				= {
> @@ -97,6 +103,7 @@ static struct event_group energy_0x26696143 = {
>  static struct event_group perf_0x26557651 = {
>  	.name		= "perf",
>  	.guid		= 0x26557651,
> +	.num_rmids	= 576,
>  	.mmio_size	= XML_MMIO_SIZE(576, 7, 3),
>  	.num_events	= 7,
>  	.evts				= {
> @@ -177,6 +184,17 @@ static int configure_events(struct event_group *e, struct pmt_feature_group *p)
>  			return -EINVAL;
>  		}
>  
> +		/*
> +		 * Ignore event group with fewer RMIDs than can be loaded
> +		 * into the IA32_PQR_ASSOC MSR unless the user used
> +		 * the rdt= boot option to specifically ask for it to
> +		 * be enabled.
> +		 */
> +		if (tr->num_rmids < rdt_num_system_rmids &&

This check comes as a surprise after thinking that I understood the changelog
and function comments. I was expecting a check against e->num_rmids instead?


The changelog states:

> +		    !rdt_is_software_feature_force_enabled(e->name))

Having this call here is unexpected. The way resctrl has handled quirks thus far
is to disable a particular feature explicitly based on some external knowledge (eg. errata).
This is different in that resctrl does not hardcode that a feature is disabled but attempts
to determine this by something that is discoverable from hardware self. Integrating the
feature enable/disable into the flow that actually initializes the feature looks
complicated to me and the strange rdt_is_software_feature_force_enabled() supports this.

Could you please consider adding a new function call at beginning of
configure_events()/discover_events() (before calling rdt_is_feature_enabled()) that
peeks into the struct pmt_feature_group to verify if all parameters are "sane" and
then explicitly disables the feature (for example, set_rdt_options("!perf"))
if any parameter is not "sane". This makes it clear what requirements are from
hardware and what is considered a "quirk" and not even attempt to enable it by default.

Following this function with rdt_is_feature_enabled() still enables user to override
such disable.



> +			return -EINVAL;
> +		e->num_rmids = min(e->num_rmids, tr->num_rmids);
> +
>  		if (!pkgcounts) {
>  			pkgcounts = kcalloc(num_pkgs, sizeof(*pkgcounts), GFP_KERNEL);
>  			if (!pkgcounts)
> @@ -263,11 +281,22 @@ static bool get_pmt_feature(enum pmt_feature_id feature)
>   */
>  bool intel_aet_get_events(void)
>  {
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_PERF_PKG].r_resctrl;
> +	struct event_group **eg;
>  	bool ret1, ret2;
>  
>  	ret1 = get_pmt_feature(FEATURE_PER_RMID_ENERGY_TELEM);
>  	ret2 = get_pmt_feature(FEATURE_PER_RMID_PERF_TELEM);
>  
> +	for (eg = &known_event_groups[0]; eg < &known_event_groups[NUM_KNOWN_GROUPS]; eg++) {
> +		if (!(*eg)->pfg)
> +			continue;
> +		if (r->num_rmid)
> +			r->num_rmid = min(r->num_rmid, (*eg)->num_rmids);
> +		else
> +			r->num_rmid = (*eg)->num_rmids;
> +	}
> +
>  	return ret1 || ret2;
>  }
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 51d7d99336c6..b36634f1439b 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -33,6 +33,7 @@ bool rdt_mon_capable;
>  
>  #define CF(cf)	((unsigned long)(1048576 * (cf) + 0.5))
>  
> +int rdt_num_system_rmids;

Is this necessary? If I understand correctly the next patch will ensure that
resctrl will not use fewer RMIDs than what can be loaded into IA32_PQR_ASSOC MSR.

If it is required, considering that this patch goes through effort to change type
of rdt_resource::num_rmid to u32, should this also be u32?

>  static int snc_nodes_per_l3_cache = 1;
>  
>  /*
> @@ -358,6 +359,7 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>  	resctrl_rmid_realloc_limit = boot_cpu_data.x86_cache_size * 1024;
>  	hw_res->mon_scale = boot_cpu_data.x86_cache_occ_scale / snc_nodes_per_l3_cache;
>  	r->num_rmid = (boot_cpu_data.x86_cache_max_rmid + 1) / snc_nodes_per_l3_cache;
> +	rdt_num_system_rmids = r->num_rmid;
>  	hw_res->mbm_width = MBM_CNTR_WIDTH_BASE;
>  
>  	if (mbm_offset > 0 && mbm_offset <= MBM_CNTR_WIDTH_OFFSET_MAX)

Reinette

Powered by blists - more mailing lists