[PATCH] powerpc: Fix smp_wmb barrier definition use use lwsync consistently
npiggin at gmail.com
Thu Mar 29 00:43:10 AEDT 2018
On Wed, 28 Mar 2018 23:40:05 +1100
Michael Ellerman <mpe at ellerman.id.au> wrote:
> Nicholas Piggin <npiggin at 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
> - 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.
More information about the Linuxppc-dev