Re: [PATCH v9 06/12] arm,x86,fs/resctrl: Handle change in number of RMIDs on each mount

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

Hi Tony,

On 7/14/26 9:01 AM, Luck, Tony wrote:
> On Tue, Jul 14, 2026 at 08:18:30AM -0700, Reinette Chatre wrote:
>> On 7/10/26 1:51 PM, Luck, Tony wrote:
>>> On Wed, Jul 08, 2026 at 03:50:45PM -0700, Reinette Chatre wrote:
>>>> Hi Tony,
>>>>
>>>> On 7/1/26 2:35 PM, Tony Luck wrote:
>>>>> Application Energy Telemetry (AET) event enumeration takes place
>>>>> asynchronously. Linux builds the pmt_telemetry module into the kernel to
>>>>> kick off enumeration early enough that it completes before first mount of
>>>>> the resctrl file system.
>>>>>
>>>>> Allowing pmt_telemetry to be a loadable module means that it is possible
>>>>> for different numbers of RMIDs to be supported on each mount, depending
>>>>> on whether pmt_telemetry module is loaded.
>>>>>
>>>>> For simplicity, calculate the maximum possible number of RMIDs and use
>>>>> that value to allocate the rmid_ptrs[] array just once. Also use this
>>>>> maximum RMID value when allocating rdt_l3_mon_domain::rmid_busy_llc
>>>>> bitmap and rdt_l3_mon_domain::mbm_states.
>>>>
>>>> This does not sound right. Why use the maximum RMID for L3 monitoring state?
>>>> It can be guaranteed that L3 monitoring state is only accessed when that
>>>> monitoring is enabled and when it is enabled it can be guaranteed to never
>>>> use more RMID than what L3 itself supports. Why would it ever be required
>>>> to allocate more than that? Could this not instead be limited to
>>>> rdt_resource::resctrl_mon::num_rmid? From what I can tell such transition
>>>> will make it explicit and consistent (since arch code already allocates
>>>> this state based on this) how the L3 monitoring state is sized.
>>>
>>> L3 monitoring on X86 happens to provide the overall maximum possible RMID
>>> value (any larger value with #GP fault when written to IA32_PQR_ASSOC MSR).
>>>
>>> AET could potentially claim to support a larger number (and this could
>>> happen on low core count SKUs where the number of RMIDs supported scales
>>> down with the number of cores).
>>>
>>> resctrl_arch_system_max_rmid_idx() needs better comments, and
>>> implementation. On x86 even if the L3 monitor functions have been
>>> disabled, the limit is still bounded by CPUID. Not by what might
>>> be found in the AET->mon.num_rmid.
>>
>> I see. I assume the implementation change you refer to involves dropping
>> usage of r->mon_capable that depends on rdt_cpu_has().
>>
>> How much to rely on CPUID is not clear to me. The direction seems to
>> be to move away from CPUID, which makes adding new CPUID dependencies
>> less ideal? 
> 
> Here's my current work-in-progress version:
> 
> u32 resctrl_arch_system_max_rmid_idx(void)
> {
> 	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> 	u32 ret;
> 
> 	/* CPUID provides maximum possible RMID value */
> 	ret = cpuid_ebx(0xf) + 1;

There is also boot_cpu_data.x86_cache_max_rmid initialized in resctrl_cpu_detect() that
could be used directly? Looks like resctrl_cpu_detect() already scales the number of
RMID down if needed when L3 monitoring is supported, but it does not take SNC into account.


> 
> 	/*
> 	 * if system is capable of L3 monitoring the maximum RMID value may
> 	 * be lower that system maximum. Either because the L3 monitoring
> 	 * feature supports fewer RMIDs (CPUID(0xF, 0x1).ECX), or because SNC
> 	 * (Sub-NUMA Cluster) is enabled and divides RMIDs per cluster.
> 	 */
> 	if (r->mon_capable)
> 		ret = r->mon.num_rmid;
> 
> 	return ret;
> }
> 
> CPUID seems unavoidable in the case that the platform doesn't support
> (or has disabled the various L3 monitoring events). In that case using

Since L3 monitoring is the only CPUID supported monitoring resource I do not
think cpuid_ebx(0xf) can be used if the platform does not support L3 monitoring.
I expect that would mean that leaf 0x7 would indicate that monitoring is
not supported which would make leaf 0xf invalid? Interestingly resctrl does
not seem to consider X86_FEATURE_CQM from leaf 0x7 at all and just goes straight
to leaf 0xf.

> cpuid_ebx(0xf) gets the max value that can be written to the RMID field
> of IA32_PQR_ASSOC. This is common to Intel and AMD.
> 
> If RDT_RESOURCE_L3 monitoring is enabled, then it could impose a smaller
> value from CPUID(0xF, 0x1).ECX. SNC might also divide down the RMID
> space per node. That calculation has already been done, so just re-using
> the result from r->mon.num_rmid avoids mistakes.
> 
> Even if AET supports a larger value of num_rmid, the system is limited
> to what can be written to IA32_PQR_ASSOC, so no need to scan all
> resources.
> 
> 
>>> Though perhaps intel_aet.c should look at CPUID to avoid believing any
>>> larger numbers seen in AET enumeration.
>>>

...

 
>>>>> @@ -970,27 +984,29 @@ int setup_rmid_lru_list(void)
>>>>>  		return 0;
>>>>>  
>>>>>  	/*
>>>>> -	 * Called on every mount, but the number of RMIDs cannot change
>>>>> -	 * after the first mount, so keep using the same set of rmid_ptrs[]
>>>>> -	 * until resctrl_exit(). Note that the limbo handler continues to
>>>>> -	 * access rmid_ptrs[] after resctrl is unmounted.
>>>>> +	 * Allocate the largest number of RMIDs that this system will ever
>>>>> +	 * need. These cannot be freed until resctrl_exit() because the limbo
>>>>> +	 * handler continues to access rmid_ptrs[] after resctrl is unmounted.
>>>>>  	 */
>>>>> -	if (rmid_ptrs)
>>>>> -		return 0;
>>>>> -
>>>>> -	idx_limit = resctrl_arch_system_num_rmid_idx();
>>>>> -	rmid_ptrs = kzalloc_objs(struct rmid_entry, idx_limit);
>>>>> -	if (!rmid_ptrs)
>>>>> -		return -ENOMEM;
>>>>> +	if (!rmid_ptrs) {
>>>>> +		max_idx_limit = resctrl_arch_system_max_rmid_idx();
>>>>> +		rmid_ptrs = kzalloc_objs(struct rmid_entry, max_idx_limit);
>>>>> +		if (!rmid_ptrs)
>>>>> +			return -ENOMEM;
>>>>>  
>>>>> -	for (i = 0; i < idx_limit; i++) {
>>>>> -		entry = &rmid_ptrs[i];
>>>>> -		INIT_LIST_HEAD(&entry->list);
>>>>> +		for (i = 0; i < max_idx_limit; i++) {
>>>>> +			entry = &rmid_ptrs[i];
>>>>> +			INIT_LIST_HEAD(&entry->list);
>>>>>  
>>>>> -		resctrl_arch_rmid_idx_decode(i, &entry->closid, &entry->rmid);
>>>>> -		list_add_tail(&entry->list, &rmid_free_lru);
>>>>> +			resctrl_arch_rmid_idx_decode(i, &entry->closid, &entry->rmid);
>>>>> +		}
>>>>>  	}
>>>>>  
>>>>> +	/* Find how many RMIDs are needed for this mount */
>>>>> +	min_idx_limit = resctrl_arch_system_num_rmid_idx();
>>>>
>>>> Since mon_capable resources can now come and go from mount to mount there are scenarios where
>>>> rmid_ptrs[] may be smaller than min_idx_limit so above needs extra checks to protect against
>>>> overrun below.
>>>
>>> I don't intend for that to happen. The initial allocation of rmid_ptrs[]
>>> is supposed to be large enough that no subsequent mount would need more.
>>>
>>> I.e. resctrl_arch_system_max_rmid_idx() must return a value >= than
>>> any subsequent resctrl_arch_system_num_rmid_idx() call.
>>>
>>> Perhaps I should add this to resctrl_arch_system_num_rmid_idx()?
>>>
>>> 	num_rmids = min(num_rmids, resctrl_arch_system_max_rmid_idx());
>>
>> While trusting the arch helpers to "do the right thing" occurs frequently in resctrl
>> as a result of its evolution I do not think this is something that it should keep doing.
>> I think it will be more robust for resctrl fs to protect itself here. For example, could
>> resctrl fs obtain a new partner variable to rmid_ptrs[] that reflects how many elements it has?
>> This could be done in addition to changes to arch helpers that will then be required to
>> meet resctrl fs expectations.
> 
> This is a good idea.  I can switch "max_idx_limit" from being a local
> variable in setup_rmid_lru_list() to being global (static). Initialize
> it exactly once from resctrl_arch_system_max_rmid_idx() here, and remove
> all the other local copies and initializations.
> 
> setup_rmid_lru_list() can then defend against bad resctrl_arch_system_num_rmid_idx()
> return values with:
> 
> 	/* Find how many RMIDs are needed for this mount */
> 	min_idx_limit = resctrl_arch_system_num_rmid_idx();
> 	if (min_idx_limit > max_idx_limit) {
> 		pr_warn_once("Attempt to use too many RMIDs\n");
> 		min_idx_limit = max_idx_limit;
> 	}
> 
> Does that seem reasonable?
It does, yes, thank you.

Reinette