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

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

Hi Reinette,

On Fri, Oct 23, 2026 at 05:08:44PM -0700, Reinette Chatre wrote:
> Hi Chenyu,
> 
> On 10/13/26 8:01 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.
> > 
> > Although the occupancy counters can now be read from any CPU via
> > MMIO, keep the per-domain limbo handler. A global handler would
> > still have to iterate over every domain and would save nr_domains - 1
> 
> Please write what "nr_domains - 1" means in English since as it is written
> it is not clear how this "does not justify maintaining two limbo handler
> models."
>

OK, I'll rephrase it to a simpler version, how about this:

Although the occupancy counters can now be read from any CPU via MMIO,
keep the per domain limbo handler. A single global handler would still
have to check the limbo RMIDs of every L3 domain, so it would do the same
amount of work. The only saving would be running one limbo worker instead
of one per L3 domain, which does not justify maintaining two limbo handler
models.

> > worker threads, which does not justify 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>
> > ---
> 
> > +
> > +unsigned int erdt_get_max_rmid(void)
> > +{
> > +	if (!erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))
> > +		return 0;
> 
> This does not look right. Why is this restriction needed? At first glance it seems to
> be a quirk used to simplify the get_rdt_mon_resources() changes but not actually
> be the right thing to do?
>

OK, I put the erdt_cpu_has() check here because the only ERDT feature supported in
the kernel is X86_FEATURE_CQM_OCCUP_LLC. But as you mentioned, erdt_max_rmid comes
from the RMDD max_rmid field, so it limits all RMIDs, not just those used for LLC occupancy.
Let me use the erdt_enabled check:

unsigned int erdt_get_max_rmid(void)
{
        if (!erdt_enabled)
                return 0;

        return erdt_max_rmid;
}

> > +
> > +	return erdt_max_rmid;
> >  }
> >  
> >  unsigned int erdt_get_scale(void)
> >  {
> > +	if (!erdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))
> > +		return 0;
> > +
> >  	return erdt_scale;
> 
> Constraining a global value to one feature is confusing, could this
> instead be, for exaple, erdt_get_cmt_scale() that returns erdt_cmt_scale
> that will be more accurate and clear why the erdt_cpu_has() check is there?

Yes, this would be clearer. It now uses erdt_get_cmt_scale() / erdt_cmt_scale.

> This change can be added to the introduction of this function when the patch
> doing so is placed correctly in the series.
>

OK, the scale_* helpers are moved into ("x86/resctrl: Add the ERDT CMT scale").
which comes after "x86/resctrl: Introduce erdt_cpu_has() and erdt_support()".

> > +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);
> > +	if (offset + sizeof(u64) > (u32)cmrc->cmt_reg_size * SZ_4K)
> 
> Could you please explain what this is validating here? 
> cmrc->cmt_reg_size is already u32 so the cast is unexpected. These comparisons
> with different types can be tricky, looks like right side is 32 bit and may be
> truncated before the comparison to left 64 bit?
>

My original intent was to force the value into a u32 in case this field is
invalid, but I realize it should be:
if (offset + sizeof(u64) > (u64)cmrc->cmt_reg_size * SZ_4K)

> >  static void __iomem *erdt_ioremap(resource_size_t base, u32 num_pages, const char *desc)
> >  {
> >  	void __iomem *addr;
> > @@ -161,6 +234,8 @@ static __init bool cmrc_init(struct acpi_subtbl_hdr_16 *subtbl,
> >  		return false;
> >  	}
> >  
> > +	erdt_scale = max(erdt_scale, cmrc->up_scale);
> > +
> 
> This change is out of place.
>

OK, moved to ("x86/resctrl: Add the ERDT CMT scale")

> > @@ -438,7 +442,7 @@ unsigned int resctrl_arch_round_mon_val(unsigned int val)
> >  	if (!scale)
> >  		scale = boot_cpu_data.x86_cache_occ_scale;
> >  
> > -	/* h/w works in units of "boot_cpu_data.x86_cache_occ_scale" */
> > +	/* h/w works in units of the occupancy scale  */
> 
> Please move to patch that introduces the "scale" usage to resctrl_arch_round_mon_val().
>

OK, will do.

> >  	val /= scale;
> >  	return val * scale;
> >  }
> > @@ -447,12 +451,21 @@ int __init rdt_get_l3_mon_config(struct rdt_resource *r)
> >  {
> >  	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
> >  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> > +	unsigned int erdt_max_rmid;
> 
> Please do not shadow globals.
>

OK, rename it to erdt_rmid_limit.

> >  	unsigned int threshold;
> > +	int max_rmid;
> >  	u32 eax, ebx, ecx, edx;
> 
> reverse fir please
>

OK, will fix it.

thanks,
Chenyu
> >  
> > +	erdt_max_rmid = erdt_get_max_rmid();
> > +	if (erdt_max_rmid)
> > +		max_rmid = min_t(unsigned int, erdt_max_rmid,
> > +				 boot_cpu_data.x86_cache_max_rmid);
> > +	else
> > +		max_rmid = boot_cpu_data.x86_cache_max_rmid;
> > +
> >  	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;
> > +	r->mon.num_rmid = (max_rmid + 1) / snc_nodes_per_l3_cache;
> >  	hw_res->mbm_width = MBM_CNTR_WIDTH_BASE;
> >  
> >  	if (mbm_offset > 0 && mbm_offset <= MBM_CNTR_WIDTH_OFFSET_MAX)
> 
> Reinette