powerpc: smp_wmb lwsync optimisation fix

PATCH In review Nick Piggin 3 participants 7 messages Nov 1, 2008 – Nov 3, 2008

Subsystems (4)

PowerPC Locking primitives Atomic infrastructure Bitops API

lore

Share & tools

lore

Nick Piggin

author

2008-11-01 12:33 UTC #0 lore raw

PATCH 1/1 powerpc: smp_wmb lwsync optimisation fix

A previous change removed __SUBARCH_HAS_LWSYNC define, and replaced it with __powerpc64__. smp_wmb() seems to be the last place not updated.

Signed-off-by Nick Piggin <[email protected]>

---

arch/powerpc/include/asm/system.h +1 -1

Index: linux-2.6/arch/powerpc/include/asm/system.h
===================================================================
@@ -44,7 +44,7 @@
 #define AT_VECTOR_SIZE_ARCH 6 /* entries in ARCH_DLINFO */
 #ifdef CONFIG_SMP
 
-#ifdef __SUBARCH_HAS_LWSYNC
+#if defined(__powerpc64__)
 #    define SMPWMB      lwsync
 #else
 #    define SMPWMB      eieio

Nick Piggin

author

2008-11-01 13:05 UTC #1 lore raw

PATCH 1/1 powerpc: rmp_wmb lwsync optimisation

smp_rmb can be lwsync if possible. Clarify the comment.

Signed-off-by Nick Piggin <[email protected]>

---

arch/powerpc/include/asm/system.h +11 -9

Index: linux-2.6/arch/powerpc/include/asm/system.h
===================================================================
@@ -23,15 +23,17 @@
  * read_barrier_depends() prevents data-dependent loads being reordered
  *	across this point (nop on PPC).
  *
- * We have to use the sync instructions for mb(), since lwsync doesn't
- * order loads with respect to previous stores.  Lwsync is fine for
- * rmb(), though. Note that rmb() actually uses a sync on 32-bit
- * architectures.
+ * *mb() variants without smp_ prefix must order all types of memory
+ * operations with one another. sync is the only instruction sufficient
+ * to do this.
  *
- * For wmb(), we use sync since wmb is used in drivers to order
- * stores to system memory with respect to writes to the device.
- * However, smp_wmb() can be a lighter-weight lwsync or eieio barrier
- * on SMP since it is only used to order updates to system memory.
+ * For the smp_ barriers, ordering is for cacheable memory operations
+ * only. We have to use the sync instruction for smp_mb(), since lwsync
+ * doesn't order loads with respect to previous stores.  Lwsync can be
+ * used for smp_rmb() and smp_wmb().
+ *
+ * However, on 32-bit, lwsync is actually just a sync, in which case smp_wmb()
+ * can be a lighter-weight eieio barrier.
  */
 #define mb()   __asm__ __volatile__ ("sync" : : : "memory")
 #define rmb()  __asm__ __volatile__ ("sync" : : : "memory")

@@ -51,7 +53,7 @@
 #endif
 
 #define smp_mb()	mb()
-#define smp_rmb()	rmb()
+#define smp_rmb()	__asm__ __volatile__(LWSYNC_ON_SMP : : : "memory")
 #define smp_wmb()	__asm__ __volatile__ (__stringify(SMPWMB) : : :"memory")
 #define smp_read_barrier_depends()	read_barrier_depends()
 #else

Nick Piggin

author

2008-11-01 13:07 UTC #2 lore raw

RFC PATCH 1/1 powerpc: replace isync with lwsync

In reply to Nick Piggin

Hi guys,

This is an interesting one for me. AFAIKS it is possible to use lwsync for a full barrier after a successful ll/sc operation, right? (or stop me here if I'm wrong).

Anyway, I was interested in exploring this. Unfortunately my G5 might not be very indicative of more modern, and future developments in high end powerpc CPUs... it would be interesting to get opinion and verification from insiders.

OK, on my G5, using lwsync instead of isync in spinlocks is a bit faster in really stupid userspace microbenchmark (4 threads, looping, locking, incrementing shared variable, unlocking). This prompted me to look at bit further.

So I converted a significant number of isyncs in the kernel to lwsync. The resulting kernel (on 2 core, 2 socket system) ran tbench consistently about 1.75% faster than unpatched (avg ~934MB/s vs ~918MB/s) (Tbench was just the first benchmark I picked that could run really quickly and give relatively stable numbers). This seems pretty significant. More than I was expecting.

I've attached the patch I used (I've not thoroughly audited the code for all users of isync, only replaced some main ones)

