Re: [PATCH v11 17/23] arm,x86/resctrl: Resolve INTEL_PMT_TELEMETRY symbols at runtime

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

Hi Tony,

On 8/31/26 10:44 AM, 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 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.
> 
> intel_aet_exit() was never called because resctrl is built into the kernel. All
> cleanup is now handled in the unmount path. Remove intel_aet_exit().
> 
> 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.
> 
> Zero rdt_resource::resctrl_mon::num_rmid for RDT_RESOURCE_PERF_PKG so
> that it will be re-computed next mount.
> 
> 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.
> 
> Place a hold on the pmt_telemetry while enumerating AET events during

"on the pmt_telemetry" -> "on the pmt_telemetry module"?

> pre-mount processing. Release the hold if no events were enabled.
> Otherwise keep the hold until the resctrl file system is unmounted.

Same comment as previous version: this "hold" is asymmetric in this patch since
the unmount code is introduced here, but never called. Could you please describe
in changelog why this is ok?

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> v11:
> 	Drop unneeded include of <linux/cleanup.h> from core.c
> 	Comment on data protected by  aet_register_lock
> 	Add commit comment on module{get,put} activity
> 
>  include/linux/resctrl.h                 |  6 +++

This change implies fs/resctrl is needed in subject prefix also.

>  arch/x86/kernel/cpu/resctrl/internal.h  |  8 ++--
>  arch/x86/kernel/cpu/resctrl/core.c      | 24 ++++++++--
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 62 ++++++++++++++++++++++---
>  drivers/resctrl/mpam_resctrl.c          |  4 ++
>  5 files changed, 90 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 4fb06d434c85..0d7fd294b760 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -591,6 +591,12 @@ void resctrl_offline_cpu(unsigned int cpu);
>   */
>  void resctrl_arch_pre_mount(void);
>  
> +/*
> + * Architecture hook called when mount fails, or on unmount.
> + * No locks are held.
> + */
> +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 27dc27480f6e..4409ee20474d 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -234,17 +234,17 @@ 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 __init intel_aet_init(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 intel_aet_init(void) { }
> -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 8b763cf638ef..cdcc5611345f 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -20,6 +20,7 @@
>  #include <linux/slab.h>
>  #include <linux/err.h>
>  #include <linux/cpuhotplug.h>
> +#include <linux/mutex.h>
>  
>  #include <asm/cpu_device_id.h>
>  #include <asm/cpuid/api.h>
> @@ -800,7 +801,7 @@ 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())
> +	if (!intel_aet_pre_mount())
>  		return;
>  
>  	/*
> @@ -816,6 +817,25 @@ 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;
> +
> +	if (!r->mon_capable)
> +		return;

Since resctrl_arch_unmount() is introduced but not called it is difficult to reason about
the safety of r->mon_capable accessed here without, what appears to be, any locks held. 

> +
> +	intel_aet_unmount();
> +
> +	cpus_read_lock();
> +	mutex_lock(&domain_list_lock);
> +	for_each_online_cpu(cpu)
> +		domain_remove_cpu_mon(cpu, r);
> +	r->mon_capable = false;
> +	mutex_unlock(&domain_list_lock);
> +	cpus_read_unlock();
> +}
> +
>  enum {
>  	RDT_FLAG_CMT,
>  	RDT_FLAG_MBM_TOTAL,
> @@ -1160,8 +1180,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 f5007bea8346..c3bd3536c514 100644
> --- a/arch/x86/kernel/cpu/resctrl/intel_aet.c
> +++ b/arch/x86/kernel/cpu/resctrl/intel_aet.c
> @@ -12,6 +12,7 @@
>  #define pr_fmt(fmt)   "resctrl: " fmt
>  
>  #include <linux/bits.h>
> +#include <linux/cleanup.h>
>  #include <linux/compiler_types.h>
>  #include <linux/container_of.h>
>  #include <linux/cpumask.h>
> @@ -25,6 +26,7 @@
>  #include <linux/io.h>
>  #include <linux/minmax.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/printk.h>
>  #include <linux/rculist.h>
>  #include <linux/rcupdate.h>
> @@ -293,10 +295,22 @@ static enum pmt_feature_id lookup_pfid(const char *pfname)
>  	return FEATURE_INVALID;
>  }
>  
> +/*
> + * Protects pmt_module, get_feature, put_feature against races between module
> + * load/unload of the pmt_telemetry module and mount/unmount of the resctrl
> + * file system. Also protects pmt_in_use.

Thank you for adding this. This accurately reflects what aet_register_lock protects
in *this* patch. aet_register_lock protects more as this series progresses from here,
could you please update this text as this mutex protects more and more?

> + */
> +static DEFINE_MUTEX(aet_register_lock);
> +
>  static struct module *pmt_module;
>  static struct pmt_feature_group *(*get_feature)(enum pmt_feature_id id);
>  static void (*put_feature)(struct pmt_feature_group *p);
>  
Reinette