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

[Reinette Chatre <reinette.chatre@intel.com>]

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.

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  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.

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