Now I'd like to know why this is faster, whether it makes sense to to, whether it helps with more useful workloads and modern systems.

isync followed by a branch I guess does something like puts a bubble into the pipeline until the branch retires? So it is probably always going to cost some cycles.

lwsync on the other hand, I suppose has to do a bit more when it comes to the store queue. Maybe flush it or insert a barrier or something into it. Also has some ordering of loads, but effectively no more than isync AFAIKS.

Thanks,
Nick
---

arch/powerpc/include/asm/atomic.h +12 ~~-12~~

Index: linux-2.6/arch/powerpc/include/asm/atomic.h
===================================================================
@@ -55,7 +55,7 @@
 	PPC405_ERR77(0,%2)
 "	stwcx.	%0,0,%2 \n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (t)
 	: "r" (a), "r" (&v->counter)
 	: "cc", "memory");

@@ -91,7 +91,7 @@
 	PPC405_ERR77(0,%2)
 "	stwcx.	%0,0,%2 \n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (t)
 	: "r" (a), "r" (&v->counter)
 	: "cc", "memory");

@@ -125,7 +125,7 @@
 	PPC405_ERR77(0,%1)
 "	stwcx.	%0,0,%1 \n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (t)
 	: "r" (&v->counter)
 	: "cc", "memory");

@@ -169,7 +169,7 @@
 	PPC405_ERR77(0,%1)
 "	stwcx.	%0,0,%1\n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (t)
 	: "r" (&v->counter)
 	: "cc", "memory");

@@ -202,7 +202,7 @@
 	PPC405_ERR77(0,%2)
 "	stwcx.	%0,0,%1 \n\
 	bne-	1b \n"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 "	subf	%0,%2,%0 \n\
 2:"
 	: "=&r" (t)

@@ -235,7 +235,7 @@
 	PPC405_ERR77(0,%1)
 "	stwcx.	%0,0,%1\n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	"\n\
 2:"	: "=&b" (t)
 	: "r" (&v->counter)

@@ -293,7 +293,7 @@
 	add	%0,%1,%0\n\
 	stdcx.	%0,0,%2 \n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (t)
 	: "r" (a), "r" (&v->counter)
 	: "cc", "memory");

@@ -327,7 +327,7 @@
 	subf	%0,%1,%0\n\
 	stdcx.	%0,0,%2 \n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (t)
 	: "r" (a), "r" (&v->counter)
 	: "cc", "memory");

@@ -359,7 +359,7 @@
 	addic	%0,%0,1\n\
 	stdcx.	%0,0,%1 \n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (t)
 	: "r" (&v->counter)
 	: "cc", "memory");

@@ -401,7 +401,7 @@
 	addic	%0,%0,-1\n\
 	stdcx.	%0,0,%1\n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (t)
 	: "r" (&v->counter)
 	: "cc", "memory");

@@ -427,7 +427,7 @@
 	blt-	2f\n\
 	stdcx.	%0,0,%1\n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	"\n\
 2:"	: "=&r" (t)
 	: "r" (&v->counter)

@@ -460,7 +460,7 @@
 	add	%0,%2,%0 \n"
 "	stdcx.	%0,0,%1 \n\
 	bne-	1b \n"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 "	subf	%0,%2,%0 \n\
 2:"
 	: "=&r" (t)

arch/powerpc/include/asm/bitops.h +4 -4

Index: linux-2.6/arch/powerpc/include/asm/bitops.h
===================================================================
@@ -139,7 +139,7 @@
 	PPC405_ERR77(0,%3)
 	PPC_STLCX "%1,0,%3 \n"
 	"bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (old), "=&r" (t)
 	: "r" (mask), "r" (p)
 	: "cc", "memory");

@@ -160,7 +160,7 @@
 	PPC405_ERR77(0,%3)
 	PPC_STLCX "%1,0,%3 \n"
 	"bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (old), "=&r" (t)
 	: "r" (mask), "r" (p)
 	: "cc", "memory");

@@ -182,7 +182,7 @@
 	PPC405_ERR77(0,%3)
 	PPC_STLCX "%1,0,%3 \n"
 	"bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (old), "=&r" (t)
 	: "r" (mask), "r" (p)
 	: "cc", "memory");

@@ -204,7 +204,7 @@
 	PPC405_ERR77(0,%3)
 	PPC_STLCX "%1,0,%3 \n"
 	"bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (old), "=&r" (t)
 	: "r" (mask), "r" (p)
 	: "cc", "memory");

