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

Luck, Tony
2025-11-18T17:35:40Z

On Tue, Nov 18, 2025 at 08:48:18AM -0800, Reinette Chatre wrote:
> Hi Tony,
>
> On 11/17/25 10:52 AM, Luck, Tony wrote:
> > On Mon, Nov 17, 2025 at 09:31:41AM -0800, Reinette Chatre wrote:
> >> On 11/17/25 8:37 AM, Luck, Tony wrote:
> >>> On Fri, Nov 14, 2025 at 03:26:42PM -0800, Reinette Chatre wrote:
> >>>> On 11/14/25 1:55 PM, Luck, Tony wrote:
> >>>> 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.
>
> My concern is that after skipping the event there is a deceiving message that the event group was
> enabled successfully.

I can change resctrl_enable_mon_event() to return a "bool" to say
whether each event was successfully enabled.

Then change to:

int skipped_events = 0;

for (int j = 0; j < e->num_events; j++) {
if (!resctrl_enable_mon_event(e->evts[j].id, true,
e->evts[j].bin_bits, &e->evts[j]))
skipped_events++;
}

if (e->num_events == skipped_events) {
pr_info("No events enabled in %s %s:0x%x\n", r->name, e->name, e->guid);
return false;
}

if (skipped_events)
pr_info("%s %s:0x%x monitoring detected (skipped %d events)\n", r->name,
r->name, e->name, e->guid, skipped_events);
else
pr_info("%s %s:0x%x monitoring detected\n", r->name, e->name, e->guid);
>
> ...
>
> > ---
> >
> > 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) { };
>
> (nit: stray semicolon)
>
> > #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';
>
> This looks like an open code of strsep()?
>
> > + intel_aet_option(force_off, tok, suboption);
> > + }
>
> I think this can be simplified. It also looks possible to follow some patterns of existing
> option handling.
>
> By adding the force_on/force_off members to struct event_group I do not see the perf and
> energy options needed in rdt_options[] anymore. rdt_set_feature_disabled() and
> rdt_is_feature_enabled() now also seems unnecessary because the event_group::force_on and
> event_group::force_off are sufficient.
>
> It looks to me that the entire token can be passed here to intel_aet_option() and it
> returns 1 to indicate that it was able to "handle" the token, 0 otherwise. If intel_aet_option()
> was able to handle the option then it is not necessary to do further parsing.
> "handle" means that it could successfully initialize the new members of struct event_group.
>
> So instead, how about something like:
> if (intel_aet_option(force_off, tok))
> continue;
>
> > 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.
>
> To be consistent, can also be true if event group was found to have insufficient RMID.
>
> > + * @force_on: Set true when "rdt" command line overrides disable of
> > + * this @guid due to insufficeint @num_rmid.
>
> "Set" can be dropped to be explicit of state instead of potentially confusing "Set" to
> be a verb.
>
> insufficeint -> insufficient
>
> > * @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;
> > +
>
> Can use strsep() here to split provided token into name and guid. Take care to
> check if guid NULL before attempting kstrtou32().
>
> > + if (kstrtou32(suboption, 16, &guid))
> > + return;
> > +
> > + for_each_event_group(peg) {
> > + if (!strcmp(option, (*peg)->name))
>
> !strcmp() -> strcmp()?
>
> > + continue;
> > + if ((*peg)->guid != guid)
> > + continue;
>
> If no guid provided then all event groups with matching name can have
> force_on/force_off member set to support user providing, for example: "!perf" to
> disable all perf event groups.
>
> > + 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;
> >
>
> rdt_set_feature_disabled() that is around here can be replaced with setting
> event_group::force_off
>
> > @@ -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) {
>
> rdt_is_feature_enabled() can be replaced with check of event_group::force_off
>
> > if (warn_disable)
> > pr_info("Feature %s guid=0x%x not enabled due to insufficient RMIDs\n",
> > e->name, e->guid);
> >
> >
>
> I believe changes I mention would simplify the implementation a lot while making it
> more powerful to support the, for example, "!perf" use case. What do you think?

Agreed. Simpler and more flexible. I'll make these changes.
>
> Reinette

-Tony