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

["Luck, Tony" <tony.luck@intel.com>]

On Tue, Jul 14, 2026 at 08:18:30AM -0700, Reinette Chatre wrote:
> 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? 

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;

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

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

-Tony
> 
> 
>