Re: [PATCH] powerpc: Fix smp_wmb barrier definition use use lwsync consistently

[Nicholas Piggin <npiggin@gmail.com>]

On Wed, 28 Mar 2018 23:40:05 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > asm/barrier.h is not always included after asm/synch.h, which meant
> > it was missing __SUBARCH_HAS_LWSYNC, so in some files smp_wmb() would
> > be eieio when it should be lwsync. kernel/time/hrtimer.c is one case.  
> 
> Wow nice catch. Only broken since 2008 presumably.
> 
> Some days I think maybe we aren't very good at this writing software
> thing, good to have some certainty :)

Yeah, I only caught it by luck when looking through instruction traces.
The pipeline model just happens to make eieio look different to most
other instructions (which is likely a bug in the model) which made me
look closer at it. Could have been with us for another 10 years.

> > __SUBARCH_HAS_LWSYNC is only used in one place, so just fold it in
> > to where it's used. Previously with my small simulator config, 377
> > instances of eieio in the tree. After this patch there are 55.  
> 
> At least for Book3S this isn't actually a terrible bug AFAICS:
> 
>  - smp_wmb() is only defined to order accesses to cacheable memory.
>  - smp_wmb() only orders prior stores vs later stores.
>  - eieio orders all prior stores vs all later stores for cacheable
>    memory.
>  - lwsync orders everything except prior stores vs later loads for
>    cacheable memory.
> 
> So eieio and lwsync are both valid to use as smp_wmb(), but it's still
> terrible fishy that we were using both in different places depending on
> include ordering.

Oh yeah it's not a bug in that it would cause violation of memory
ordering, only performance (and expectations when debugging and
observing things I guess). eieio works fine for smp_wmb().

> I'm inclined to tag this for stable unless anyone can think of a reason
> not to?

I think that would be good.

Thanks,
Nick