Re: [PATCH v8 9/9] x86/resctrl: Add MMIO-based LLC occupancy monitoring support

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

Hi Reinette,

On Thu, Oct 08, 2026 at 08:52:26AM -0700, Reinette Chatre wrote:
> Hi Chenyu,
> 
>  
> >>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> >>> index ca7e67f976d5..135f2eb2a5a5 100644
> >>> --- a/arch/x86/kernel/cpu/resctrl/core.c
> >>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> >>> @@ -1007,7 +1007,10 @@ static __init bool get_rdt_mon_resources(void)
> >>>  	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> >>>  	bool ret = false;
> >>>  
> >>> -	if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
> >>> +	if (erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
> >>> +		resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, true, 0, NULL);
> >>> +		ret = true;
> >>> +	} else if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
> >>>  		resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, false, 0, NULL);
> >>>  		ret = true;
> >>>  	}
> >>
> >> Would https://lore.kernel.org/lkml/20260916231320.14502-3-tony.luck@intel.com/
> >> break this?
> >>
> > 
> > It would not break this - because the ERDT exists independently of the CPUID
> > query result, but the CPUID result should be the fundamental check before ERDT
> 
> Does this mean this implementation, that parses ERDT _before_ CPUID, needs to be
> flipped? Is this required? This series parses the table before checking CPUID, leaving
> the decision whether to use it or not based on CPUID data checked afterwards. This
> seems ok and looks to support a simpler implementation?
>

Yes, currently the ERDT parsing is done before this CPUID check. We can leave the
ERDT parsing as it is, and then when it comes to the actual CPUID check, only when
CPUID is supported will we further check if the ERDT is supported. If yes, enable
the MMIO interface, something like:

if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
	resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC), 0, NULL);
	ret = true;
}

> > parsing - the ERDT can decide whether to use the MMIO or legacy MSR interface,
> > but the existence of CMT support should be the first thing to check. I
> > got the following feedback from the RDT arch:
> > "Architecturally, CPUID remains the mechanism that enumerates RDT monitoring
> > capability and the specific monitoring features exposed to software. The ERDT 
> > CMRC table provides topology/resource description associated with those monitoring
> > capabilities, but it is not itself a capability enumeration mechanism.
> 
> If I interpret this correctly a system with a CMRC table still has to indicate
> LLC occupancy support via CPUID.0FH.01H:EDX?

Yes, this is my understanding.

> For the implementation that would mean
> MMIO interface is only used when rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC) *and* 
> erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC) are true?
>

Yes, I think so.

> Apart from that it sounds like Tony's patch will support this work to make the
> enumeration dependency clear.
>

Yes, Tony's patch helps filter the platforms without CPUID support, even if the ERDT
table is present (unlikely).

> > From that perspective, a CMRC table existing without the associated CPUID monitoring
> > enumeration would create an architectural inconsistency, as software would have topology
> > information for a monitoring capability that has not been exposed through the
> > architectural discovery mechanism."
> >>> +	/*
> >>> +	 * In legacy mode, scale is divided by snc_nodes_per_l3_cache to
> >>> +	 * prevent over-calculation of aggregated monitor data, do it
> >>> +	 * the same for MMIO based access.
> >>> +	 * This scaling factor might need to be revisited/tuned for future
> >>> +	 * platforms that support both SNC and MMIO-based monitoring
> >>> +	 * simultaneously.
> >>> +	 */
> >>> +	*val = l3_cmt_count * cmrc->up_scale / snc_nodes_per_l3_cache;
> >>
> >> Please consider all sashiko's comments about SNC systems - from what I can tell
> >> the comments are accurate and the SNC support needs a second look.
> >>
> > 
> > Yes, I saw Sashiko's comments about SNC, but it looks like it's not a practical
> > issue for now - at least for the current platform, I'm not sure how ERDT and SNC
> > can co-exist:
> > 
> > The definition of SNC (Sub-NUMA Clustering) is to 'divide' an L3 into smaller L3
> > slices by mapping addresses to different L3 slices. But the current platform with
> > ERDT enabled has 4 L3 per socket, and it is unlikely this L3 is further divided.
> > So my understanding is that, unless the real platform will do the SNC division,
> > and with ERDT enhanced to support SNC node (currently ERDT is only L3 scope for
> > the CPU agent, no node-scope domains), we can consider SNC. For now I do not see
> > the need to consider SNC on an ERDT-enabled platform. Maybe we can print a warning
> > if snc_nodes_per_l3_cache > 1 that the ERDT parsing should be stopped and should
> > fall back to the legacy MSR interfaces?
> Yes, if SNC need not be supported then please do make that clear. The current implementation,
> for example in the line above, makes it seem as though SNC is supported but for correctness
> then subtly requires it not to.
>

OK, let me revise the code/comment to explicitly say that "SNC and ERDT cannot co-exist".

thanks,
Chenyu