Re: [PATCH v10 12/17] x86/resctrl: Prepare to handle nested mount requests

Hi Tony,

On 7/29/26 10:27 AM, Tony Luck wrote:
> There is no upper level serialization of mount(2) system calls.
> 
> mount/unmount operations can happen in parallel with CPU hotplug events
> that need to add/remove files and directories when domains are added or
> removed.
> 
> Use cpus_read_lock() plus mutex_lock(&domain_list_lock) to protect
> architecture code from races.

Please describe how these locks are used to achieve this protection (more below).

> 
> Signed-off-by: Tony Luck <[email protected]>
> ---
> v10:
> 	New patch
> 
>  include/linux/resctrl.h            |  2 +-
>  arch/x86/kernel/cpu/resctrl/core.c | 40 ++++++++++++++++++++++++------
>  drivers/resctrl/mpam_resctrl.c     |  3 ++-
>  3 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 568a650c0224..65245f2fdad0 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -579,7 +579,7 @@ void resctrl_offline_cpu(unsigned int cpu);
>   * Architecture hook called at beginning of first file system mount attempt.
>   * No locks are held.
>   */
> -void resctrl_arch_pre_mount(void);
> +int resctrl_arch_pre_mount(void);

There is no mention in changelog why this change is needed.

>  
>  /*
>   * Architecture hook called when mount fails, or on unmount.
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index e335a143f3e5..906aa4dfc363 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -16,7 +16,6 @@
>  
>  #define pr_fmt(fmt)	"resctrl: " fmt
>  
> -#include <linux/cleanup.h>
>  #include <linux/cpu.h>
>  #include <linux/slab.h>
>  #include <linux/err.h>
> @@ -807,25 +806,47 @@ static int resctrl_arch_offline_cpu(unsigned int cpu)
>  	return 0;
>  }
>  
> -void resctrl_arch_pre_mount(void)
> +/*
> + * Linux provides no synchronization for mount(2) system calls.
> + * resctrl_arch_pre_mount() and resctrl_arch_unmount() are called
> + * with no locks held. Mount/unmount may also race with CPU hotplug
> + * events that add/remove per-domain files and directories.
> + *
> + * Use cpus_read_lock() plus domain_list_lock to protect operations.

"protect operations"? Which operations? I expect it to protect concurrent
changes to data expected to be protected by the lock. domain_list_lock is
understood to protect the architecture managed domain list. 
It looks to me as though what this actually does is add new architecture
state, arch_mounted, and let this new state fall under protection of
domain_list_lock to protect it from concurrent changes. The reason why
cpus_read_lock() is needed is the locking order requires it.

I find "protect operations" to be too vague description to use as motivation
and documentation of locking behavior.

For completeness I expect arch_mounted to be added to the comments
above the original definition of domain_list_lock to make it clear that the
lock now has expanded scope.

Reinette