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

From: Will Deacon <will@kernel.org>
To: Dave Martin <Dave.Martin@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>, Qian Cai <cai@lca.pw>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/2] arm64/sve: Eliminate data races on sve_default_vl
Date: Wed, 17 Jun 2020 11:08:32 +0100	[thread overview]
Message-ID: <20200617100832.GA3368@willie-the-truck> (raw)
In-Reply-To: <20200617094054.GP25945@arm.com>

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

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