arch/powerpc/include/asm/futex.h +1 -1

Index: linux-2.6/arch/powerpc/include/asm/futex.h
===================================================================
@@ -97,7 +97,7 @@
         PPC405_ERR77(0,%2)
 "2:     stwcx.  %4,0,%2\n\
         bne-    1b\n"
-        ISYNC_ON_SMP
+        LWSYNC_ON_SMP
 "3:	.section .fixup,\"ax\"\n\
 4:	li	%0,%5\n\
 	b	3b\n\

arch/powerpc/include/asm/spinlock.h +3 -3

Index: linux-2.6/arch/powerpc/include/asm/spinlock.h
===================================================================
@@ -65,7 +65,7 @@
 	bne-		2f\n\
 	stwcx.		%1,0,%2\n\
 	bne-		1b\n\
-	isync\n\
+	lwsync\n\
 2:"	: "=&r" (tmp)
 	: "r" (token), "r" (&lock->slock)
 	: "cr0", "memory");

@@ -193,7 +193,7 @@
 	PPC405_ERR77(0,%1)
 "	stwcx.		%0,0,%1\n\
 	bne-		1b\n\
-	isync\n\
+	lwsync\n\
 2:"	: "=&r" (tmp)
 	: "r" (&rw->lock)
 	: "cr0", "xer", "memory");

@@ -217,7 +217,7 @@
 	PPC405_ERR77(0,%1)
 "	stwcx.		%1,0,%2\n\
 	bne-		1b\n\
-	isync\n\
+	lwsync\n\
 2:"	: "=&r" (tmp)
 	: "r" (token), "r" (&rw->lock)
 	: "cr0", "memory");

arch/powerpc/include/asm/system.h +4 -4

Index: linux-2.6/arch/powerpc/include/asm/system.h
===================================================================
@@ -235,7 +235,7 @@
 	PPC405_ERR77(0,%2)
 "	stwcx.	%3,0,%2 \n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (prev), "+m" (*(volatile unsigned int *)p)
 	: "r" (p), "r" (val)
 	: "cc", "memory");

@@ -278,7 +278,7 @@
 	PPC405_ERR77(0,%2)
 "	stdcx.	%3,0,%2 \n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	: "=&r" (prev), "+m" (*(volatile unsigned long *)p)
 	: "r" (p), "r" (val)
 	: "cc", "memory");

@@ -371,7 +371,7 @@
 	PPC405_ERR77(0,%2)
 "	stwcx.	%4,0,%2\n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	"\n\
 2:"
 	: "=&r" (prev), "+m" (*p)

@@ -416,7 +416,7 @@
 	bne-	2f\n\
 	stdcx.	%4,0,%2\n\
 	bne-	1b"
-	ISYNC_ON_SMP
+	LWSYNC_ON_SMP
 	"\n\
 2:"
 	: "=&r" (prev), "+m" (*p)

arch/powerpc/include/asm/synch.h +1 -1

Index: linux-2.6/arch/powerpc/include/asm/synch.h
===================================================================
@@ -34,7 +34,7 @@
 
 #ifdef CONFIG_SMP
 #define ISYNC_ON_SMP	"\n\tisync\n"
-#define LWSYNC_ON_SMP	stringify_in_c(LWSYNC) "\n"
+#define LWSYNC_ON_SMP	"\n\t" stringify_in_c(LWSYNC) "\n"
 #else
 #define ISYNC_ON_SMP
 #define LWSYNC_ON_SMP

Paul Mackerras

2008-11-03 05:32 UTC #3 lore raw

Re: [rfc][patch] powerpc: replace isync with lwsync

Quoted 3 lines · Nick Piggin

