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

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

Hi Reinette,

On Mon, Sep 28, 2026 at 02:54:48PM -0700, Reinette Chatre wrote:
> Hi Chenyu,
> 
> On 9/17/26 9:51 PM, Chen Yu wrote:
> > Add erdt_mon_read() to read LLC occupancy via MMIO and use it when the platform
> > supports ERDT. Register the L3 occupancy event with ERDT enabled when available,
> > falling back to the MSR-based path otherwise.
> > 
> > Use the CMRC (Cache Monitoring Registers for CPU Agents Description) ACPI
> > sub-table to read LLC occupancy counters for each RMID via MMIO when ERDT is
> > enabled. Store the CMRC information in the rdt_hw_l3_mon_domain, which could be
> > accessed directly.
> 
> Regarding "Store the CMRC ..." - wasn't that done in patch #5?
>

Yes, let me remove this duplicated description, to only focus on current
patch.

> > 
> > Although the occupancy counters can now be read from any CPU via MMIO, the
> > per-domain limbo handler is kept. A global handler would still have to iterate
> 
> "per-domain limbo handler is kept" - needs imperative
>

OK, will revise the description.

> > over every domain and would only save one worker thread, which does not justify
> 
> "would only save one worker thread"? this assumes that all systems will have two domains?
>

I intended to say each domain saves one worker, but actually it will save nr_domains - 1
in total. Let me revise the description to make it clearer.

> > maintaining two limbo handler models.
> > 
> > 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>
> > ---
> >  arch/x86/include/asm/resctrl.h         |  8 ++-
> >  arch/x86/kernel/cpu/resctrl/core.c     |  5 +-
> >  arch/x86/kernel/cpu/resctrl/erdt.c     | 74 +++++++++++++++++++++++++-
> >  arch/x86/kernel/cpu/resctrl/internal.h |  9 +++-
> >  arch/x86/kernel/cpu/resctrl/monitor.c  | 14 +++--
> >  5 files changed, 102 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> > index bcfcc8ac1fe6..31c2f81cd06e 100644
> > --- a/arch/x86/include/asm/resctrl.h
> > +++ b/arch/x86/include/asm/resctrl.h
> > @@ -135,7 +135,13 @@ static inline void __resctrl_sched_in(struct task_struct *tsk)
> >  
> >  static inline unsigned int resctrl_arch_round_mon_val(unsigned int val)
> >  {
> > -	unsigned int scale = boot_cpu_data.x86_cache_occ_scale;
> > +	unsigned int scale = boot_cpu_data.x86_cache_occ_scale, escale;
> > +
> > +	if (erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
> > +		escale = erdt_get_scale();
> > +		if (escale)
> > +			scale = escale;
> > +	}
> >  
> >  	/* h/w works in units of "boot_cpu_data.x86_cache_occ_scale" */
> 
> This comment is no longer accurate.
>

OK, will revise this.

> >  	val /= scale;
> > 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
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.
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
> > @@ -26,6 +26,10 @@ static bool erdt_enabled;
> >  #define CMRC_SUPPORTED_INDEX_FN		1
> >  #define RMDD_FLAG_CPU_L3_DOMAIN		BIT(0)
> >  
> > +/* Set in a monitoring counter when it holds no valid data to report. */
> > +#define UNAVAILABLE_COUNTER		BIT_ULL(63)
> 
> Can this be CMRC_UNAVAILABLE_COUNTER? (or some other appropriate prefix that makes
> it more specific)
>

Yes, let me rename it to this.

> > +#define CMRC_FLAG_UNAVAILABLE_BIT	BIT(0)
> > +
> >  /* Bitmask of valid sub-tables found in the first RMDD, used to ensure all RMDDs match. */
> >  static u32 valid_subtbl_mask;
> >  
> > @@ -50,7 +54,12 @@ static unsigned int erdt_scale;
> >  
> >  bool erdt_support(int flag)
> >  {
> > -	return false;
> > +	switch (flag) {
> > +	case X86_FEATURE_CQM_OCCUP_LLC:
> > +		return valid_subtbl_mask & BIT(ACPI_ERDT_TYPE_CMRC);
> > +	default:
> > +		return false;
> > +	}
> >  }
> >  
> >  unsigned int erdt_get_max_rmid(void)
> > @@ -60,7 +69,68 @@ unsigned int erdt_get_max_rmid(void)
> >  
> >  unsigned int erdt_get_scale(void)
> >  {
> > -	return erdt_scale;
> > +	/* Divided by snc_nodes_per_l3_cache, see erdt_read_l3_occupancy(). */
> > +	return erdt_scale / snc_nodes_per_l3_cache;
> > +}
> > +
> 
> 
> > +static u32 cmrc_index_function_1(struct acpi_erdt_cmrc *cmrc, u32 rmid)
> Please add a function comment connecting cmrc_index_function_1() to CMRC_SUPPORTED_INDEX_FN
> and also documents where this function comes from (eg. RDT architecture spec).
>

OK, will do.

> > +{
> > +	/*
> > +	 * MMIO_offset_for_RMID# =
> > +	 *   (RMID / ClumpSize) * Stride +
> > +	 *   (RMID % ClumpSize) * 8
> 
> Looks like above can just be on one line? When adding function comment about origin of
> algorithm it may be more fitting to move this comment there.
>

OK, will re-format above comment.

> > +	 */
> > +	return (rmid / cmrc->clump_size) * cmrc->clump_stride +
> > +	       (rmid % cmrc->clump_size) * 8;
> > +}
> > +
> > +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?

thanks,
Chenyu