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

On Tue, Jun 16, 2020 at 05:17:04PM +0100, Dave Martin wrote:

quoted

On Tue, Jun 16, 2020 at 04:04:51PM +0100, Will Deacon wrote:

quoted

On Tue, Jun 16, 2020 at 09:18:08AM -0400, Qian Cai wrote:

quoted

On Wed, Jun 10, 2020 at 06:03:10PM +0100, Dave Martin wrote:

quoted

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) */