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

[Qian Cai <cai@lca.pw>]

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

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