Re: [PATCH v9 09/12] arm,x86/resctrl: Resolve INTEL_PMT_TELEMETRY symbols at runtime

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

Hi Tony,

On 7/1/26 2:35 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 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.

intel_aet_exit -> intel_aet_exit()

At first glance this change does not seem symmetrical since it introduces
the new enumeration and cleanup calls but does not call the cleanup call that
appears as though it keeps a reference to the module. I think it will help to
highlight that the previous exit code was not called anyway and at this point
the module is still required to be built in so there is no actual reference taken.

> 
> 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.
> 
> Use "pmt_in_use" to detect nested mount attempts.
> 
> 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.

If event_group::num_rmid gets a mention, should rdt_resource::resctrl_mon::num_rmid also?

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> --
> v9:
> 	Drop broken atomic ops introduced in v8
> 	Check pmt_in_use in intel_aet_pre_mount() to detect nested
> 	mount flow.
> ---
>  include/linux/resctrl.h                 |  6 +++
>  arch/x86/kernel/cpu/resctrl/internal.h  | 23 ++++++++--
>  arch/x86/kernel/cpu/resctrl/core.c      | 25 +++++++++--
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 58 +++++++++++++++++++++----
>  drivers/resctrl/mpam_resctrl.c          |  4 ++
>  5 files changed, 101 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index e1501ee72d28..4dc41dc99dc2 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -564,6 +564,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 e3cfa0c10e92..a891017f69ec 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;
> @@ -253,4 +253,19 @@ static inline void intel_aet_mon_domain_setup(int cpu, int id, struct rdt_resour
>  static inline bool intel_handle_aet_option(bool force_off, char *tok) { return false; }
>  #endif
>  
> +#ifdef CONFIG_INTEL_PMT_TELEMETRY_MODULE
> +static inline bool intel_aet_try_module_get(struct module *mod)
> +{
> +	return try_module_get(mod);
> +}
> +
> +static inline void intel_aet_module_put(struct module *mod)
> +{
> +	module_put(mod);
> +}
> +#else
> +static inline bool intel_aet_try_module_get(struct module *mod) { return true; }
> +static inline void intel_aet_module_put(struct module *mod) { }
> +#endif


Trying to look back to how the above ended up like this I find:
https://lore.kernel.org/lkml/SJ1PR11MB60838C247727AE7CC324BB25FC1A2@SJ1PR11MB6083.namprd11.prod.outlook.com/
that states:
	pmt_module is NULL if CONFIG_INTEL_PMT_TELEMETRY=y ... i.e. built-in to the kernel.

	In that case it obviously can't go away, and doesn't need module_get()/module_put().
	There's no special case for this. try_module_get() takes a fault on NULL dereference.
	...

Taking a closer look at try_module_get() I do not see it attempting to dereference
a NULL parameter but instead it returns true in this case which makes the above unnecessary?
What am I missing?

> +
>  #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 3169441a2d40..3ef620d8e0c8 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/cpuid/api.h>
> @@ -798,7 +800,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;
>  
>  	/*
> @@ -814,6 +816,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;
> +
> +	intel_aet_unmount();
> +
> +	if (!r->mon_capable)
> +		return;

Could the mon_capable check be moved before 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,
> @@ -1187,8 +1208,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 c26bd813e057..c2e27ce52248 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>

This can be squashed with earlier patch.

> @@ -310,7 +310,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;
> @@ -319,14 +319,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);
>  		}
>  	}
>  
> @@ -359,16 +359,58 @@ void intel_aet_unregister_enumeration(void)
>  }
>  EXPORT_SYMBOL_NS_GPL(intel_aet_unregister_enumeration, "INTEL_PMT");
>  
> -void __exit intel_aet_exit(void)
> +/*
> + * Track whether pmt_telemetry enumeration succeeded during mount to
> + * detect nested mount attempts and for use during unmount.
> + */
> +static bool pmt_in_use;
> +
> +bool intel_aet_pre_mount(void)
> +{
> +	bool ret;
> +
> +	guard(mutex)(&aet_register_lock);
> +
> +	/* Nested mount attempt. File system code will return -EBUSY */
> +	if (pmt_in_use)
> +		return false;
> +
> +	if (!get_feature || !put_feature)
> +		return false;
> +
> +	if (!intel_aet_try_module_get(pmt_module))
> +		return false;
> +
> +	ret = aet_get_events();
> +
> +	if (!ret)
> +		intel_aet_module_put(pmt_module);
> +	else
> +		pmt_in_use = true;
> +
> +	return ret;
> +}
> +
> +void intel_aet_unmount(void)
>  {
>  	struct event_group **peg;
>  
> +	guard(mutex)(&aet_register_lock);
> +	if (!pmt_in_use)
> +		return;
> +
>  	for_each_event_group(peg) {
> -		if ((*peg)->pfg) {
> -			intel_pmt_put_feature_group((*peg)->pfg);
> -			(*peg)->pfg = NULL;
> +		struct event_group *e = *peg;
> +
> +		if (e->pfg) {
> +			for (int j = 0; j < e->num_events; j++)
> +				resctrl_disable_mon_event(e->evts[j].id);
> +			put_feature(e->pfg);
> +			e->pfg = NULL;
>  		}
>  	}
> +	intel_aet_module_put(pmt_module);
> +	pmt_in_use = 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