diff mbox series

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

Message ID 1591808590-20210-3-git-send-email-Dave.Martin@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64/sve: Misc fixes | expand

Commit Message

Dave Martin June 10, 2020, 5:03 p.m. UTC 
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>
---

Build-tested only, but it seems pretty straightforward.

 arch/arm64/kernel/fpsimd.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

Comments

Qian Cai June 16, 2020, 1:18 p.m. UTC | #1 
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

> ---
> 
> Build-tested only, but it seems pretty straightforward.
> 
>  arch/arm64/kernel/fpsimd.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 94289d1..19865c9 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -12,6 +12,7 @@
>  #include <linux/bug.h>
>  #include <linux/cache.h>
>  #include <linux/compat.h>
> +#include <linux/compiler.h>
>  #include <linux/cpu.h>
>  #include <linux/cpu_pm.h>
>  #include <linux/kernel.h>
> @@ -119,7 +120,17 @@ struct fpsimd_last_state_struct {
>  static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state);
>  
>  /* Default VL for tasks that don't set it explicitly: */
> -static int sve_default_vl = -1;
> +static int __sve_default_vl = -1;
> +
> +static int get_sve_default_vl(void)
> +{
> +	return READ_ONCE(__sve_default_vl);
> +}
> +
> +static void set_sve_default_vl(int val)
> +{
> +	WRITE_ONCE(__sve_default_vl, val);
> +}
>  
>  #ifdef CONFIG_ARM64_SVE
>  
> @@ -345,7 +356,7 @@ static int sve_proc_do_default_vl(struct ctl_table *table, int write,
>  				  loff_t *ppos)
>  {
>  	int ret;
> -	int vl = sve_default_vl;
> +	int vl = get_sve_default_vl();
>  	struct ctl_table tmp_table = {
>  		.data = &vl,
>  		.maxlen = sizeof(vl),
> @@ -362,7 +373,7 @@ static int sve_proc_do_default_vl(struct ctl_table *table, int write,
>  	if (!sve_vl_valid(vl))
>  		return -EINVAL;
>  
> -	sve_default_vl = find_supported_vector_length(vl);
> +	set_sve_default_vl(find_supported_vector_length(vl));
>  	return 0;
>  }
>  
> @@ -869,7 +880,7 @@ void __init sve_setup(void)
>  	 * For the default VL, pick the maximum supported value <= 64.
>  	 * VL == 64 is guaranteed not to grow the signal frame.
>  	 */
> -	sve_default_vl = find_supported_vector_length(64);
> +	set_sve_default_vl(find_supported_vector_length(64));
>  
>  	bitmap_andnot(tmp_map, sve_vq_partial_map, sve_vq_map,
>  		      SVE_VQ_MAX);
> @@ -890,7 +901,7 @@ void __init sve_setup(void)
>  	pr_info("SVE: maximum available vector length %u bytes per vector\n",
>  		sve_max_vl);
>  	pr_info("SVE: default vector length %u bytes per vector\n",
> -		sve_default_vl);
> +		get_sve_default_vl());
>  
>  	/* KVM decides whether to support mismatched systems. Just warn here: */
>  	if (sve_max_virtualisable_vl < sve_max_vl)
> @@ -1030,13 +1041,13 @@ void fpsimd_flush_thread(void)
>  		 * vector length configured: no kernel task can become a user
>  		 * task without an exec and hence a call to this function.
>  		 * By the time the first call to this function is made, all
> -		 * early hardware probing is complete, so sve_default_vl
> +		 * early hardware probing is complete, so __sve_default_vl
>  		 * should be valid.
>  		 * If a bug causes this to go wrong, we make some noise and
>  		 * try to fudge thread.sve_vl to a safe value here.
>  		 */
>  		vl = current->thread.sve_vl_onexec ?
> -			current->thread.sve_vl_onexec : sve_default_vl;
> +			current->thread.sve_vl_onexec : get_sve_default_vl();
>  
>  		if (WARN_ON(!sve_vl_valid(vl)))
>  			vl = SVE_VL_MIN;
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Will Deacon June 16, 2020, 3:04 p.m. UTC | #2 
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.

Will
Dave Martin June 16, 2020, 4:17 p.m. UTC | #3 
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.

Cheers
---Dave
Will Deacon June 16, 2020, 5:19 p.m. UTC | #4 
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) */
 
 #define ZREG(sve_state, vq, n) ((char *)(sve_state) +		\
 	(SVE_SIG_ZREG_OFFSET(vq, n) - SVE_SIG_REGS_OFFSET))
