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

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

Hi Chenyu,

On 10/8/26 12:00 AM, Chen Yu wrote:
> On Mon, Sep 28, 2026 at 02:54:48PM -0700, Reinette Chatre wrote:
>> On 9/17/26 9:51 PM, Chen Yu wrote:
 
>>> 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?

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

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

> 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."
>>> diff --git a/arch/x86/kernel/cpu/resctrl/erdt.c b/arch/x86/kernel/cpu/resctrl/erdt.c
>>> index 400973ebf2b8..f350a516d3d9 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/erdt.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/erdt.c

...

>>> +static int erdt_read_l3_occupancy(const struct erdt_domain_info *d, u32 rmid, u64 *val)
>>> +{
>>> +	struct acpi_erdt_cmrc *cmrc;
>>> +	u64 l3_cmt_count;
>>> +	u32 offset;
>>> +
>>> +	cmrc = d->cmrc;
>>> +	if (!cmrc)
>>> +		return -EIO;
>>> +
>>> +	offset = cmrc_index_function_1(cmrc, rmid);
>>> +	/* Overflow of cmt_reg_size * SZ_4K already validated in erdt_ioremap(). */
>>> +	if (offset + sizeof(u64) > (u32)cmrc->cmt_reg_size * SZ_4K)
>>> +		return -EINVAL;
>>> +
>>> +	l3_cmt_count = readq(d->base[ERDT_MMIO_CMRC_BASE] + offset);
>>> +	if ((cmrc->flags & CMRC_FLAG_UNAVAILABLE_BIT) &&
>>> +	    (l3_cmt_count & UNAVAILABLE_COUNTER))
>>> +		return -EINVAL;
>>> +
>>> +	/*
>>> +	 * 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.

Reinette