Re: [PATCH v10 08/17] x86/resctrl: Enforce system RMID limit on AET event groups

[Luck, Tony]

On Thu, Aug 20, 2026 at 03:28:18PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 8/20/26 2:55 PM, Reinette Chatre wrote:
> > Hi Tony,
> > 
> > On 8/20/26 11:42 AM, Luck, Tony wrote:
> >> On Mon, Aug 17, 2026 at 05:58:13PM -0700, Reinette Chatre wrote:
> >>> Hi Tony,
> >>>
> >>> On 7/29/26 10:27 AM, Tony Luck wrote:
> >>>> AET (Application Energy Telemetry) event groups each support a specific
> >>>> number of RMIDs. But that number may be lower than the number supported
> >>>> by the system. Especially true on systems with SNC (Sub-NUMA Cluster)
> >>>> enabled as that reduces the number of supported RMIDs.
> >>>>
> >>>> Fix get_rdt_mon_resources() to return true when any monitor resource is
> >>>
> >>> hmmm ... "Fix" makes one look for the accompanying "Fixes:" tag. What is
> >>> the fix here? What is wrong with existing implementation that needs fixing?
> >>> To me this does not look like a fix though (more below).
> >>>
> >>>> possibly enabled. Call intel_aet_init() to adjust the event_group::num_rmid
> >>>> values to not exceed the system supported maximum.
> >>>
> >>> Last sentence just documents the code. Please describe why this is needed.
> >>> Is this a separate logical change?
> >>>
> >>>>
> >>>> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> >>>> ---
> >>>
> >>> ...
> >>>
> >>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> >>>> index 092764cf693f..2c938b97b147 100644
> >>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
> >>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> >>>> @@ -1019,10 +1019,10 @@ static __init bool get_rdt_mon_resources(void)
> >>>>  	if (rdt_cpu_has(X86_FEATURE_ABMC))
> >>>>  		ret = true;
> >>>>  
> >>>> -	if (!ret)
> >>>> -		return false;
> >>>> +	if (ret)
> >>>> +		rdt_get_l3_mon_config(r);
> >>>>  
> >>>> -	return !rdt_get_l3_mon_config(r);
> >>>> +	return boot_cpu_data.x86_cache_max_rmid > 0;
> >>>>  }
> >>>
> >>> >From what I can tell this will return true when the system supports monitoring,
> >>> but no resource may actually have monitoring enabled at this point. Specifically,
> >>> no resource has rdt_resource::mon_capable set. 
> >>>
> >>> The resctrl initialization now proceeds where it used to stop. resctrl_arch_late_init()
> >>> will proceed and initialize the resctrl filesystem, which in turn would allow user space
> >>> to mount it.
> >>>
> >>> rdt_get_tree() handling the user mount request could thus be run on a system that does
> >>> not have a monitoring or allocation capable resource and then we see in rdt_get_tree():
> >>> 	if (resctrl_arch_alloc_capable() || resctrl_arch_mon_capable())
> >>> 		resctrl_mounted = true;
> >>
> >> Should the inverse of that check really be an error condition and result
> >> in failing the mount? There seems no point in a mount with no monitor or
> >> alloc features.
> > 
> > I agree that there is no point in a mount with no monitor or alloc features.
> > That is indeed how resctrl behaved until this misrepresentation of a fix changed
> > this behavior.
> > 
> >>
> >> This code appeared as part of James' separating the x86 specific static
> >> branch code out of the filesystem generic mount path in commit
> >> 13e5769debf0 ("x86/resctrl: Make resctrl_mounted checks explicit")
> > 
> > This change would have had same consequence even without that patch, no? Only
> > difference is that before the patch you point out rdt_enable_key was used to
> > check if resctrl fs was mounted and rdt_enable_key would not be set if there
> > are no monitor or alloc features resulting in the same problem of resctrl
> > allowing a remount after it is already mounted.
> > 
> >>
> >> My plan is to move this inverted check earlier (right after the call
> >> to resctrl_arch_pre_mount() which could be the decision point on if
> >> any monitor resources are enabled.
> >>
> >>
> >> rdt_get_tree()
> >> {
> >> 	mutex_lock(&resctrl_mount_lock); // NEW (revived from v4 of series)
> >>
> >> 	check for nested mount -> -EBUSY
> >>
> >> 	resctrl_arch_pre_mount();
> >>
> >> 	if (!resctrl_arch_alloc_capable() && !resctrl_arch_mon_capable()) {
> > 
> > A test like this looks to be needed after this patch, yes. Even so, why does this
> > need to be outside of the main function protected by rdtgroup_mutex? I do not think
> > that it is ideal to have this information accessed by resctrl fs with different mutex
> > inconsistently held. Or does this actually reflect the larger "always return the same
> > data from mount to unmount" implicit contract with architecture?

It doesn't need to be outside the cpus_read_lock() and mutex_lock(&rdtgroup_mutex);

I agree that it would be much cleaner covered by those locks.

> > 
> > There clearly needs to be a contract between fs and arch on what the fs expects these
> > two functions to return. Patch 6/17 is beginning to create this contract. 
> 
> Thinking about this more ... does resctrl really need these to be arch helpers? With
> the mon_capable/alloc_capable available, resctrl fs does not need any help from architecture
> to learn whether a resource is monitor or alloc capable, no?

x86 and mpam versions of these currently use different iterator functions to walk over all
resources. But there is a file system version in <linux/resctrl.h> so we
could do

static inline bool resctrl_alloc_capable(void)
{
	struct rdt_resource *r;

	for_each_rdt_resource(r)
		if ((r)->alloc_capable)
			return true;
	return false;
}

Ditto with s/alloc/mon/ for the matching partner,

Would you like a patch to replace the arch versions woven into this series?

> > 
> >> 		ret = -EINVAL;
> >> 		goto out_mount_unlock;
> >> 	}
> >>
> >> 	...
> >>
> >> 	resctrl_mounted = true;
> >>
> >> Choice of error code is still open. I tried -ENODEV, but that results in
> >> an error message to the user saying the resctrl filesystem is not
> >> supported.
> >>
> >>>
> >>> The above flow change would cause resctrl to think the system supports monitoring
> >>> but resctrl_arch_mon_capable() returns false. If there are no allocation features
> >>> then this will result in resctrl fs mounted ... but resctrl_mounted is not set to
> >>> true and thus allow a remount that is not supported.
> > Reinette
> 
> Reinette
> 

-Tony