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

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

[Nicholas Piggin]

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.

Cc: Anton Blanchard <anton@samba.org>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/barrier.h | 3 ++-
 arch/powerpc/include/asm/synch.h   | 4 ----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
index 10daa1d56e0a..c7c63959ba91 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -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
diff --git a/arch/powerpc/include/asm/synch.h b/arch/powerpc/include/asm/synch.h
index 63e7f5a1f105..6ec546090ba1 100644
--- a/arch/powerpc/include/asm/synch.h
+++ b/arch/powerpc/include/asm/synch.h
@@ -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,
-- 
2.16.1

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

[Michael Ellerman]

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 :)

> __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.

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

cheers

> diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
> index 10daa1d56e0a..c7c63959ba91 100644
> --- a/arch/powerpc/include/asm/barrier.h
> +++ b/arch/powerpc/include/asm/barrier.h
> @@ -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
> diff --git a/arch/powerpc/include/asm/synch.h b/arch/powerpc/include/asm/synch.h
> index 63e7f5a1f105..6ec546090ba1 100644
> --- a/arch/powerpc/include/asm/synch.h
> +++ b/arch/powerpc/include/asm/synch.h
> @@ -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,
> -- 
> 2.16.1

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

[Nicholas Piggin]

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

Re: powerpc: Fix smp_wmb barrier definition use use lwsync consistently

[Michael Ellerman]

On Thu, 2018-03-22 at 10:41:46 UTC, Nicholas Piggin wrote:
> 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.
> 
> Cc: Anton Blanchard <anton@samba.org>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/0bfdf598900fd62869659f360d3387

cheers