Message-ID: <aRyuY_TF5d6Me7MD@agluck-desk3>
Date: Tue, 18 Nov 2025 09:35:31 -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 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

Powered by blists - more mailing lists