On Wed, 28 Mar 2018 23:40:05 +1100 Michael Ellerman <m...@ellerman.id.au> wrote:
> Nicholas Piggin <npig...@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