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

[Reinette Chatre <reinette.chatre@intel.com>]

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.

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?

Reinette