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

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

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