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

[Luck, Tony]

Hi Reinette,

On Fri, Apr 03, 2026 at 05:52:30PM -0700, Reinette Chatre wrote:
> 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.

OK. I will expand.

> > 
> > Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> > ---
> > 
> > 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?

I'll look again because several things have changed since I ordered
the series this way. But some bits got overly complicated trying to
make AET ready to be called multiple times. If I can't solve elegantly
I'll move this into the proper changelog.

> > 
> >  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()?

You are right. If the mount fails in the resctrl code, resctrl_arch_unmount()
can clean things up.

> > +
> > +/* 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?

I'll move to the next patch. It's needed for the case when AET works on some
mount and then fails on a subsequent one. But that can't happen at this point
in this patch series.

> >  		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?

If you dig in lore for the v3 of this patch, you'll see I had the mutex in the
AET code. But there were some complications.

1) Need to acquire in intel_aet_pre_mount() and release in intel_aet_mount_result()
which is legal, but makes code more complex when call chains need to be compared to
check that the mutex is being released correctly.

2) The "only mounted once" case meant extra state (AET_PRESENT, which you note
in next patch may be redundant) because intel_aet_pre_mount() is called, but
needs to do nothing.

Adding resctrl_mount_lock to the file system code made things simpler. The
pre-mount code can't be called with rdtgroup_mutex held because it needs to
build the domains. That needs cpus_read_lock() + mutex_lock(&domain_list_lock);

I need to add more comments on locking. resctrl_mounted is only modified when both
resctrl_mount_lock AND rdtgroup_mutex are held. I believe that makes it safe to
read the value of resctrl_mounted with just rdtgroup_mutex held.

> > +	resctrl_arch_pre_mount();
> > +
> > +	ret = rdt_get_tree(fc);
> > +
> > +	resctrl_arch_mount_result(ret);
> 
> Could this instead just call resctrl_arch_unmount() on failure?

Yes. Thanks for the suggestion.

> Reinette

-Tony