Message-ID: <f3ec7e8b-b042-4d38-823f-4f4174cb9d09@intel.com>
Date: Tue, 18 Nov 2025 08:48:18 -0800
From: Reinette Chatre <reinette.chatre@...el.com>
To: "Luck, Tony" <tony.luck@...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

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

Powered by blists - more mailing lists