Dave Martin June 17, 2020, 9:40 a.m. UTC | #5 
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
Will Deacon June 17, 2020, 10:08 a.m. UTC | #6 
On Wed, Jun 17, 2020 at 10:40:54AM +0100, Dave Martin wrote:
> On Tue, Jun 16, 2020 at 06:19:27PM +0100, Will Deacon wrote:
> > 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.

This is just extending an existing #ifdef though, and I don't think it
makes any sense to compile in the SVE sysctl logic if SVE is not enabled.
If CONFIG_SYSCTL didn't exist, this code would almost certainly be inside
a CONFIG_SVE block anyway.

> 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);
>  }

That would work too, although I'd be wary of somebody removing the inline
later on because "the compiler knows best about inlining decisions". I'd
also say that calling set_sve_default_vl() is an error if CONFIG_SVE is
not defined as we really want get_sve_default_vl() to return -1
unconditionally in that case. Having set_sve_default_vl() inside the
#ifdef ensures that.

I don't care too strongly either way, but I already queued my diff last
night [1] in order to fix linux-next, so I'd prefer not to drop it unless
there's a functional reason to do so.

Will

[1] https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?h=for-next/fixes&id=e575fb9e76c8e33440fb859572a8b7d430f053d6
Dave Martin June 17, 2020, 11:06 a.m. UTC | #7 
On Wed, Jun 17, 2020 at 11:08:32AM +0100, Will Deacon wrote:
> On Wed, Jun 17, 2020 at 10:40:54AM +0100, Dave Martin wrote:
> > On Tue, Jun 16, 2020 at 06:19:27PM +0100, Will Deacon wrote:
> > > 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.
> 
> This is just extending an existing #ifdef though, and I don't think it
> makes any sense to compile in the SVE sysctl logic if SVE is not enabled.
> If CONFIG_SYSCTL didn't exist, this code would almost certainly be inside
> a CONFIG_SVE block anyway.

Only code that's unreachable from inside the translation unit needs to
be #ifdeffed.  For the rest, the compiler knows how to determine what's
used (indeed, it's better at it than humans).

Originally I relied on #ifdefs more, but I needed a lot of them, and it
was hell to rebase every time anything needed to be moved around.

Currently I don't see anything that gets compiled in if CONFIG_SYSCTL=n.
Other than what was already compiled before this patch.  We still need
to track the default vl, because it depends on the hardware; however it's
effectively ro-after-init if CONFIG_SYSCTL=n.

I think that complicating the #ifdef conditions in this file is a
slippery slope, but I guess it's the it's up to the maintainer whether
to care about that.

Am I missing something?

> > 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);
> >  }
> 
> That would work too, although I'd be wary of somebody removing the inline
> later on because "the compiler knows best about inlining decisions". I'd

