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

[Reinette Chatre]

Hi Tony,

On 8/31/26 10:44 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.
> 
> Add resctrl_arch_get_num_rmid_idx(r) to report the maximum RMID index
> for a resource. Use it to allocate the rdt_l3_mon_domain::rmid_busy_llc
> bitmap and rdt_l3_mon_domain::mbm_states and when operating on these
> structures.
> 
> 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.

(last sentence needs imperative)

The changelog is reading more like a list of changes. Can some of these
be split out?

> 
> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> ---
> v11:
> 	Add resctrl_arch_get_num_rmid_idx(r) and use it for allocating
> 	an operating on L3 per-domain dynamically allocated structures.
> 	Update kernel doc comment for max_idx_limit.
> 	Update comment to explain why all RMIDs need to be checked
> 	for LLC cache occupancy.
> 
>  include/linux/resctrl.h            |  8 ++-
>  arch/x86/kernel/cpu/resctrl/core.c | 30 +++++++++++
>  drivers/resctrl/mpam_resctrl.c     | 14 +++++
>  fs/resctrl/monitor.c               | 87 +++++++++++++++++++++---------
>  fs/resctrl/rdtgroup.c              |  6 +--
>  5 files changed, 114 insertions(+), 31 deletions(-)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index e9094d886ba7..4fb06d434c85 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -183,10 +183,12 @@ struct mbm_cntr_cfg {
>   * struct rdt_l3_mon_domain - group of CPUs sharing RDT_RESOURCE_L3 monitoring
>   * @hdr:		common header for different domain types
>   * @ci_id:		cache info id for this domain
> - * @rmid_busy_llc:	bitmap of which limbo RMIDs are above threshold
> + * @rmid_busy_llc:	bitmap of which limbo RMIDs are above threshold. Sized for
> + *			maximum supported RMIDs in L3 resource.
>   * @mbm_states:		Per-event pointer to the MBM event's saved state.
>   *			An MBM event's state is an array of struct mbm_state
>   *			indexed by RMID on x86 or combined CLOSID, RMID on Arm.
> + *			Sized same as @rmid_busy_llc.
>   * @mbm_over:		worker to periodically read MBM h/w counters
>   * @cqm_limbo:		worker to periodically read CQM h/w counters
>   * @mbm_work_cpu:	worker CPU for MBM h/w counters
> @@ -440,9 +442,11 @@ static inline u32 resctrl_get_default_ctrl(struct rdt_resource *r)
>  	return WARN_ON_ONCE(1);
>  }
>  
> -/* The number of closid supported by this resource regardless of CDP */
> +/* The number of closid/rmid supported by this resource regardless of CDP */
>  u32 resctrl_arch_get_num_closid(struct rdt_resource *r);
> +u32 resctrl_arch_get_num_rmid_idx(struct rdt_resource *r);
>  u32 resctrl_arch_system_num_rmid_idx(void);
> +u32 resctrl_arch_system_max_rmid_idx(void);
>  int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);
>  
>  /**
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index e851da431dd9..ef37fbb586d3 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -124,6 +124,31 @@ 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: Maximum possible number of RMIDs used for boot time allocations.
> + */
> +u32 resctrl_arch_system_max_rmid_idx(void)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +	u32 ret;
> +
> +	/* CPUID enumerates maximum value that can be written to IA32_PQR_ASSOC.RMID */
> +	ret = cpuid_ebx(0xf) + 1;

This patch with its one caller of resctrl_arch_system_max_rmid_idx() seems ok but this
series adds more callers and with that the repeated CPUID does not seem necessary.
Looking back I wonder if it will not make this work easier to consume if this RMID
count is instead enumerated from get_rdt_mon_resources() and stored in a global
variable. I think doing so would help to understand the dependencies among and capabilities
of the related feature bits. Something like:

	get_rdt_mon_resources()
	{

		if (!cpu_feature_enabled(X86_FEATURE_RDT_M)) /* or boot_cpu_has() */
			return false;

		/* Maximum value that can be written to IA32_PQR_ASSOC.RMID */
		pqr_assoc_max_rmid = cpuid_ebx(0xf) + 1;

		if (!cpu_feature_enabled(X86_FEATURE_L3_MON)) /* or boot_cpu_has() */
			return pqr_assoc_max_rmid > 0;

		/* L3 monitoring enumeration */

		return resctrl_arch_system_max_rmid_idx() > 0;
	}
			
With something like above resctrl_arch_system_max_rmid_idx() could use pqr_assoc_max_rmid
instead of calling CPUID every time?

> +
> +	/*
> +	 * If the system is capable of L3 monitoring the maximum RMID value may
> +	 * be lower than the system maximum. Either because the L3 monitoring
> +	 * feature supports fewer RMIDs, or because SNC (Sub-NUMA Cluster)
> +	 * is enabled and divides RMIDs per cluster.
> +	 */
> +	if (r->mon_capable)
> +		ret = r->mon.num_rmid;
> +
> +	return ret;
> +}
> +
>  struct rdt_resource *resctrl_arch_get_resource(enum resctrl_res_level l)
>  {
>  	if (l >= RDT_NUM_RESOURCES)
> @@ -360,6 +385,11 @@ u32 resctrl_arch_get_num_closid(struct rdt_resource *r)
>  	return resctrl_to_arch_res(r)->num_closid;
>  }
>  
> +u32 resctrl_arch_get_num_rmid_idx(struct rdt_resource *r)
> +{
> +	return r->mon.num_rmid;
> +}
> +
>  void rdt_ctrl_update(void *arg)
>  {
>  	struct rdt_hw_resource *hw_res;
> diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
> index 0db62dd2a71c..a117aa98ae90 100644
> --- a/drivers/resctrl/mpam_resctrl.c
> +++ b/drivers/resctrl/mpam_resctrl.c
> @@ -247,11 +247,25 @@ u32 resctrl_arch_get_num_closid(struct rdt_resource *ignored)
>  	return mpam_partid_max + 1;
>  }
>  
> +/*
> + * File system calls this for one-time allocation of structures
> + * during initialization. Return the largest possible value.
> + */

Above comment seems more appropriate for resctrl_arch_system_max_rmid_idx().

> +u32 resctrl_arch_get_num_rmid_idx(struct rdt_resource *ignored)
> +{
> +	return resctrl_arch_system_num_rmid_idx();
> +}
> +
>  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 2a28fe04284b..5340c764bf7f 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 in rmid_ptrs[].
> + */
> +static u32 max_idx_limit;

Please change this patch to avoid local variables shadow this global.
For example, this patch adds a max_idx_limit local variable to
domain_setup_l3_mon_state.

While the name is technically accurate, could this variable perhaps be
more descriptive with a name like: "num_rmid_ptrs" ?

Reinette