Re: [PATCH v9 07/12] x86/resctrl: Add a global ERDT monitor scale

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

Hi Chenyu,

On 10/13/26 7:59 PM, Chen Yu wrote:
> The limbo handler polls the LLC occupancy of stale RMIDs and frees an RMID

"stale RMIDs"? This is a new concept unfamiliar to resctrl.

> once its occupancy drops below max_threshold_occupancy. User space writes

Please do not interchangeably use the name of the resctrl file and the variable
that backs it because the variable that backs the file may not have the value
that user space wrote to the file.

> that threshold in bytes, but the hardware counts occupancy in units of the
> occupancy scale, so the threshold is rounded down to a multiple of that
> scale. The scale is the number of bytes one counter unit represents, i.e.

This does not sound right, the threshold is explicitly noted as:
 	"This is the threshold cache occupancy in *bytes* ..."

> the granularity of the occupancy counter.
> 
> ERDT may provide per-domain monitoring scale factors, but resctrl rounds
> max_threshold_occupancy through a single global
> resctrl_rmid_realloc_threshold value that is visible to user space.
> 
> Prepare for that by adding erdt_scale/erdt_get_scale() and teaching

"Prepare for that" - Prepare for what?

> resctrl_arch_round_mon_val() to consult the ERDT scale when firmware has
> populated one.
> 
> When CMRC parsing provides per-domain up_scale values, erdt_scale will be
> updated with max(). The reason to use max is because rounding is a floor:
> a larger scale yields a slightly lower threshold, i.e. an RMID has to
> drop to a slightly lower occupancy before it is reused.
> 
> For now erdt_scale is intentionally left at its default value of 0.

This is a clear signal that the patch ordering needs to change.

> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> ---
> Changes since v8:
> - New patch. erdt_scale and erdt_get_scale() were introduced inside
>   "x86/resctrl: Parse ACPI CMRC table" without any mention in its
>   changelog. Split them out and describe why a single global scale is
>   used. (Reinette Chatre)
> - Reword the erdt_scale comment: it is not used by the limbo handler.
>   (Reinette Chatre)
> 
>  arch/x86/kernel/cpu/resctrl/erdt.c     | 16 ++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>  arch/x86/kernel/cpu/resctrl/monitor.c  |  5 ++++-
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/erdt.c b/arch/x86/kernel/cpu/resctrl/erdt.c
> index d4b630358d6d..09072e478bde 100644
> --- a/arch/x86/kernel/cpu/resctrl/erdt.c
> +++ b/arch/x86/kernel/cpu/resctrl/erdt.c
> @@ -38,6 +38,22 @@ static u16 first_rmdd_domain_id;
>   */
>  static unsigned int erdt_max_rmid;
>  
> +/*
> + * Used only to round the global resctrl_rmid_realloc_threshold for the limbo
> + * handler. resctrl_rmid_realloc_threshold is exposed to user space as a single
> + * global value, so ERDT uses one scale derived from the per-domain
> + * cmrc->up_scale values. max() keeps this single threshold conservative across
> + * domains because the rounding is a floor: a larger scale yields a slightly
> + * lower threshold, i.e. an RMID has to drop to a slightly lower occupancy
> + * before it is reused.

This documents changes that do not exist at this point in series. Please reorder
this patch to ensure its dependencies land first.

> + */
> +static unsigned int erdt_scale;
> +
> +unsigned int erdt_get_scale(void)
> +{
> +	return erdt_scale;

The name and usage implies that this is a global scale. This will become more confusing
when trying to land the memory bandwidth monitoring changes that have separate scale values.

Having this change pretend it is a global change and then add a snippet to final
patch to constrain it with a "if (!erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))" just makes
this work difficult to consume. 

If this is specific to the CMT scale only then let the global variable and its accessor
make that obvious. Let this change land after the CMRC parsing and erdt_cpu_has()
introduction so that it can be introduced as one coherent change.

> +}
> +
>  static void __iomem *erdt_ioremap(resource_size_t base, u32 num_pages, const char *desc)
>  {
>  	void __iomem *addr;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 48aa64c1ff96..75dd4736937d 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -284,6 +284,7 @@ static inline void intel_aet_mon_domain_setup(int cpu, int id, struct rdt_resour
>  static inline bool intel_handle_aet_option(bool force_off, char *tok) { return false; }
>  #endif
>  
> +unsigned int erdt_get_scale(void);
>  void erdt_init(void);
>  void erdt_exit(void);
>  bool erdt_try_bind_cpu(int cpu);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index e42ee2769a39..f1a28a3c81bc 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -423,7 +423,10 @@ __init void resctrl_arch_update_snc(void)
>  
>  unsigned int resctrl_arch_round_mon_val(unsigned int val)
>  {
> -	unsigned int scale = boot_cpu_data.x86_cache_occ_scale;
> +	unsigned int scale = erdt_get_scale();
> +
> +	if (!scale)
> +		scale = boot_cpu_data.x86_cache_occ_scale;
>  
>  	/* h/w works in units of "boot_cpu_data.x86_cache_occ_scale" */

The fixup of this comment found in patch #12 needs to be squashed here.

Reinette