Message-ID: <aRtu0J4os4bXTLV2@agluck-desk3>
Date: Mon, 17 Nov 2025 10:52:00 -0800
From: "Luck, Tony" <tony.luck@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
CC: 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>, <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

On Mon, Nov 17, 2025 at 09:31:41AM -0800, Reinette Chatre wrote:
> Hi Tony,
> 
> On 11/17/25 8:37 AM, Luck, Tony wrote:
> > On Fri, Nov 14, 2025 at 03:26:42PM -0800, Reinette Chatre wrote:
> >> Hi Tony,
> >>
> >> On 11/14/25 1:55 PM, Luck, Tony wrote:
> >>>
> >>> resctrl: Feature energy guid=0x26696143 not enabled due to insufficient RMIDs
> >>>
> >>>
> >>> 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;
> >>> 	bool warn_disable = false;
> >>>
> >>> 	if (!group_has_usable_regions(e, p))
> >>> 		return false;
> >>>
> >>> 	/* Disable feature if insufficient RMIDs */
> >>> 	if (!all_regions_have_sufficient_rmid(e, p)) {
> >>> 		warn_disable = true;
> >>> 		rdt_set_feature_disabled(e->name);
> >>> 	}
> >>>
> >>> 	/* User can override above disable from kernel command line */
> >>> 	if (!rdt_is_feature_enabled(e->name)) {
> >>> 		if (warn_disable)
> >>> 			pr_info("Feature %s guid=0x%x not enabled due to insufficient RMIDs\n",
> >>> 				e->name, e->guid);
> >>> 		return false;
> >>> 	}
> >>> 	...
> >>> }
> >>
> >> Thank you for considering. This looks good to me.
> >>
> >> I now realize that if a system supports, for example, two energy guid and only one has insufficient
> >> RMID then one or both may be disabled by default depending on which resctrl attempts to enable
> >> first. This is arbitrary based on where the event group appears in the array.
> > 
> > intel_pmt_get_regions_by_feature() does return arrays of telemetry_region
> > with different guids today, but not currently for the "RMID" features.
> > So this could be a problem in the future.
> > 
> > I think I need to drop the "rdt=perf,!energy" command line control as
> > being too coarse. Instead add a new boot argument. E.g.
> > 
> > 	rdtguid=0x26696143,!0x26557651
> > 
> > to give the user control per-guid instead of per-pmt_feature_id. Users
> > can discover which guids are supported on a system by looking in
> > /sys/bus/auxiliary/devices/intel_vsec.discovery.*/intel_pmt/features*/per_rmid*
> > where there are "guids" and "num_rmids" files.
> 
> Should disable/enable be per RMID telemetry feature? I do not see anything preventing a system from
> using the same guid for different RMID telemetry features.
> 
> I think it will be useful to look at how other kernel parameters distinguish different
> categories of parameters so that resctrl can be consistent here. Looks like an underscore is
> most useful and also flexible since it allows both a dash and underscore to be used.
> 
> Another alternative that is common in kernel parameters is to use ":". For example, 
> 	rdt=energy:0x26696143
> 
> With something like above user can, for example, use just "energy" to disable all RMID energy
> telemetry or be specific to which guid should be disabled. This seems to fit well with existing
> rdt parameters and be quite flexible.

See rough patch at foot of this e-mail. It's just on top of my WIP v14,
but if it looks like the right direction I will merge it into the series
in the patch that adds the energy/perf options.

> > 
> >> How a system with two guid of the same feature type would work is not clear to me though. Looks
> >> like they cannot share events at all since an event is uniquely associated with a struct pmt_event
> >> that can belong to only one event group. If they may share events then enable_events()->resctrl_enable_mon_event()
> >> will complain loudly but still proceed and allow the event group to be enabled.
> > 
> > I can't see a good reason why the same event would be enabled under
> > different guids present on the same system. We can revisit my assumption
> > if the "Duplicate enable for event" message shows up.
> 
> This would be difficult to handle at that time, no? From what I can tell this would enable
> an unusable event group to actually be enabled resulting in untested and invalid flows.
> I think it will be safer to not enable an event group in this scenario and seems to math your
> expectation that this would be unexpected. The "Duplicate enable for event" message will still
> appear and we can still revisit those assumptions when they do, but the systems encountering
> them will not be running with enabled event groups that are not actually fully enabled.

