Re: [PATCH v4 4/7] fs,x86/resctrl: Add architecture hooks for every mount/unmount

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

Hi Tony,

On 3/30/26 2:43 PM, Tony Luck wrote:
> Add hooks for every mount/unmount of the resctrl file system so that
> architecture code can allocate on mount and free on unmount.

Please use the changelog to describe and motivate all the other things
that this patch does.

> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> 
> Note this patch disables enumeration of AET monitor events because the
> new mount/unmount hooks do not call intel_aet_get_events() (which is
> not ready for the change from "just on first mount" to "called on
> every mount"). That is resolved in the next patch.

This could be part of the proper changelog.

Could patches be re-ordered to support incremental changes?

> 
>  include/linux/resctrl.h                 | 12 +++++--
>  arch/x86/kernel/cpu/resctrl/internal.h  |  8 +++--
>  arch/x86/kernel/cpu/resctrl/core.c      | 14 +++++++-
>  arch/x86/kernel/cpu/resctrl/intel_aet.c | 13 ++++++++
>  fs/resctrl/rdtgroup.c                   | 43 ++++++++++++++++++-------
>  5 files changed, 74 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index b312aaf76974..489c7d4ae3e9 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -518,11 +518,19 @@ void resctrl_online_cpu(unsigned int cpu);
>  void resctrl_offline_cpu(unsigned int cpu);
>  
>  /*
> - * Architecture hook called at beginning of first file system mount attempt.
> - * No locks are held.
> + * Architecture hooks for resctrl mount/unmount.
> + * Each is called with resctrl_mount_lock held.
>   */
> +
> +/* Called at beginning of each file system mount attempt. */
>  void resctrl_arch_pre_mount(void);
>  
> +/* Called to report success/failure of mount. */
> +void resctrl_arch_mount_result(int ret);

Why is this needed? Why not just always call resctrl_arch_unmount()?

> +
> +/* 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 e3cfa0c10e92..6f322818a9e6 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -234,14 +234,18 @@ 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);
> +bool intel_aet_pre_mount(void);
> +void intel_aet_mount_result(int ret);
> +void intel_aet_unmount(void);
>  void __exit intel_aet_exit(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 bool intel_aet_pre_mount(void) { return false; }
> +static inline void intel_aet_mount_result(int ret) { }
> +static inline void intel_aet_unmount(void) { }
>  static inline void __exit intel_aet_exit(void) { }
>  static inline int intel_aet_read_event(int domid, u32 rmid, void *arch_priv, u64 *val)
>  {
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 7667cf7c4e94..162eca2cfcdb 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -769,8 +769,10 @@ 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()) {
> +		r->mon_capable = false;

Why is this needed?

>  		return;
> +	}
>  
>  	/*
>  	 * Late discovery of telemetry events means the domains for the


...

>  #include "internal.h"
>  
> +/* Mutex protecting mount/unmount operations */
> +static DEFINE_MUTEX(resctrl_mount_lock);
> +
>  /* Mutex to protect rdtgroup access. */
>  DEFINE_MUTEX(rdtgroup_mutex);
>  
> @@ -2788,17 +2790,8 @@ static int rdt_get_tree(struct fs_context *fc)
>  	struct rdt_resource *r;
>  	int ret;
>  
> -	DO_ONCE_SLEEPABLE(resctrl_arch_pre_mount);
> -
>  	cpus_read_lock();
>  	mutex_lock(&rdtgroup_mutex);
> -	/*
> -	 * resctrl file system can only be mounted once.
> -	 */
> -	if (resctrl_mounted) {
> -		ret = -EBUSY;
> -		goto out;
> -	}
>  
>  	ret = setup_rmid_lru_list();
>  	if (ret)
> @@ -2900,6 +2893,30 @@ static int rdt_get_tree(struct fs_context *fc)
>  	return ret;
>  }
>  
> +static int rdt_get_tree_wrapper(struct fs_context *fc)
> +{
> +	int ret;
> +
> +	mutex_lock(&resctrl_mount_lock);
> +
> +	/*
> +	 * resctrl file system can only be mounted once.
> +	 */
> +	if (resctrl_mounted) {
> +		mutex_unlock(&resctrl_mount_lock);
> +		return -EBUSY;
> +	}
> +

This does not look right. Here too is resctrl_mounted accessed without rdtgroup_mutex
held. This change implies that resctrl_mounted is now protected by resctrl_mount_lock
but resctrl is not changed to respect this throughout resulting in unsafe access of
resctrl_mounted.

Does this new resctrl_mount_lock need to be in resctrl fs? It really seems as though the
needed synchronization belongs in the architecture. Could this instead be accomplished
with a private mutex within the AET code?

> +	resctrl_arch_pre_mount();
> +
> +	ret = rdt_get_tree(fc);
> +
> +	resctrl_arch_mount_result(ret);

Could this instead just call resctrl_arch_unmount() on failure?

Reinette