Message-ID: <50149b02-d6ea-4fb2-806a-f16e80d3d52b@intel.com>
Date: Thu, 13 Nov 2025 14:51:45 -0800
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>, Chen Yu <yu.c.chen@...el.com>
CC: <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
	<patches@...ts.linux.dev>
Subject: Re: [PATCH v13 25/32] x86/resctrl: Handle number of RMIDs supported
 by RDT_RESOURCE_PERF_PKG

Hi Tony,

On 10/29/25 9:21 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
> MSR_IA32_PQR_ASSOC. 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.
> 
> 3) The number of "hardware 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 "hardware counters" to track all RMIDs
> are difficult for users to use, since the system may reassign "hardware
> counters" at any time. This means that users cannot reliably collect
> two consecutive event counts to compute the rate at which events are
> occurring.
> 
> Introduce rdt_set_feature_disabled() to mark any under-resourced event groups
> (those with telemetry_region::num_rmids < event_group::num_rmids  for any of

Extra space above, also note how line lengths differ between paragraphs.

> the event group's telemetry regions) as unusable.  Note that the rdt_options[]
> structure must now be writable at run-time.
> 
> Limit an under-resourced event group's number of possible monitor
> resource groups to the lowest number of "hardware counters" if the
> user explicitly requests to enable it.
> 
> 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 to determine how many monitoring resource groups
> are supported.
> 
> N.B. Change type of rdt_resource::num_rmid to u32 to match type of

resctrl_mon::num_rmid

> event_group::num_rmids so that min(r->num_rmid, e->num_rmids) won't
> complain about mixing signed and unsigned types.

May be worthwhile to highlight that resctrl_mon::num_rmid is already
used as a u32 so changing its type does not need to cascade into all
users. Something like "Change type of resctrl_mon::num_rmid to u32 to
match its usage and the type of ..."


> diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> index 781ca8ede39e..252a3fd4260c 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -25,6 +25,7 @@
>  #include <linux/intel_pmt_features.h>
>  #include <linux/intel_vsec.h>
>  #include <linux/io.h>
> +#include <linux/minmax.h>
>  #include <linux/overflow.h>
>  #include <linux/printk.h>
>  #include <linux/rculist.h>
> @@ -57,6 +58,7 @@ struct pmt_event {
>   * struct event_group - All information about a group of telemetry events.
>   * @feature:		Argument to intel_pmt_get_regions_by_feature() to
>   *			discover if this event_group is supported.
> + * @name:		Name for this group (used by boot rdt= option)
>   * @pfg:		Points to the aggregated telemetry space information
>   *			returned by the intel_pmt_get_regions_by_feature()
>   *			call to the INTEL_PMT_TELEMETRY driver that contains
> @@ -64,6 +66,10 @@ struct pmt_event {
>   *			Valid if the system supports the event group.
>   *			NULL otherwise.
>   * @guid:		Unique number per XML description file.
> + * @num_rmids:		Number of RMIDs supported by this group. May be
> + *			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.
> @@ -71,10 +77,12 @@ struct pmt_event {
>  struct event_group {
>  	/* Data fields for additional structures to manage this group. */
>  	enum pmt_feature_id		feature;
> +	char				*name;
>  	struct pmt_feature_group	*pfg;
>  
>  	/* Remaining fields initialized from XML file. */
>  	u32				guid;
> +	u32				num_rmids;

This addition is to resctrl where there is already a resctrl_mon::num_rmid.
Can this naming be made consistent with resctrl instead of INTEL_PMT_TELEMETRY?

>  	size_t				mmio_size;
>  	unsigned int			num_events;
>  	struct pmt_event		evts[] __counted_by(num_events);
> @@ -89,7 +97,9 @@ struct event_group {
>   */
>  static struct event_group energy_0x26696143 = {
>  	.feature	= FEATURE_PER_RMID_ENERGY_TELEM,
> +	.name		= "energy",
>  	.guid		= 0x26696143,
> +	.num_rmids	= 576,
>  	.mmio_size	= XML_MMIO_SIZE(576, 2, 3),
>  	.num_events	= 2,
>  	.evts		= {
> @@ -104,7 +114,9 @@ static struct event_group energy_0x26696143 = {
>   */
>  static struct event_group perf_0x26557651 = {
>  	.feature	= FEATURE_PER_RMID_PERF_TELEM,
> +	.name		= "perf",
>  	.guid		= 0x26557651,
> +	.num_rmids	= 576,
>  	.mmio_size	= XML_MMIO_SIZE(576, 7, 3),
>  	.num_events	= 7,
>  	.evts		= {
> @@ -174,11 +186,54 @@ static bool group_has_usable_regions(struct event_group *e, struct pmt_feature_g
>  	return usable_regions;
>  }
>  
> +static bool all_regions_have_sufficient_rmid(struct event_group *e, struct pmt_feature_group
> +*p)
> +{
> +	struct telemetry_region *tr;
> +	bool ret = true;
> +
> +	for (int i = 0; i < p->count; i++) {
> +		if (!p->regions[i].addr)
> +			continue;
> +		tr = &p->regions[i];
> +		if (tr->num_rmids < e->num_rmids)
> +			ret = false;
> +	}
> +
> +	return ret;
> +}
> +
>  static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
>  {
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_PERF_PKG].r_resctrl;
> +
>  	if (!group_has_usable_regions(e, p))
>  		return false;
>  
> +	/* Disable feature if insufficient RMIDs */
> +	if (!all_regions_have_sufficient_rmid(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 false;

Considering this from the user's perspective I do not think there is an easy way for user space
to know that the feature was force disabled and that there is an option to override this.
What is the use case considered here? If I understand correctly there may be a way to deduce this
by consulting files in /sys/class/intel_pmt/ where the telemetry regions' number of supported
RMID is printed. A user can compare that to the XML files to determine that there is an issue
that can explain resctrl not exposing the feature. Considering this difficulty I wonder if it
may be helpful to print an error message here? This is of course not perfect since the kernel
log cannot be guaranteed to forever contain it ... but it may help?

Reinette

Powered by blists - more mailing lists