There's a hardware cost to including an event in an aggregator.
Inclusing the same event in mutliple aggregators described by
different guids is really something that should never happen.
Just printing a warning and skipping the event seems an adequate
defense.
> 
> > 
> >> I think the resctrl_enable_mon_event() warnings were added to support enabling of new features
> >> so that the WARNs can catch issues during development ... now it may encounter issues when a
> >> kernel with this implementation is run on a system that supports a single feature with
> >> multiple guid. Do you have more insight in how the "single feature with multiple guid" may look to
> >> better prepare resctrl to handle them?
> >>
> >> Should "enable_events" be split so that a feature can be disabled for all its event groups if
> >> any of them cannot be enabled due to insufficient RMIDs?
> >> Perhaps resctrl_enable_mon_event() should also now return success/fail so that an event group
> >> cannot be enabled if its events cannot be enabled?
> >> Finally, a system with two guid of the same feature type will end up printing duplicate
> >> "<feature type> monitoring detected" that could be more descriptive?
> > 
> > I need to add the guid to that message.
> 
> Sounds good. Thank you.
> 
> Reinette

-Tony

---

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 08eb78acb988..25df1abc1537 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -241,6 +241,7 @@ int intel_aet_read_event(int domid, u32 rmid, void *arch_priv, u64 *val);
 void intel_aet_mon_domain_setup(int cpu, int id, struct rdt_resource *r,
 				struct list_head *add_pos);
 void intel_aet_add_debugfs(void);
+void intel_aet_option(bool force_off, const char *option, const char *suboption);
 #else
 static inline bool intel_aet_get_events(void) { return false; }
 static inline void __exit intel_aet_exit(void) { }
@@ -252,6 +253,7 @@ static inline int intel_aet_read_event(int domid, u32 rmid, void *arch_priv, u64
 static inline void intel_aet_mon_domain_setup(int cpu, int id, struct rdt_resource *r,
 					      struct list_head *add_pos) { }
 static inline void intel_aet_add_debugfs(void) { }
+static inline void intel_aet_option(bool force_off, const char *option, const char *suboption) { };
 #endif
 
 #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 5cae4119686e..68195f458c0b 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -842,6 +842,7 @@ static struct rdt_options rdt_options[] = {
 static int __init set_rdt_options(char *str)
 {
 	struct rdt_options *o;
+	char *suboption;
 	bool force_off;
 	char *tok;
 
@@ -851,6 +852,11 @@ static int __init set_rdt_options(char *str)
 		force_off = *tok == '!';
 		if (force_off)
 			tok++;
+		suboption = strpbrk(tok, ":");
+		if (suboption) {
+			*suboption++ = '\0';
+			intel_aet_option(force_off, tok, suboption);
+		}
 		for (o = rdt_options; o < &rdt_options[NUM_RDT_OPTIONS]; o++) {
 			if (strcmp(tok, o->name) == 0) {
 				if (force_off)
diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
index 6028bfec229b..b3c61bcd3e8f 100644
--- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
+++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
@@ -69,6 +69,9 @@ struct pmt_event {
  *			data for all telemetry regions type @feature.
  *			Valid if the system supports the event group.
  *			NULL otherwise.
+ * @force_off:		Set true when "rdt" command line disables this @guid.
+ * @force_on:		Set true when "rdt" command line overrides disable of
+ *			this @guid due to insufficeint @num_rmid.
  * @guid:		Unique number per XML description file.
  * @num_rmid:		Number of RMIDs supported by this group. May be
  *			adjusted downwards if enumeration from
@@ -83,6 +86,7 @@ struct event_group {
 	enum pmt_feature_id		feature;
 	char				*name;
 	struct pmt_feature_group	*pfg;
+	bool				force_off, force_on;
 
 	/* Remaining fields initialized from XML file. */
 	u32				guid;
@@ -144,6 +148,26 @@ static struct event_group *known_event_groups[] = {
 	     _peg < &known_event_groups[ARRAY_SIZE(known_event_groups)];	\
 	     _peg++)
 
+void intel_aet_option(bool force_off, const char *option, const char *suboption)
+{
+	struct event_group **peg;
+	u32 guid;
+
+	if (kstrtou32(suboption, 16, &guid))
+		return;
+
+	for_each_event_group(peg) {
+		if (!strcmp(option, (*peg)->name))
+			continue;
+		if ((*peg)->guid != guid)
+			continue;
+		if (force_off)
+			(*peg)->force_off = true;
+		else
+			(*peg)->force_on = true;
+	}
+}
+
 /*
  * Clear the address field of regions that did not pass the checks in
  * skip_telem_region() so they will not be used by intel_aet_read_event().
@@ -252,6 +276,9 @@ 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;
 	bool warn_disable = false;
 
+	if (e->force_off)
+		return false;
+
 	if (!group_has_usable_regions(e, p))
 		return false;
 
@@ -262,7 +289,7 @@ static bool enable_events(struct event_group *e, struct pmt_feature_group *p)
 	}
 
 	/* User can override above disable from kernel command line */
-	if (!rdt_is_feature_enabled(e->name)) {
+	if (!rdt_is_feature_enabled(e->name) && !e->force_on) {
 		if (warn_disable)
 			pr_info("Feature %s guid=0x%x not enabled due to insufficient RMIDs\n",
 				e->name, e->guid);

Powered by blists - more mailing lists