Re: [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl

[Dave Martin <Dave.Martin@arm.com>]

On Tue, Jun 16, 2020 at 06:19:27PM +0100, Will Deacon wrote:
> On Tue, Jun 16, 2020 at 05:17:04PM +0100, Dave Martin wrote:
> > On Tue, Jun 16, 2020 at 04:04:51PM +0100, Will Deacon wrote:
> > > On Tue, Jun 16, 2020 at 09:18:08AM -0400, Qian Cai wrote:
> > > > On Wed, Jun 10, 2020 at 06:03:10PM +0100, Dave Martin wrote:
> > > > > sve_default_vl can be modified via the /proc/sys/abi/sve_default_vl
> > > > > sysctl concurrently with use, and modified concurrently by multiple
> > > > > threads.
> > > > > 
> > > > > Adding a lock for this seems overkill, and I don't want to think any
> > > > > more than necessary, so just define wrappers using READ_ONCE()/
> > > > > WRITE_ONCE().
> > > > > 
> > > > > This will avoid the possibility of torn accesses and repeated loads
> > > > > and stores.
> > > > > 
> > > > > There's no evidence yet that this is going wrong in practice: this
> > > > > is just hygiene.  For generic sysctl users, it would be better to
> > > > > build this kind of thing into the sysctl common code somehow.
> > > > > 
> > > > > Reported-by: Will Deacon <will@kernel.org>
> > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > 
> > > > While this original patch looks correct, linux-next has this,
> > > > 
> > > > [will: move set_sve_default_vl() inside #ifdef to squash allnoconfig warning]
> > > > 
> > > > 1e570f512cbd ("arm64/sve: Eliminate data races on sve_default_vl")
> > > > 
> > > > which causes an error with CONFIG_ARM64_SVE=n,
> > > > 
> > > > This .config,
> > > > https://raw.githubusercontent.com/cailca/linux-mm/master/arm64.config
> > > > 
> > > > arch/arm64/kernel/fpsimd.c: In function ‘sve_proc_do_default_vl’:
> > > > arch/arm64/kernel/fpsimd.c:375:2: error: implicit declaration of
> > > > function ‘set_sve_default_vl’; did you mean ‘get_sve_default_vl’?
> > > > [-Werror=implicit-function-declaration]
> > > >   set_sve_default_vl(find_supported_vector_length(vl));
> > > >   ^~~~~~~~~~~~~~~~~~
> > > >   get_sve_default_vl
> > > 
> > > Thanks, I'll take a look.
> > 
> > I haven't looked in detail at this; I guess the new helpers just
> > need to be manually placed in the right #ifdef block.
> 
> That was what I did when I merged the patch, but that broke configurations
> where SYSCTL is enabled but SVE is disabled. I've ended up with this
> diff on top of for-next/fixes.
> 
> Will
> 
> --->8
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index d9eee9194511..55c8f3ec6705 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -349,7 +349,7 @@ static unsigned int find_supported_vector_length(unsigned int vl)
>  	return sve_vl_from_vq(__bit_to_vq(bit));
>  }
>  
> -#ifdef CONFIG_SYSCTL
> +#if defined(CONFIG_ARM64_SVE) && defined(CONFIG_SYSCTL)
>  
>  static int sve_proc_do_default_vl(struct ctl_table *table, int write,
>  				  void *buffer, size_t *lenp, loff_t *ppos)
> @@ -394,9 +394,9 @@ static int __init sve_sysctl_init(void)
>  	return 0;
>  }
>  
> -#else /* ! CONFIG_SYSCTL */
> +#else /* ! (CONFIG_ARM64_SVE && CONFIG_SYSCTL) */
>  static int __init sve_sysctl_init(void) { return 0; }
> -#endif /* ! CONFIG_SYSCTL */
> +#endif /* ! (CONFIG_ARM64_SVE && CONFIG_SYSCTL) */

Hmm, I guess that works, but it still seems cumbersome.  #ifdefs do
tend to breed as the code gets extended, so I'd worked hard to
eliminate them as much as possible.

Can't we simply leave the helpers outside the #ifdef, and do this?

 /* Default VL for tasks that don't set it explicitly: */
 static int __sve_default_vl = -1;
 
-static int get_sve_default_vl(void)
+static inline int get_sve_default_vl(void)
 {
 	return READ_ONCE(__sve_default_vl);
 }
 
-static void set_sve_default_vl(int val)
+static inline void set_sve_default_vl(int val)
 {
 	WRITE_ONCE(__sve_default_vl, val);
 }


Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel