Re: [PATCH v10 07/17] arm,x86,fs/resctrl: Handle change in number of RMIDs on each mount

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

Hi Tony,

On 7/29/26 10:27 AM, 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. Use this same
> calculated value for all references to rmid_ptrs[] instead of calling
> resctrl_arch_system_max_rmid_idx() in multiple places.
> 
> Also use this maximum RMID value when allocating
> rdt_l3_mon_domain::rmid_busy_llc bitmap and rdt_l3_mon_domain::mbm_states.

ok, but why? I have the same comment as v9 about this and I still do not see
why resctrl fs need to allocate the L3 monitoring state for "maximum RMID"
when this is unique to L3 monitoring with its own limits. There can never be
more state used than what L3 monitoring support so why not limit the state to
that instead of using the system wide maximum? 
Looking back at v9 the motivation is that "this works for x86" which causes
resctrl fs to obfuscate its implementation on x86 behavior without consideration
how it impacts other architectures.

resctrl fs already supports a per-resource resctrl_arch_get_num_closid(). Could
resctrl add a, for example, per-resource resctrl_arch_get_num_rmid_idx()?
If that was already available, would this patch not have used it instead of
using the system max?

> 
> 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/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
> index 226ff6f532fa..7079870ca894 100644
> --- a/drivers/resctrl/mpam_resctrl.c
> +++ b/drivers/resctrl/mpam_resctrl.c
> @@ -272,6 +272,11 @@ u32 resctrl_arch_system_num_rmid_idx(void)
>  	return (mpam_pmg_max + 1) * (mpam_partid_max + 1);
>  }
>  
> +u32 resctrl_arch_system_max_rmid_idx(void)
> +{
> +	return resctrl_arch_system_num_rmid_idx();
> +}
> +
>  u32 resctrl_arch_rmid_idx_encode(u32 closid, u32 rmid)
>  {
>  	return closid * (mpam_pmg_max + 1) + rmid;
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index d7ad976ed503..2a1d2ee2b91d 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -75,6 +75,11 @@ static unsigned int rmid_limbo_count;
>   */
>  static struct rmid_entry	*rmid_ptrs;
>  
> +/*
> + * @max_idx_limit - The number of elements allocated in *rmid_ptrs.

To be more specific, could this instead be: "The number of elements in rmid_ptrs[]."?

> + */
> +static u32 max_idx_limit;
> +
>  /*
>   * This is the threshold cache occupancy in bytes at which we will consider an
>   * RMID available for re-allocation.
> @@ -115,10 +120,18 @@ static inline struct rmid_entry *__rmid_entry(u32 idx)
>  
>  static void limbo_release_entry(struct rmid_entry *entry)
>  {
> +	u32 cur_idx_limit = resctrl_arch_system_num_rmid_idx();
> +
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
>  	rmid_limbo_count--;
> -	list_add_tail(&entry->list, &rmid_free_lru);
> +
> +	/*
> +	 * Limbo may be freeing an RMID from a previous mount where there
> +	 * were more RMIDs available.
> +	 */
> +	if (resctrl_arch_rmid_idx_encode(entry->closid, entry->rmid) < cur_idx_limit)
> +		list_add_tail(&entry->list, &rmid_free_lru);
>  
>  	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
>  		closid_num_dirty_rmid[entry->closid]--;
> @@ -133,7 +146,6 @@ static void limbo_release_entry(struct rmid_entry *entry)
>  void __check_limbo(struct rdt_l3_mon_domain *d, bool force_free)
>  {
>  	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
> -	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
>  	struct rmid_entry *entry;
>  	bool rmid_dirty = true;
>  	u32 idx, cur_idx = 1;
> @@ -156,8 +168,12 @@ void __check_limbo(struct rdt_l3_mon_domain *d, bool force_free)
>  	 * RMID and move it to the free list when the counter reaches 0.
>  	 */
>  	for (;;) {
> -		idx = find_next_bit(d->rmid_busy_llc, idx_limit, cur_idx);
> -		if (idx >= idx_limit)
> +		/*
> +		 * Need to check all possible RMIDs, not just the range
> +		 * available in this mount cycle.
> +		 */

This just documents what can be seen from the code. Would be more helpful to have
comment describe *why* it is possible for an RMID different from the available range
to be busy.

> +		idx = find_next_bit(d->rmid_busy_llc, max_idx_limit, cur_idx);
> +		if (idx >= max_idx_limit)
>  			break;
>  
>  		entry = __rmid_entry(idx);

Reinette