AFAIK inline is widely used for static functions in headers for precisely
this reason.  I have tried to use __maybe_unused (or even #ifdefs) in
the past to be more explicit, but got shouted at.  We could optionally
use __maybe_unused here if you think that's more self-explanatory.

> also say that calling set_sve_default_vl() is an error if CONFIG_SVE is
> not defined as we really want get_sve_default_vl() to return -1
> unconditionally in that case. Having set_sve_default_vl() inside the
> #ifdef ensures that.

Fair point, I'm not sure how valuable it is.  We manage without it thus
far: prior to these changes, the sve_default_vl variable was not #ifdeffed.

> I don't care too strongly either way, but I already queued my diff last
> night [1] in order to fix linux-next, so I'd prefer not to drop it unless
> there's a functional reason to do so.

Ack, since this change would be purely to ease future maintanence (or
not, if you judge it's not useful), it's not urgent.

Cheers
---Dave
diff mbox series

Patch

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 94289d1..19865c9 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -12,6 +12,7 @@ 
 #include <linux/bug.h>
 #include <linux/cache.h>
 #include <linux/compat.h>
+#include <linux/compiler.h>
 #include <linux/cpu.h>
 #include <linux/cpu_pm.h>
 #include <linux/kernel.h>
@@ -119,7 +120,17 @@  struct fpsimd_last_state_struct {
 static DEFINE_PER_CPU(struct fpsimd_last_state_struct, fpsimd_last_state);
 
 /* Default VL for tasks that don't set it explicitly: */
-static int sve_default_vl = -1;
+static int __sve_default_vl = -1;
+
+static int get_sve_default_vl(void)
+{
+	return READ_ONCE(__sve_default_vl);
+}
+
+static void set_sve_default_vl(int val)
+{
+	WRITE_ONCE(__sve_default_vl, val);
+}
 
 #ifdef CONFIG_ARM64_SVE
 
@@ -345,7 +356,7 @@  static int sve_proc_do_default_vl(struct ctl_table *table, int write,
 				  loff_t *ppos)
 {
 	int ret;
-	int vl = sve_default_vl;
+	int vl = get_sve_default_vl();
 	struct ctl_table tmp_table = {
 		.data = &vl,
 		.maxlen = sizeof(vl),
@@ -362,7 +373,7 @@  static int sve_proc_do_default_vl(struct ctl_table *table, int write,
 	if (!sve_vl_valid(vl))
 		return -EINVAL;
 
-	sve_default_vl = find_supported_vector_length(vl);
+	set_sve_default_vl(find_supported_vector_length(vl));
 	return 0;
 }
 
@@ -869,7 +880,7 @@  void __init sve_setup(void)
 	 * For the default VL, pick the maximum supported value <= 64.
 	 * VL == 64 is guaranteed not to grow the signal frame.
 	 */
-	sve_default_vl = find_supported_vector_length(64);
+	set_sve_default_vl(find_supported_vector_length(64));
 
 	bitmap_andnot(tmp_map, sve_vq_partial_map, sve_vq_map,
 		      SVE_VQ_MAX);
@@ -890,7 +901,7 @@  void __init sve_setup(void)
 	pr_info("SVE: maximum available vector length %u bytes per vector\n",
 		sve_max_vl);
 	pr_info("SVE: default vector length %u bytes per vector\n",
-		sve_default_vl);
+		get_sve_default_vl());
 
 	/* KVM decides whether to support mismatched systems. Just warn here: */
 	if (sve_max_virtualisable_vl < sve_max_vl)
@@ -1030,13 +1041,13 @@  void fpsimd_flush_thread(void)
 		 * vector length configured: no kernel task can become a user
 		 * task without an exec and hence a call to this function.
 		 * By the time the first call to this function is made, all
-		 * early hardware probing is complete, so sve_default_vl
+		 * early hardware probing is complete, so __sve_default_vl
 		 * should be valid.
 		 * If a bug causes this to go wrong, we make some noise and
 		 * try to fudge thread.sve_vl to a safe value here.
 		 */
 		vl = current->thread.sve_vl_onexec ?
-			current->thread.sve_vl_onexec : sve_default_vl;
+			current->thread.sve_vl_onexec : get_sve_default_vl();
 
 		if (WARN_ON(!sve_vl_valid(vl)))
 			vl = SVE_VL_MIN;