Re: [PATCH v7 25/31] x86/resctrl: Handle number of RMIDs supported by telemetry resources

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 7/11/25 4:53 PM, 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

"h/w" -> hardware (throughout please)

> 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"
> at any time. This means that users cannot reliably collect two consecutive
> event counts to compute the rate at which events are occurring.

Based on definitions in (1), (2), (3) I interpret above paragraph to mean that
event groups with insufficient hardware counters, thus when 
"telemetry_region::num_rmids < event_group::num_rmids", are hard to use. 

> Add a variable rdt_num_system_rmids which holds the number of RMIDs
> supported by the system (including adjustments if Sub-NUMA Cluster
> mode is enabled).

I asked in v6 why rdt_num_system_rmids is necessary but that was not answered
so here we are again. It still is not clear how this fits in.

> 
> Use rdt_set_feature_disabled() to mark such under-resourced event groups
> as unusable.  Note that the rdt_options[] structure must now be writable

So an "under resourced event group" is one that does not have sufficient
hardware counters, aka telemetry_region::num_rmids, right?

> at run-time.  The request to disable will be overridden if the user
> explicitly requests to enable 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 as this
> value will be used later to compare against the number of RMIDs
> supported by other resources.

... and that "later" is the spot where the max RMID that can be loaded into
IA32_PQR_ASSOC will be taken into account.

> 
> N.B. Changed type of rdt_resource::num_rmid to u32 to match, and
> print as unsigned value in rdt_num_rmids_show().

Only if rdt_num_system_rmids is really needed but even then  
resctrl_arch_system_num_rmid_idx() already exists and returns a u32.


> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  include/linux/resctrl.h                 |  2 +-
>  arch/x86/kernel/cpu/resctrl/internal.h  |  4 +++
>  arch/x86/kernel/cpu/resctrl/core.c      | 18 ++++++++++++-
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 36 +++++++++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/monitor.c   |  2 ++
>  fs/resctrl/rdtgroup.c                   |  2 +-
>  6 files changed, 61 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index da76e9c37b69..74cd2979549b 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 83166dd0b9c8..a6c41068dc2f 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 u32 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_feature_enabled(char *option);
>  
> +void rdt_set_feature_disabled(char *name);
> +
>  #ifdef CONFIG_X86_RESCTRL_CPU_INTEL_AET
>  bool intel_aet_get_events(void);
>  void __exit intel_aet_exit(void);
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 83e046313600..31fb598482bf 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -807,7 +807,7 @@ struct rdt_options {
>  	bool	force_off, force_on;
>  };
>  
> -static struct rdt_options rdt_options[]  __ro_after_init = {
> +static struct rdt_options rdt_options[] = {
>  	RDT_OPT(RDT_FLAG_CMT,	    "cmt",	X86_FEATURE_CQM_OCCUP_LLC),
>  	RDT_OPT(RDT_FLAG_MBM_TOTAL, "mbmtotal", X86_FEATURE_CQM_MBM_TOTAL),
>  	RDT_OPT(RDT_FLAG_MBM_LOCAL, "mbmlocal", X86_FEATURE_CQM_MBM_LOCAL),
> @@ -869,6 +869,22 @@ bool rdt_cpu_has(int flag)
>  	return ret;
>  }
>  
> +/*
> + * Can be called during feature enumeration if sanity check of
> + * a features parameters indicates problems with the feature.

"features" -> "feature's"

> + */
> +void rdt_set_feature_disabled(char *name)
> +{
> +	struct rdt_options *o;
> +
> +	for (o = rdt_options; o < &rdt_options[NUM_RDT_OPTIONS]; o++) {
> +		if (!strcmp(name, o->name)) {
> +			o->force_off = true;
> +			return;
> +		}
> +	}
> +}
> +
>  /*
>   * Hardware features that do not have X86_FEATURE_* bits.
>   * There is no "hardware does not support this at all" case.
> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index e64a4630e95c..6958efbf7e81 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>
>  
> @@ -56,6 +57,9 @@ struct pmt_event {
>   * @list:		List of active event groups.
>   * @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.

ok ... still matches changelog by confirming "downward adjustment" is based on telemetry
region data ...

>   * @mmio_size:		Number of bytes of MMIO registers for this group.
>   * @num_events:		Number of events in this group.
>   * @evts:		Array of event descriptors.
> @@ -69,6 +73,7 @@ struct event_group {
>  
>  	/* Remaining fields initialized from XML file. */
>  	u32				guid;
> +	u32				num_rmids;
>  	size_t				mmio_size;
>  	unsigned int			num_events;
>  	struct pmt_event		evts[] __counted_by(num_events);
> @@ -86,6 +91,7 @@ static LIST_HEAD(active_event_groups);
>  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				= {
> @@ -101,6 +107,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				= {
> @@ -143,6 +150,22 @@ static bool skip_this_region(struct telemetry_region *tr, struct event_group *e)
>  	return false;
>  }
>  
> +static bool check_rmid_count(struct event_group *e, struct pmt_feature_group *p)
> +{
> +	struct telemetry_region *tr;
> +
> +	for (int i = 0; i < p->count; i++) {
> +		tr = &p->regions[i];
> +		if (skip_this_region(tr, e))
> +			continue;
> +
> +		if (tr->num_rmids < rdt_num_system_rmids)

This is the unexpected check that seems to contradict everything described thus far.
Should this not be "tr->num_rmids < e->num_rmids"?

> +			return false;
> +	}
> +
> +	return true;
> +}
> +
>  static void free_pkg_mmio_info(struct pkg_mmio_info **mmi)
>  {
>  	int num_pkgs = topology_max_packages();
> @@ -165,12 +188,18 @@ DEFINE_FREE(pkg_mmio_info, struct pkg_mmio_info **, free_pkg_mmio_info(_T))
>   */
>  static int discover_events(struct event_group *e, struct pmt_feature_group *p)
>  {
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_PERF_PKG].r_resctrl;
>  	struct pkg_mmio_info **pkginfo __free(pkg_mmio_info) = NULL;
>  	int *pkgcounts __free(kfree) = NULL;
>  	struct telemetry_region *tr;
>  	struct pkg_mmio_info *mmi;
>  	int num_pkgs;
>  
> +	/* Potentially disable feature if insufficient RMIDs */
> +	if (!check_rmid_count(e, p))
> +		rdt_set_feature_disabled(e->name);
> +
> +	/* User can override above disable from kernel command line */
>  	if (!rdt_is_feature_enabled(e->name))
>  		return -EINVAL;
>  
> @@ -182,6 +211,8 @@ static int discover_events(struct event_group *e, struct pmt_feature_group *p)
>  		if (skip_this_region(tr, e))
>  			continue;
>  
> +		e->num_rmids = min(e->num_rmids, tr->num_rmids);

This seems to confirm that the check_rmid_count() check should be against e->num_rmids.

> +
>  		if (!pkgcounts) {
>  			pkgcounts = kcalloc(num_pkgs, sizeof(*pkgcounts), GFP_KERNEL);
>  			if (!pkgcounts)
> @@ -228,6 +259,11 @@ static int discover_events(struct event_group *e, struct pmt_feature_group *p)
>  		resctrl_enable_mon_event(eventid, true, e->evts[i].bin_bits, &e->evts[i]);
>  	}
>  

Would this be "step 4" of discover_events()? If you started a "let's keep track of the
steps" then the expectation is that all steps will be tracked.

> +	if (r->num_rmid)
> +		r->num_rmid = min(r->num_rmid, e->num_rmids);
> +	else
> +		r->num_rmid = e->num_rmids;
> +
>  	return 0;
>  }
>  

Reinette