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

[Chen Yu <yu.c.chen@intel.com>]

Hi Reinette,

On Fri, Oct 23, 2026 at 04:54:04PM -0700, Reinette Chatre wrote:
> 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"
>

OK, will change it.

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

OK, I'll adjust above commit log accordingly.

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

OK, let me open-code it.

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

Yes, let me drop CMT.

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

OK dropped the last sentence.

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

Yes, let me move it at the beginning of enumerate_erdt_table().

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

OK, this was not specific to CMT, so I dropped the "CMT" term and changed it as
you suggested.

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

OK, a new patch is introduced
"x86/resctrl: Detect Sub-NUMA Cluster mode before enumerating resources"
to initialize the snc_nodes_per_l3_cache earlier, and keep the
"snc_nodes_per_l3_cache > 1" check in current patch.

> > +__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().
> 

Yes, I moved the definition of snc_nodes_per_l3_cache to core.c and initialize
it there, but we cannot move all references to snc_nodes_per_l3_cache out of monitor.c
for now because several places in monitor.c use it. After all, IMHO snc_nodes_per_l3_cache
was mainly introduced to deal with MBM.

 grep -r -n "snc_nodes_per_l3_cache" arch/x86/kernel/cpu/resctrl/monitor.c
101: * "snc_nodes_per_l3_cache == 1") no translation of the RMID value is
129:    if (snc_nodes_per_l3_cache == 1)
132:    return lrmid + (cpu_to_node(cpu) % snc_nodes_per_l3_cache) * r->mon.num_rmid;
384:    if (snc_nodes_per_l3_cache > 1)
457:    hw_res->mon_scale = boot_cpu_data.x86_cache_occ_scale / snc_nodes_per_l3_cache;
458:    r->mon.num_rmid = (max_rmid + 1) / snc_nodes_per_l3_cache;

thanks,
Chenyu
> >  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