Re: [PATCH v7 11/14] mpam,x86/resctrl: Resolve INTEL_PMT_TELEMETRY symbols at runtime

[Luck, Tony]

On Mon, Jun 08, 2026 at 04:25:51PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> Regarding a subject prefix ... We do not yet have a precedent for prefix
> when MPAM, x86 and fs is changed so this series may indeed have the
> opportunity to set that precedent. When doing so, please be consistent.
> 
> On 6/1/26 12:56 PM, Tony Luck wrote:
> > resctrl is always built-in, but INTEL_PMT_TELEMETRY and INTEL_TPMI are
> > logically independent and should be loadable modules.  Switch AET to use
> > the function-pointer registration API (introduced in the preceding patch)
> 
> Please drop "(introduced in the preceding patch)" since patches may not
> always keep this series's order once merged.

OK

> 
> > instead of direct link-time references to PMT symbols.
> > 
> > Prepare for the file system to call resctrl_arch_pre_mount() on every mount
> > by moving AET enumeration into resctrl_arch_pre_mount() and cleanup into
> > resctrl_arch_unmount(). This allows the PMT module to be unloaded whenever
> > the filesystem is not mounted.
> > 
> > Remove intel_aet_exit because all cleanup now happens in the unmount path.
> > 
> > Note that the Linux file system code does not serialize calls to
> > fs_context_operations::get_tree(), so there may be arbitrarily many
> > parallel calls if users invoke mount(2) multiple times.
> > 
> > Add locking and state (resctrl_arch_mount_entries) to avoid repeated
> > enumeration on nested mount requests from file system code (which will be
> > failed with -EBUSY status).
> > 
> > event_group::num_rmid may be reset (reduced) during enumeration. This is
> > not worth resetting on unmount because the same reduction would occur on
> > each subsequent mount.
> 
> Even more important to ensure that PMT is done with its enumeration when
> AET enumeration starts?

Would be nice, but I still don't know a way to make that happen.

