Re: [PATCH v4 6/6] x86/resctrl: Add support for L3 occupancy monitoring via RMID MMIO read

[Chen, Yu C]

Hi Reinette,

On 6/24/2026 12:48 AM, Reinette Chatre wrote:
> Hi Chenyu,
>
> On 6/22/26 10:00 PM, Chen, Yu C wrote:
> > Hi Reinette,
> >
> > On 6/23/2026 5:30 AM, Reinette Chatre wrote:
> > > Hi Chenyu,
> > > > > > diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> > > > > > index 97c2f6bc7a5f..9b3b03279dd8 100644
> > > > > > --- a/arch/x86/include/asm/resctrl.h
> > > > > > +++ b/arch/x86/include/asm/resctrl.h
> > > > > > @@ -41,6 +41,8 @@ struct resctrl_pqr_state {
> > > > > >     };
> > > > > >       bool erdt_enabled(void);
> > > > > > +struct rdt_domain_hdr;
> > > > > > +int erdt_mon_read(struct rdt_domain_hdr *hdr, int ev_id, int rmid, u64 *val);
> > > > > >       DECLARE_PER_CPU(struct resctrl_pqr_state, pqr_state);
> > > > > >     diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > > > > > index 90730f0851fa..fe812f7190fc 100644
> > > > > > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > > > > > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > > > > > @@ -965,7 +965,7 @@ static __init bool get_rdt_mon_resources(void)
> > > > > >         bool ret = false;
> > > > > >           if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
> > > > > > -        resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, false, 0, NULL);
> > > > > > +        resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID, erdt_enabled(), 0, NULL);
> > > > > >             ret = true;
> > > > > >         }
> > > > > >         if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
> > > > >
> > > > > As mentioned in patch #1, when erdt_enabled() is true the enumeration still proceeds to
> > > > > enumerate the monitoring properties via CPUID to discover the number of RMIDs that the
> > > > > *MSR* supports and use it as the maximum RMID (and thus the maximum number of registers)
> > > > > that MMIO supports?
> > > >
> > > > OK, will switch to the maximum RMID exposed by ACPI table, if erdt_enabled() is true.
> > >
> > > I believe the issue is larger than just the RMID enumeration. The CPUID and ACPI enumeration
> > > appears to be fully intertwined. Taking a closer look at what above code does:
> > > it checks *CPUID* whether CMT is enabled and then enables the LLC occupancy event to blindly use
> > > MMIO if ERDT is enabled, irrespective of whether the ERDT tables include a cache monitoring table
> > > or not. How is it guaranteed that if ERDT is enabled that there is a cache monitoring table?
> > > Should it not be the existence of the ACPI cache monitoring table and its properties that
> > > determines whether the LLC occupancy counter using MMIO registers should be enabled?
> >
> > I see. How about replacing erdt_enabled() with fine-grained helper functions such as
> > erdt_has_cmrc(), erdt_has_mmrc(), and erdt_has_marc()? The latter two will be added
> > later for region-aware MBM/MBA. CMRC, MMRC and MARC are not guaranteed to coexist,
> > so splitting them into separate helpers would offer finer control.
> I am concerned where this is headed since it looks to me as though the plan is to
> sprinkle these finer grained checks throughout resctrl. 

The erdt_has_* helper functions would ideally live under 
arch/x86/kernel/cpu/resctrl/
rather than the generic fs/resctrl directory, as architecture-specific 
code in the
former path is allow to see erdt logic I suppose? That said, since there 
are a large
number of such routines, introduce a dedicated helper to handle this 
uniformly would
be better(using arch_priv)

> To me this sounds complicated
> and error prone. Consider that resctrl_enable_mon_event() has an arch_priv parameter. To me
> this seems to be the appropriate place for the architecture to give itself the needed
> information about how to read the event.

OK, if I understand correctly, we can use the following logic to hide
erdt from monitor read:

/*
 * helper to get the erdt's monitor arch_priv,
 * defined in erdt.c, NULL in other place.
 *
 * caller doesn't know about CMRC
 */
void *get_evt_priv(enum resctrl_event_id eventid)
{
        if (!erdt_enabled())
                return NULL;

        switch (eventid) {
        case QOS_L3_OCCUP_EVENT_ID:
                return cmrc_priv_valid ? &cmrc_priv : NULL;
        default:
                return NULL;
        }
}

arch/x86/kernel/cpu/resctrl/core.c
get_rdt_mon_resources():
if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC)) {
        void *priv = get_evt_priv(QOS_L3_OCCUP_EVENT_ID);

        resctrl_enable_mon_event(QOS_L3_OCCUP_EVENT_ID,
                                 priv != NULL, 0, priv);
}

arch/x86/kernel/cpu/resctrl/monitor.c
resctrl_arch_rmid_read():
        if (arch_priv)
                return erdt_mon_read(hdr, eventid, rmid, val);
        return arch_l3_read_event(hdr, rmid, eventid, val, r);

So in this way, there is no event-type checks in the read
path. Adding MMRC later only extends the switch in get_evt_priv().

thanks,
Chenyu