Re: [PATCH v9 04/12] x86/resctrl: Do not enable ERDT when Sub-NUMA Cluster is enabled

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

Hi Chenyu,

On 10/13/26 7:58 PM, Chen Yu wrote:
> ERDT cannot coexist with SNC(Sub-NUMA-Clustering). The definition of SNC

"Sub-NUMA-Clustering" -> "Sub-NUMA-Cluster mode"

> is to 'divide' an L3 into smaller L3 slices by mapping addresses to
> different L3 slices. The current platform with ERDT enabled has several
> L3 per socket, and it will not be divided into smaller slices.
> Furthermore, ERDT has only L3 scope for the CPU agent, and there are no
> node-scope domains.
> 
> snc_nodes_per_l3_cache is calculated in rdt_get_l3_mon_config(), which
> runs after erdt_init() has parsed the ACPI ERDT table. Introduce
> resctrl_arch_update_snc() to do that calculation and call it from
> get_rdt_resources() before erdt_init(). Leave ERDT disabled if SNC is
> detected and keep using the legacy MSR based CMT interface. Do this check
> while parsing the ERDT table so that platforms that do not provide an
> ERDT table at all stay silent.

Please do not just narrate the code in changelog but instead describe
at high level in regular language what the change does.

> 
> Suggested-by: Tony Luck <tony.luck@intel.com>
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> Tested-by: Hongyu Ning <hongyu.ning@linux.intel.com>
> ---


> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 7fadfebb5312..0ea888807bd0 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -1018,6 +1018,7 @@ static __init bool get_rdt_resources(void)
>  {
>  	bool ret;
>  
> +	resctrl_arch_update_snc();

"resctrl_arch_..." is the common prefix for helpers found in include/linux/resctrl.h that all
architectures are required to implement.
Is a helper really needed here? It is just one line, could it just be open coded?

>  	erdt_init();
>  	rdt_alloc_capable = get_rdt_alloc_resources();
>  	rdt_mon_capable = get_rdt_mon_resources();
> diff --git a/arch/x86/kernel/cpu/resctrl/erdt.c b/arch/x86/kernel/cpu/resctrl/erdt.c
> index eb5e262e243a..d94c54617fa1 100644
> --- a/arch/x86/kernel/cpu/resctrl/erdt.c
> +++ b/arch/x86/kernel/cpu/resctrl/erdt.c
> @@ -229,6 +229,18 @@ static __init int enumerate_erdt_table(struct acpi_table_header *table_hdr)
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * ERDT describes monitoring resources per L3 cache domain, while SNC
> +	 * splits an L3 cache into NUMA node scoped monitoring domains. The two
> +	 * cannot be matched up, so leave ERDT disabled and fall back to the
> +	 * MSR based CMT interface. snc_nodes_per_l3_cache has been computed by

"MSR CMT interface" - this restriction is not specific to CMT, is it? Could "CMT"
just be dropped?

Last sentence of comment can be dropped. It surely will be a bug if snc_nodes_per_l3_cache
is not initialized before it is used here and one can just go find where that happens.

> +	 * resctrl_arch_update_snc() before erdt_init() is called.
> +	 */
> +	if (snc_nodes_per_l3_cache > 1) {

Looks like this check occurs *after* parsing of ERDT table already started. Could this
be moved to be the first check to avoid that unnecessary work?

> +		pr_info("Not supported with Sub-NUMA Cluster enabled, using legacy CMT\n");

"using legacy CMT" ERDT is not limited to CMT so this can just be dropped?
I understand that the pr_info() adds an "ERDT:" prefix but I think it will be useful
to include in this message that ERDT in its entirety is not supported:
 "ERDT not supported with Sub-NUMA Cluster enabled".

I think this work could be integrated better by switching the order between
this patch and "x86/resctrl: Parse ACPI ERDT table and save CACD cpumask per RMD",
separating the change to where snc_nodes_per_l3_cache is initialized and the new
dependency by moving just this hunk to "x86/resctrl: Parse ACPI ERDT table and save
CACD cpumask per RMD" with a note in the changelog that ERDT and SNC are not compatible.

> +		return -EINVAL;
> +	}
> +
>  	for (subtbl = (void *)erdt + sizeof(*erdt);
>  	     subtbl_valid((void *)erdt + erdt->header.length, subtbl);
>  	     subtbl = next_subtbl(subtbl)) {
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index e2f3b884847c..34d29f719076 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -177,6 +177,8 @@ static inline struct rdt_hw_resource *resctrl_to_arch_res(struct rdt_resource *r
>  
>  extern struct rdt_hw_resource rdt_resources_all[];
>  
> +extern int snc_nodes_per_l3_cache;
> +
>  void arch_mon_domain_online(struct rdt_resource *r, struct rdt_l3_mon_domain *d);
>  
>  /* CPUID.(EAX=10H, ECX=ResID=1).EAX */
> @@ -251,6 +253,7 @@ union l3_qos_abmc_cfg {
>  void rdt_ctrl_update(void *arg);
>  
>  int rdt_get_l3_mon_config(struct rdt_resource *r);
> +__init void resctrl_arch_update_snc(void);
>  
>  bool rdt_cpu_has(int flag);
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 3838e0a13d36..6e46f10d8640 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -34,7 +34,7 @@ bool rdt_mon_capable;
>  
>  #define CF(cf)	((unsigned long)(1048576 * (cf) + 0.5))
>  
> -static int snc_nodes_per_l3_cache = 1;
> +int snc_nodes_per_l3_cache = 1;
>  
>  /*
>   * The correction factor table is documented in Documentation/filesystems/resctrl.rst.
> @@ -416,6 +416,11 @@ static __init int snc_get_config(void)
>  	return ret;
>  }
>  
> +__init void resctrl_arch_update_snc(void)
> +{
> +	snc_nodes_per_l3_cache = snc_get_config();
> +}
> +

This looks unnecessary. Could all these references to snc_nodes_per_l3_cache be dropped
from monitor.c?
Definition of snc_nodes_per_l3_cache could be moved to core.c where it is
initialized now and its initialization could just be open coded in get_rdt_resources().

>  int __init rdt_get_l3_mon_config(struct rdt_resource *r)
>  {
>  	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
> @@ -423,8 +428,6 @@ int __init rdt_get_l3_mon_config(struct rdt_resource *r)
>  	unsigned int threshold;
>  	u32 eax, ebx, ecx, edx;
>  
> -	snc_nodes_per_l3_cache = snc_get_config();
> -
>  	resctrl_rmid_realloc_limit = boot_cpu_data.x86_cache_size * 1024;
>  	hw_res->mon_scale = boot_cpu_data.x86_cache_occ_scale / snc_nodes_per_l3_cache;
>  	r->mon.num_rmid = (boot_cpu_data.x86_cache_max_rmid + 1) / snc_nodes_per_l3_cache;

Reinette