> > 
> > Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> > ---
> >  include/linux/resctrl.h                 |  3 ++
> >  arch/x86/kernel/cpu/resctrl/internal.h  |  8 ++--
> >  arch/x86/kernel/cpu/resctrl/core.c      | 41 +++++++++++++++++--
> >  arch/x86/kernel/cpu/resctrl/intel_aet.c | 52 ++++++++++++++++++++++---
> >  drivers/resctrl/mpam_resctrl.c          |  4 ++
> >  5 files changed, 95 insertions(+), 13 deletions(-)
> > 
> > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> > index 3705f0214fa6..9534d42e0c57 100644
> > --- a/include/linux/resctrl.h
> > +++ b/include/linux/resctrl.h
> > @@ -557,6 +557,9 @@ void resctrl_offline_cpu(unsigned int cpu);
> >   */
> >  void resctrl_arch_pre_mount(void);
> >  
> > +/* Called to report unmount. */
> > +void resctrl_arch_unmount(void);
> > +
> >  /**
> >   * resctrl_arch_rmid_read() - Read the eventid counter corresponding to rmid
> >   *			      for this resource and domain.
> > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> > index 3b09cfe9a046..017a19143ec9 100644
> > --- a/arch/x86/kernel/cpu/resctrl/internal.h
> > +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> > @@ -234,15 +234,15 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
> >  void resctrl_arch_mbm_cntr_assign_set_one(struct rdt_resource *r);
> >  
> >  #ifdef CONFIG_X86_CPU_RESCTRL_INTEL_AET
> > -bool intel_aet_get_events(void);
> > -void __exit intel_aet_exit(void);
> > +bool intel_aet_pre_mount(void);
> > +void intel_aet_unmount(void);
> >  int intel_aet_read_event(int domid, u32 rmid, void *arch_priv, u64 *val);
> >  void intel_aet_mon_domain_setup(int cpu, int id, struct rdt_resource *r,
> >  				struct list_head *add_pos);
> >  bool intel_handle_aet_option(bool force_off, char *tok);
> >  #else
> > -static inline bool intel_aet_get_events(void) { return false; }
> > -static inline void __exit intel_aet_exit(void) { }
> > +static inline bool intel_aet_pre_mount(void) { return false; }
> > +static inline void intel_aet_unmount(void) { }
> >  static inline int intel_aet_read_event(int domid, u32 rmid, void *arch_priv, u64 *val)
> >  {
> >  	return -EINVAL;
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index 934492c7e643..9336299b9647 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -16,10 +16,12 @@
> >  
> >  #define pr_fmt(fmt)	"resctrl: " fmt
> >  
> > +#include <linux/cleanup.h>
> >  #include <linux/cpu.h>
> >  #include <linux/slab.h>
> >  #include <linux/err.h>
> >  #include <linux/cpuhotplug.h>
> > +#include <linux/mutex.h>
> >  
> >  #include <asm/cpu_device_id.h>
> >  #include <asm/msr.h>
> > @@ -776,12 +778,20 @@ static int resctrl_arch_offline_cpu(unsigned int cpu)
> >  	return 0;
> >  }
> >  
> > +static DEFINE_MUTEX(resctrl_arch_mount_lock);
> 
> Please document what this mutex protects.

It looks like I'm only protecting resctrl_arch_mount_entries, but I hold
the mutex for much longer. Perhaps this could just be some atomic
operations on resctrl_arch_mount_entries? I'll think about this some
more.

> 
> > +static int resctrl_arch_mount_entries;
> > +
> >  void resctrl_arch_pre_mount(void)
> >  {
> >  	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_PERF_PKG].r_resctrl;
> >  	int cpu;
> >  
> > -	if (!intel_aet_get_events())
> > +	guard(mutex)(&resctrl_arch_mount_lock);
> > +
> > +	if (++resctrl_arch_mount_entries > 1)
> > +		return;
> > +
> > +	if (!intel_aet_pre_mount())
> >  		return;
> >  
> >  	/*
> > @@ -798,6 +808,33 @@ void resctrl_arch_pre_mount(void)
> >  	cpus_read_unlock();
> >  }
> >  
> > +void resctrl_arch_unmount(void)
> > +{
> > +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_PERF_PKG].r_resctrl;
> > +	int cpu;
> > +
> > +	guard(mutex)(&resctrl_arch_mount_lock);
> > +
> > +	if (--resctrl_arch_mount_entries > 0)
> > +		return;
> > +
> > +	WARN_ON(resctrl_arch_mount_entries < 0);
> > +
> > +	intel_aet_unmount();
> > +
> > +	if (!r->mon_capable)
> > +		return;
> > +
> > +	cpus_read_lock();
> > +	mutex_lock(&domain_list_lock);
> > +	for_each_online_cpu(cpu)
> > +		domain_remove_cpu_mon(cpu, r);
> > +	r->mon_capable = false;
> > +	rdt_mon_feature_count--;
> > +	mutex_unlock(&domain_list_lock);
> > +	cpus_read_unlock();
> > +}
> > +
> >  enum {
> >  	RDT_FLAG_CMT,
> >  	RDT_FLAG_MBM_TOTAL,
> > @@ -1174,8 +1211,6 @@ late_initcall(resctrl_arch_late_init);
> >  
> >  static void __exit resctrl_arch_exit(void)
> >  {
> > -	intel_aet_exit();
> > -
> >  	cpuhp_remove_state(rdt_online);
> >  
> >  	resctrl_exit();
> > diff --git a/arch/x86/kernel/cpu/resctrl/intel_aet.c b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> > index db671725acbb..74c34593876b 100644
> > --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> > +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> > @@ -25,8 +25,8 @@
> >  #include <linux/intel_vsec.h>
> >  #include <linux/io.h>
> >  #include <linux/minmax.h>
> > -#include <linux/mutex.h>
> >  #include <linux/module.h>
> > +#include <linux/mutex.h>
> >  #include <linux/printk.h>
> >  #include <linux/rculist.h>
> >  #include <linux/rcupdate.h>
> > @@ -308,7 +308,7 @@ static void (*put_feature)(struct pmt_feature_group *p);
> >   * struct pmt_feature_group to indicate that its events are successfully
> >   * enabled.
> >   */
> > -bool intel_aet_get_events(void)
> > +static bool aet_get_events(void)
> >  {
> >  	struct pmt_feature_group *p;
> >  	enum pmt_feature_id pfid;
> > @@ -317,14 +317,14 @@ bool intel_aet_get_events(void)
> >  
> >  	for_each_event_group(peg) {
> >  		pfid = lookup_pfid((*peg)->pfname);
> > -		p = intel_pmt_get_regions_by_feature(pfid);
> > +		p = get_feature(pfid);
> >  		if (IS_ERR_OR_NULL(p))
> >  			continue;
> >  		if (enable_events(*peg, p)) {
> >  			(*peg)->pfg = p;
> >  			ret = true;
> >  		} else {
> > -			intel_pmt_put_feature_group(p);
> > +			put_feature(p);
> >  		}
> >  	}
> >  
> > @@ -353,16 +353,56 @@ void intel_aet_unregister_enumeration(void)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(intel_aet_unregister_enumeration, "INTEL_PMT");
> >  
> > -void __exit intel_aet_exit(void)
> > +/*
> > + * The pmt_telemetry module may not be loaded when the resctrl file system
> 
> Is this accurate? What prevents pmt_telemetry module from being loaded when
> resctrl file system is mounted?

Comment isn't clear. The issue is when pmt_telemetry isn't loaded during
the mount operation, but is loaded later while the file system is
mounted. On unmount "pmt_module" is set, but should not get a
module_put() because no "get" was done at mount time.

I'll rewrite to make this clear.
> 
> > + * is mounted. Keep track of whether a hold was placed on the module to know
> > + * whether to release it during unmount.
> > + */
> > +static bool have_pmt_hold;
> > +
> > +bool intel_aet_pre_mount(void)
> > +{
> > +	bool ret;
> > +
> > +	guard(mutex)(&aet_register_lock);
> > +	if (!get_feature || !put_feature)
> > +		return false;
> > +
> > +	if (pmt_module) {
> > +		if (!try_module_get(pmt_module))
> > +			return false;
> > +		have_pmt_hold = true;
> > +	}
> > +
> > +	ret = aet_get_events();
> > +
> > +	if (!ret && pmt_module && have_pmt_hold) {
> > +		module_put(pmt_module);
> > +		have_pmt_hold = false;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +void intel_aet_unmount(void)
> >  {
> >  	struct event_group **peg;
> >  
> > +	guard(mutex)(&aet_register_lock);
> 
> Could this get a short-circuit to make behavior on AMD obvious?

Like this?

	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
		return;

Same in intel_aet_pre_mount() for symmetry??

> 
> >  	for_each_event_group(peg) {
> >  		if ((*peg)->pfg) {
> > -			intel_pmt_put_feature_group((*peg)->pfg);
> > +			struct event_group *e = *peg;
> > +
> > +			for (int j = 0; j < e->num_events; j++)
> > +				resctrl_disable_mon_event(e->evts[j].id);
> > +			put_feature((*peg)->pfg);
> >  			(*peg)->pfg = NULL;
> >  		}
> >  	}
> > +	if (have_pmt_hold && pmt_module) {
> > +		module_put(pmt_module);
> > +		have_pmt_hold = false;
> > +	}
> >  }
> >  
> >  #define DATA_VALID	BIT_ULL(63)
> > diff --git a/drivers/resctrl/mpam_resctrl.c b/drivers/resctrl/mpam_resctrl.c
> > index 7079870ca894..5859cc0f5e37 100644
> > --- a/drivers/resctrl/mpam_resctrl.c
> > +++ b/drivers/resctrl/mpam_resctrl.c
> > @@ -162,6 +162,10 @@ void resctrl_arch_pre_mount(void)
> >  {
> >  }
> >  
> > +void resctrl_arch_unmount(void)
> > +{
> > +}
> > +
> >  bool resctrl_arch_get_cdp_enabled(enum resctrl_res_level rid)
> >  {
> >  	return mpam_resctrl_controls[rid].cdp_enabled;
> 
> Reinette

-Tony