Re: [PATCH v9 03/12] x86/resctrl: Honor rdt=perf option to force enable AET perf events

[Luck, Tony]

On Wed, Jul 08, 2026 at 03:46:45PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 7/1/26 2:35 PM, Tony Luck wrote:
> > The kernel command line option "rdt=" is used to force enable or disable
> > individual resctrl features.
> > 
> > Linux only enumerates AET (Application Energy Telemetry) once on first
> > mount. But that is going to change to enumerate on each mount to allow the
> > pmt_telemetry driver to be configured as a module, and allow unloading when
> > not in use.
> > 
> > The following scenario will be a problem:
> > 
> > 1) User mounts the resctrl file system.  all_regions_have_sufficient_rmid()
> > notes that a perf telemetry region supports fewer RMIDs than expected for
> > the event_group and sets e->force_off = true. But the user had specified
> > "rdt=perf" on the kernel command line so perf events are enabled for
> > this first mount.
> > 
> > 2) Resctrl file system is unmounted and later remounted.
> > 
> > 3) During this new mount cycle enable_events() sees that e->force_off is
> > set and stops enumeration for this event group. So on this second, and
> > all subsequent, mounts perf events are not enabled.
> > 
> > Fix by checking the state of e->force_on at the start of enable_events().
> 
> Above just writes in words what the patch does. Please reduce repeating code
> segments in the changelog and replace with higher level descriptions to help understand
> "why". 
> I think a more detailed context will help with this. Consider, for example,
> something like below (a suggestion to start, please do not copy&paste):
> 	resctrl can disable ("force_off") a feature supported by the platform if,       
> 	for example, there is an erratum related to the feature or, in the case of      
> 	AET (Application Energy Telemetry), when the event group has insufficient       
> 	RMIDs. The kernel command line option "rdt=" can be used to override            
> 	("force_on") when a feature is disabled in such case and also let the user      
> 	disable ("force_off") individual supported resctrl features if they are not     
> 	needed.                                
> 
> 	Linux enumerates AET once on first mount and skips this enumeration if the
> 	associated event group is supported but disabled. Since AET is only enumerated
> 	once, this check is guaranteed to only consider disabling via the rdt= kernel
> 	parameter. However, when transitioning to enumerate AET on every mount, it is
> 	no longer correct to skip enumeration simply because an event group is marked
> 	as disabled. It is then possible for resctrl to have force-disabled the event
> 	group during an earlier enumeration due to insufficient RMIDs, which would
> 	improperly cause the kernel to ignore a user's explicit rdt= override on
> 	subsequent mounts.
> 
> 	Ensure the user's command line choices take precedence over system-level limits.

This looks good. I'll dig through it to see if I can improve any parts.

> > 
> > Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> > ---
> >  arch/x86/kernel/cpu/resctrl/intel_aet.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> > index c22c3cf5167d..cd8257c58f84 100644
> > --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> > +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> > @@ -60,8 +60,8 @@ struct pmt_event {
> >   *			data for all telemetry regions of type @pfname.
> >   *			Valid if the system supports the event group,
> >   *			NULL otherwise.
> > - * @force_off:		True when "rdt" command line or architecture code disables
> > - *			this event group due to insufficient RMIDs.
> > + * @force_off:		True when "rdt" command line disables this event group
> > + *			to avoid system limitations due to insufficient RMIDs.
> >   * @force_on:		True when "rdt" command line overrides disable of this
> >   *			event group.
> >   * @guid:		Unique number per XML description file.
> 
> This is unexpected. The original comment is more accurate and I understood from 
> https://lore.kernel.org/lkml/ainCnu4xmHbgh91r@agluck-desk3/ that you were not going to
> make changes here.

Oops. I'll drop this for next time.

> 
> > @@ -228,7 +228,7 @@ 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;
> >  	int skipped_events = 0;
> >  
> > -	if (e->force_off)
> > +	if (e->force_off && !e->force_on)
> >  		return false;
> >  
> >  	if (!group_has_usable_regions(e, p))
> 
> Reinette

-Tony