This is an interesting one for me. AFAIKS it is possible to use lwsync for a full barrier after a successful ll/sc operation, right? (or stop me here if I'm wrong).

An lwsync would order subsequent loads after the lwarx/ldarx, and subsequent stores after the stcwx./stcdx., which should be good enough.

Quoted 3 lines

isync followed by a branch I guess does something like puts a bubble into the pipeline until the branch retires? So it is probably always going to cost some cycles.

I don't know about "retires", but isync is going to stop following instructions from executing until the outcome of the branch is known.

On machines that don't have lwsync we will still want to use isync (since the other alternative would be the full heavyweight sync). Your patch doesn't seem to do that.

Paul.

Nick Piggin

author

2008-11-03 08:31 UTC #4 lore raw

Re: [rfc][patch] powerpc: replace isync with lwsync

Quoted 9 lines · Paul Mackerras

Quoted 3 lines · Nick Piggin
This is an interesting one for me. AFAIKS it is possible to use lwsync for a full barrier after a successful ll/sc operation, right? (or stop me here if I'm wrong).
An lwsync would order subsequent loads after the lwarx/ldarx, and subsequent stores after the stcwx./stcdx., which should be good enough.

OK, thanks for confirmation.

Quoted 6 lines

Quoted 3 lines
isync followed by a branch I guess does something like puts a bubble into the pipeline until the branch retires? So it is probably always going to cost some cycles.
I don't know about "retires", but isync is going to stop following instructions from executing until the outcome of the branch is known.

OK, I probably don't use the right terminology. I assume the branch retires when its outcome is known and the CPU starts executing the result.

Quoted 3 lines

On machines that don't have lwsync we will still want to use isync (since the other alternative would be the full heavyweight sync). Your patch doesn't seem to do that.

No, it's just a quick hack at the moment. I think your reply gets it past the not-totally-broken stage :) But at this point I can't justify sending such a change upstream based on a small improvement on G5. I would like to know about newer POWER CPUs, and even unreleased ones. If there is some reason lwsync gets relatively more constraining or difficult to execute than isync, then maybe this change is not useful.

Thanks,
Nick

Kumar Gala

2008-11-01 16:48 UTC #5 lore raw

In reply to Nick Piggin

Quoted 2 lines · Nick Piggin

A previous change removed __SUBARCH_HAS_LWSYNC define, and replaced it with __powerpc64__. smp_wmb() seems to be the last place not updated.

Uugh... no.. I missed the patch that removed __SUBARCH_HAS_LWSYNC, but thats no good. We have LWSYNC on non-powerpc64 machines. Will go figure out who forgets we have ppc32 machines :)

- k

Quoted 23 lines

Signed-off-by Nick Piggin <[email protected]>
---
Index: linux-2.6/arch/powerpc/include/asm/system.h
===================================================================
--- linux-2.6.orig/arch/powerpc/include/asm/system.h 2008-11-01
20:31:51.000000000 +1100
+++ linux-2.6/arch/powerpc/include/asm/system.h 2008-11-01
20:32:33.000000000 +1100
arch/powerpc/include/asm/system.h +1 -1
@@ -44,7 +44,7 @@
 #define AT_VECTOR_SIZE_ARCH 6 /* entries in ARCH_DLINFO */
 #ifdef CONFIG_SMP
 
-#ifdef __SUBARCH_HAS_LWSYNC
+#if defined(__powerpc64__)
 #    define SMPWMB      lwsync
 #else
 #    define SMPWMB      eieio
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Nick Piggin

author

2008-11-02 01:42 UTC #6 lore raw

Quoted 9 lines · Kumar Gala

Quoted 2 lines · Nick Piggin
A previous change removed __SUBARCH_HAS_LWSYNC define, and replaced it with __powerpc64__. smp_wmb() seems to be the last place not updated.
Uugh... no.. I missed the patch that removed __SUBARCH_HAS_LWSYNC, but thats no good. We have LWSYNC on non-powerpc64 machines. Will go figure out who forgets we have ppc32 machines :)

I think it may have been you :)

But actually, SMPWMB would need more massaging before it is suitable, by the looks of your LWSYNC define in synch.h.

I don't mind, so much, about how this gets fixed. But it would be nice to fix it somehow. After the smp_rmb and smp_wmb patches, there are practically no sync instructions left in mm/ :)

Quoted 26 lines

- k
Quoted 23 lines
Signed-off-by Nick Piggin <[email protected]>
---
Index: linux-2.6/arch/powerpc/include/asm/system.h
===================================================================
--- linux-2.6.orig/arch/powerpc/include/asm/system.h 2008-11-01
20:31:51.000000000 +1100
+++ linux-2.6/arch/powerpc/include/asm/system.h 2008-11-01
20:32:33.000000000 +1100
arch/powerpc/include/asm/system.h +1 -1
@@ -44,7 +44,7 @@
 #define AT_VECTOR_SIZE_ARCH 6 /* entries in ARCH_DLINFO */
 #ifdef CONFIG_SMP
 
-#ifdef __SUBARCH_HAS_LWSYNC
+#if defined(__powerpc64__)
 #    define SMPWMB      lwsync
 #else
 #    define SMPWMB      eieio
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev