powerpc: Fix smp_wmb barrier definition use use lwsync consistently
Nicholas Piggin
reviewerPATCH 1/1 powerpc: Fix smp_wmb barrier definition use use lwsync consistently
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.
__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.
--- arch/powerpc/include/asm/barrier.h | 3 ++- arch/powerpc/include/asm/synch.h | 4 ---- 2 files changed, 2 insertions(+), 5 deletions(-)
arch/powerpc/include/asm/barrier.h +2 -1
@@ -35,7 +35,8 @@#define rmb () __asm__ __volatile__ ("sync" : : : "memory")#define wmb () __asm__ __volatile__ ("sync" : : : "memory") -#ifdef __SUBARCH_HAS_LWSYNC +/* The sub-arch has lwsync */ +#if defined(__powerpc64__) || defined(CONFIG_PPC_E500MC )# define SMPWMB LWSYNC#else # define SMPWMB eieio
arch/powerpc/include/asm/synch.h +0 -4
@@ -6,10 +6,6 @@#include <linux/stringify.h> #include <asm/feature-fixups.h> -#if defined(__powerpc64__) || defined(CONFIG_PPC_E500MC ) -#define __SUBARCH_HAS_LWSYNC -#endif -#ifndef __ASSEMBLY__extern unsigned int __start___lwsync_fixup, __stop___lwsync_fixup; extern void do_lwsync_fixups(unsigned long value, void *fixup_start,
Signature · 1 line
2.16.1
Michael Ellerman
reviewerQuoted 3 lines · Nicholas Piggin
asm/barrier.his not always included afterasm/synch.h, which meant it was missing__SUBARCH_HAS_LWSYNC, so in some filessmp_wmb()would be eieio when it should be lwsync.kernel/time/hrtimer.cis 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 :)
Quoted 3 lines
__SUBARCH_HAS_LWSYNCis 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.
I'm inclined to tag this for stable unless anyone can think of a reason not to?
cheers
Quoted 31 lines
arch/powerpc/include/asm/barrier.h +2
-1@@ -35,7 +35,8 @@#define rmb () __asm__ __volatile__ ("sync" : : : "memory")#define wmb () __asm__ __volatile__ ("sync" : : : "memory") -#ifdef __SUBARCH_HAS_LWSYNC +/* The sub-arch has lwsync */ +#if defined(__powerpc64__) || defined(CONFIG_PPC_E500MC )# define SMPWMB LWSYNC#else # define SMPWMB eieioarch/powerpc/include/asm/synch.h +0
-4@@ -6,10 +6,6 @@#include <linux/stringify.h> #include <asm/feature-fixups.h> -#if defined(__powerpc64__) || defined(CONFIG_PPC_E500MC ) -#define __SUBARCH_HAS_LWSYNC -#endif -#ifndef __ASSEMBLY__extern unsigned int __start___lwsync_fixup, __stop___lwsync_fixup; extern void do_lwsync_fixups(unsigned long value, void *fixup_start,Signature · 1 line
2.16.1
Nicholas Piggin
reviewerQuoted 10 lines · On Wed, 28 Mar 2018 23:40:05 +1100 Michael Ellerman <[email protected]> wrote:
Quoted 3 lines · Nicholas Piggin
asm/barrier.his not always included afterasm/synch.h, which meant it was missing__SUBARCH_HAS_LWSYNC, so in some filessmp_wmb()would be eieio when it should be lwsync.kernel/time/hrtimer.cis 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.
Quoted 13 lines
Quoted 3 lines
__SUBARCH_HAS_LWSYNCis 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.
Quoted 3 lines
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().
Quoted 2 lines
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
Michael Ellerman
AppliedQuoted 10 lines · Nicholas Piggin
asm/barrier.his not always included afterasm/synch.h, which meant it was missing__SUBARCH_HAS_LWSYNC, so in some filessmp_wmb()would be eieio when it should be lwsync.kernel/time/hrtimer.cis one case.
__SUBARCH_HAS_LWSYNCis 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.Cc Anton Blanchard <[email protected]>Signed-off-by Nicholas Piggin <[email protected]>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/0bfdf598900fd62869659f360d3387
cheers