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

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

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

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