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

Reinette Chatre
2025-11-18T16:48:25Z

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.

...

> ---
>
> 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?

Reinette