Re: [PATCH v13 25/32] x86/resctrl: Handle number of RMIDs supported by RDT_RESOURCE_PERF_PKG

Luck, Tony
2025-11-17T18:52:06Z

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);