Re: [PATCH v7 07/14] x86/resctrl: Maintain a count of enabled monitor features

[Luck, Tony]

On Mon, Jun 08, 2026 at 04:18:54PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 6/1/26 12:56 PM, Tony Luck wrote:
> > AET (Application Energy Telemetry) may be enabled/disabled from one mount
> > to the next depending on whether the pmt_telemetry module is loaded. If
> > AET is the only monitoring feature supported on a system and it is enabled
> > in one mount, but disabled in a subsequent mount this will result in empty
> > mon_data directories.
> > 
> > Change from a boolean to a count of enabled monitor features inside
> > architecture code. File system code only needs to know if any monitor
> 
> First sentence seems to be missing what is being changed.
> 
> > features are enabled so resctrl_arch_mon_capable() can still return
> > boolean.
> > 
> > Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> > ---
> >  arch/x86/include/asm/resctrl.h         |  4 ++--
> >  arch/x86/kernel/cpu/resctrl/internal.h |  2 +-
> >  arch/x86/kernel/cpu/resctrl/core.c     | 24 +++++++++++++-----------
> >  arch/x86/kernel/cpu/resctrl/monitor.c  | 11 +++--------
> >  4 files changed, 19 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> > index 575f8408a9e7..1e50c7dc3fe3 100644
> > --- a/arch/x86/include/asm/resctrl.h
> > +++ b/arch/x86/include/asm/resctrl.h
> > @@ -43,7 +43,7 @@ struct resctrl_pqr_state {
> >  DECLARE_PER_CPU(struct resctrl_pqr_state, pqr_state);
> >  
> >  extern bool rdt_alloc_capable;
> > -extern bool rdt_mon_capable;
> > +extern int rdt_mon_feature_count;
> >  
> >  DECLARE_STATIC_KEY_FALSE(rdt_enable_key);
> >  DECLARE_STATIC_KEY_FALSE(rdt_alloc_enable_key);
> > @@ -68,7 +68,7 @@ static inline void resctrl_arch_disable_alloc(void)
> >  
> >  static inline bool resctrl_arch_mon_capable(void)
> >  {
> > -	return rdt_mon_capable;
> > +	return !!rdt_mon_feature_count;
> >  }
> >  
> 
> This seem unnecessarily complicated to me. Can global "rdt_mon_capable" instead be dropped
> and let resctrl_arch_mon_capable() just return "true" if any of the resources are
> "mon_capable"?

Conceptually this is much simpler. But I have questions about the
implementation. The new function is trivial:

bool resctrl_arch_mon_capable(void)
{
        struct rdt_resource *r;

        for_each_mon_capable_rdt_resource(r)
                return true;

        return false;
}

But that led me to #include hell when I tried to keep it as an inline
function in <asm/resctrl.h> because for_each_mon_capable_rdt_resource()
is defined in <linux/resctrl.h> after the #include <asm/resctrl.h>

So I've moved it out-of-line into arch/x86/kernel/cpu/resctrl/core.c.

The MPAM implementation is also out-of-line.

But then I wondered about performance. This change on x86 goes from an
inline function that simply returns the value of a global variable to an
out-of-line function that scans the array of rdt resources. The common
case will be a hit on the first element, so not awful. But still worse
that before I touched it.

So I looked for places where resctrl_arch_mon_capable() is called in
"hot" code paths. There's a bunch in mount and mkdir, but those aren't
very hot.

My list (check to see if I missed any others):

1) Recurring call once per second in mbm_handle_overflow()

Seems redundant. There is a check to only start the overflow handler
on mon_capable systems (only with enabled MBM events!)

2) Call for potentially every task when reading tasks files in is_rmid_match()

Also seems redundant. Next part of that "if" looks at "r->type == RDTMON_GROUP"
which can only be true on mon_capable systems.

Should I clean these up in this series? As part of this patch which
exacerbates the performance impact, or as a separate cleanup patch?
> 
> Reinette
> 
-Tony