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

[Luck, Tony]

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.

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());

> > +
> > +	INIT_LIST_HEAD(&rmid_free_lru);
> > +
> >  	/*
> >  	 * RESCTRL_RESERVED_CLOSID and RESCTRL_RESERVED_RMID are special and
> >  	 * are always allocated. These are used for the rdtgroup_default
> > @@ -998,8 +1014,14 @@ int setup_rmid_lru_list(void)
> >  	 */
> >  	idx = resctrl_arch_rmid_idx_encode(RESCTRL_RESERVED_CLOSID,
> >  					   RESCTRL_RESERVED_RMID);
> > -	entry = __rmid_entry(idx);
> > -	list_del(&entry->list);
> > +
> > +	for (i = 0; i < min_idx_limit; i++) {
> > +		entry = &rmid_ptrs[i];
> > +		/* Don't add reserved or busy entries to free list */
> > +		if (i == idx || entry->busy)
> > +			continue;
> > +		list_add_tail(&entry->list, &rmid_free_lru);
> > +	}
> >  
> >  	return 0;
> >  }
> 
> Reinette

-Tony