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

[Reinette Chatre]

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

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