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

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

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@intel.com>
>>> ---
>>
>> ...
>>
>>> 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?

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. 

> 		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