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

[Luck, Tony]

On Tue, Jun 09, 2026 at 04:03:53PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 6/9/26 11:46 AM, Luck, Tony wrote:
> 
> ...
>  
> > 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?
> 
> Thanks for catching this. I think it is reasonable to include it in this series
> as a preparatory patch with this patch helping to motivate its inclusion.

OK. I'll add a new patch to the series for this.

> 
> Even so, I am now a bit confused and concerned about the PMT dependencies. I am
> not very familiar with module_get()/module_put() capabilities - can it be
> guaranteed that PMT remains accessible between those two calls? I peeked at the
> sashiko review and it mentioned the usage of "unbind" triggered from user space.
> It sounds to me as though PMT's .probe() and .remove() can be triggered at
> various times from user space irrespective of resctrl being mounted or not and not
> prevented by a module_get()?
> 
> So, consider scenario where PMT is loaded and then resctrl is mounted and user space
> creates a couple of monitor groups that contains the AET event files. What will
> happen if user space then unbinds PMT? From what I can tell this will not
> trigger AET unmount like resctrl unmount would and thus leave a lot of dangling state?
> 
> Back to the above ... if AET needs handle a PMT unbind, do you think that, for
> example like in is_rmid_match(), that resctrl_arch_mon_capable() could return
> different values during a single resctrl mount?

I'm pursing options to prevent the unbind (until someone tells me there
is a critical use case that needs this to work at any arbitrary moment).

If preventing unbind works out, then resctrl_arch_mon_capable() won't change during
a single mount.
> 
> Reinette

-Tony