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

[Reinette Chatre]

Hi Tony,

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? 
> Though perhaps intel_aet.c should look at CPUID to avoid believing any
> larger numbers seen in AET enumeration.
> 
>>>
>>> The limbo code must deal with changes in the number of RMIDs from one
>>> mount to the next because some RMIDs may still be "busy" when the file
>>> system is unmounted, but be above resctrl_arch_system_num_rmid_idx()
>>> for the remount. In this case RMIDs that can be released are not put
>>> onto the rmid_free_lru list.
>>>
>>> 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 60d50ac79e7b..3169441a2d40 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>> @@ -144,6 +144,20 @@ u32 resctrl_arch_system_num_rmid_idx(void)
>>>  	return num_rmids == U32_MAX ? 0 : num_rmids;
>>>  }
>>>  
>>> +/**
>>> + * resctrl_arch_system_max_rmid_idx - Largest possible number of RMIDs
>>> + *
>>> + * Return: If L3 monitoring is supported, largest possible comes from L3 based
>>> + * on CPUID(0xf,0x0).EBX (scaled down on Sub-NUMA Cluster systems). Otherwise
>>> + * maximum from any other mon_capable resources.
>>
>> Above reads "Otherwise maximum from any other ..." but the code is actually
>> "Otherwise minimum from any other ..." which contradicts the "max" intention of
>> this function?
>>
>>> + */
>>> +u32 resctrl_arch_system_max_rmid_idx(void)
>>> +{
>>> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>>> +
>>> +	return r->mon_capable ? r->mon.num_rmid : resctrl_arch_system_num_rmid_idx();
>>
>> This smells like a function that pretends to be generic but created to provide
>> correct results on specific x86 hardware. 
>> If this needs to provide max RMID why not cycle through all monitoring resources
>> and determine the max RMID, why special case L3?
>>
>> ...
>>
>>> @@ -961,8 +975,8 @@ void mbm_setup_overflow_handler(struct rdt_l3_mon_domain *dom, unsigned long del
>>>  
>>>  int setup_rmid_lru_list(void)
>>>  {
>>> +	u32 max_idx_limit, min_idx_limit;
>>>  	struct rmid_entry *entry = NULL;
>>> -	u32 idx_limit;
>>>  	u32 idx;
>>>  	int i;
>>>  
>>> @@ -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.

Reinette