[patch 1/2] powerpc: rmb fix

STALE6933d

15 messages, 4 authors, 2007-08-24 · open the first message on its own page

[patch 1/2] powerpc: rmb fix

From: Nick Piggin <hidden>
Date: 2007-08-21 02:11:48

In the interest of completeness, I'll split these patches up and submit to
the powerpc dev list. Any discussion or ack/nack would be appreciated.

---
lwsync is defined to only order memory operations on cacheable memory.
A full sync appears to be the only barrier that will order all memory
loads including device memory.

Signed-off-by: Nick Piggin <redacted>

Index: linux-2.6/include/asm-powerpc/system.h
===================================================================
--- linux-2.6.orig/include/asm-powerpc/system.h
+++ linux-2.6/include/asm-powerpc/system.h
@@ -33,7 +33,7 @@
  * SMP since it is only used to order updates to system memory.
  */
 #define mb()   __asm__ __volatile__ ("sync" : : : "memory")
-#define rmb()  __asm__ __volatile__ (__stringify(LWSYNC) : : : "memory")
+#define rmb()  __asm__ __volatile__ ("sync" : : : "memory")
 #define wmb()  __asm__ __volatile__ ("sync" : : : "memory")
 #define read_barrier_depends()  do { } while(0)
 
@@ -42,7 +42,7 @@
 #ifdef __KERNEL__
 #ifdef CONFIG_SMP
 #define smp_mb()	mb()
-#define smp_rmb()	rmb()
+#define smp_rmb()	__asm__ __volatile__ (__stringify(LWSYNC) : : : "memory")
 #define smp_wmb()	eieio()
 #define smp_read_barrier_depends()	read_barrier_depends()
 #else

[patch 1/2] powerpc: smp_wmb speedup

From: Nick Piggin <hidden>
Date: 2007-08-21 02:16:57

This one is perhaps not as straightforward. I'm pretty limited in the types
of powerpc machines I can test with, so I don't actually know whether this
is the right thing to do on power5/6 etc. I can supply the simple test program
I used if anybody is interested.

---
On my dual G5, lwsync is over 5 times faster than eieio when used in a simple
test case (that actually makes real use of lwsync to provide write ordering).

This is not surprising, as it avoids the IO access synchronisation of eieio,
and still permits the important relaxation of executing loads before stores.
The on sub-architectures where lwsync is unavailable, eieio is retained, as
it should be faster than the alternative full sync (eieio is a proper subset
of sync).

Signed-off-by: Nick Piggin <redacted>

Index: linux-2.6/include/asm-powerpc/system.h
===================================================================
--- linux-2.6.orig/include/asm-powerpc/system.h
+++ linux-2.6/include/asm-powerpc/system.h
@@ -43,7 +43,11 @@
 #ifdef CONFIG_SMP
 #define smp_mb()	mb()
 #define smp_rmb()	__asm__ __volatile__ (__stringify(LWSYNC) : : : "memory")
+#ifdef __SUBARCH_HAS_LWSYNC
+#define smp_wmb()	__asm__ __volatile__ (__stringify(LWSYNC) : : : "memory")
+#else
 #define smp_wmb()	eieio()
+#endif
 #define smp_read_barrier_depends()	read_barrier_depends()
 #else
 #define smp_mb()	barrier()

Re: [patch 1/2] powerpc: smp_wmb speedup

From: Nick Piggin <hidden>
Date: 2007-08-21 02:21:25

Sorry, this is patch 2/2 of course.

On Tue, Aug 21, 2007 at 04:16:52AM +0200, Nick Piggin wrote:
quoted hunk
This one is perhaps not as straightforward. I'm pretty limited in the types
of powerpc machines I can test with, so I don't actually know whether this
is the right thing to do on power5/6 etc. I can supply the simple test program
I used if anybody is interested.

---
On my dual G5, lwsync is over 5 times faster than eieio when used in a simple
test case (that actually makes real use of lwsync to provide write ordering).

This is not surprising, as it avoids the IO access synchronisation of eieio,
and still permits the important relaxation of executing loads before stores.
The on sub-architectures where lwsync is unavailable, eieio is retained, as
it should be faster than the alternative full sync (eieio is a proper subset
of sync).

Signed-off-by: Nick Piggin <redacted>

Index: linux-2.6/include/asm-powerpc/system.h
===================================================================
--- linux-2.6.orig/include/asm-powerpc/system.h
+++ linux-2.6/include/asm-powerpc/system.h
@@ -43,7 +43,11 @@
 #ifdef CONFIG_SMP
 #define smp_mb()	mb()
 #define smp_rmb()	__asm__ __volatile__ (__stringify(LWSYNC) : : : "memory")
+#ifdef __SUBARCH_HAS_LWSYNC
+#define smp_wmb()	__asm__ __volatile__ (__stringify(LWSYNC) : : : "memory")
+#else
 #define smp_wmb()	eieio()
+#endif
 #define smp_read_barrier_depends()	read_barrier_depends()
 #else
 #define smp_mb()	barrier()

Re: [patch 1/2] powerpc: rmb fix

From: Joel Schopp <hidden>
Date: 2007-08-21 19:07:22

quoted hunk
 #define mb()   __asm__ __volatile__ ("sync" : : : "memory")
-#define rmb()  __asm__ __volatile__ (__stringify(LWSYNC) : : : "memory")
+#define rmb()  __asm__ __volatile__ ("sync" : : : "memory")
 #define wmb()  __asm__ __volatile__ ("sync" : : : "memory")
 #define read_barrier_depends()  do { } while(0)
 
@@ -42,7 +42,7 @@
 #ifdef __KERNEL__
 #ifdef CONFIG_SMP
 #define smp_mb()	mb()
-#define smp_rmb()	rmb()
+#define smp_rmb()	__asm__ __volatile__ (__stringify(LWSYNC) : : : "memory")
 #define smp_wmb()	eieio()
 #define smp_read_barrier_depends()	read_barrier_depends()
 #else
I had to think about this one for awhile.  It looks at first glance to be the right 
thing to do.  But I do wonder how long rmb() has been lwsync and if as a practical 
matter that has caused any problems?  If this isn't causing any problems maybe there 
is some loigic we are overlooking?

Re: [patch 1/2] powerpc: rmb fix

From: Segher Boessenkool <hidden>
Date: 2007-08-21 19:43:26

quoted
 #define mb()   __asm__ __volatile__ ("sync" : : : "memory")
-#define rmb()  __asm__ __volatile__ (__stringify(LWSYNC) : : : 
"memory")
+#define rmb()  __asm__ __volatile__ ("sync" : : : "memory")
 #define wmb()  __asm__ __volatile__ ("sync" : : : "memory")
 #define read_barrier_depends()  do { } while(0)
@@ -42,7 +42,7 @@
 #ifdef __KERNEL__
 #ifdef CONFIG_SMP
 #define smp_mb()	mb()
-#define smp_rmb()	rmb()
+#define smp_rmb()	__asm__ __volatile__ (__stringify(LWSYNC) : : : 
"memory")
 #define smp_wmb()	eieio()
 #define smp_read_barrier_depends()	read_barrier_depends()
 #else
I had to think about this one for awhile.  It looks at first glance to 
be the right
thing to do.  But I do wonder how long rmb() has been lwsync
Since the {ppc,ppc64} -> powerpc merge.
and if as a practical matter that has caused any problems?
It has not as far as I know.
If this isn't causing any problems maybe there
is some loigic we are overlooking?
The I/O accessor functions enforce the necessary ordering
already I believe.


Segher

Re: [patch 1/2] powerpc: rmb fix

From: Linas Vepstas <hidden>
Date: 2007-08-21 21:42:22

On Tue, Aug 21, 2007 at 09:43:17PM +0200, Segher Boessenkool wrote:
quoted
quoted
 #define mb()   __asm__ __volatile__ ("sync" : : : "memory")
-#define rmb()  __asm__ __volatile__ (__stringify(LWSYNC) : : : 
"memory")
+#define rmb()  __asm__ __volatile__ ("sync" : : : "memory")
 #define wmb()  __asm__ __volatile__ ("sync" : : : "memory")
 #define read_barrier_depends()  do { } while(0)
@@ -42,7 +42,7 @@
 #ifdef __KERNEL__
 #ifdef CONFIG_SMP
 #define smp_mb()	mb()
-#define smp_rmb()	rmb()
+#define smp_rmb()	__asm__ __volatile__ (__stringify(LWSYNC) : : : 
"memory")
 #define smp_wmb()	eieio()
 #define smp_read_barrier_depends()	read_barrier_depends()
 #else
I had to think about this one for awhile.  It looks at first glance to 
be the right
thing to do.  But I do wonder how long rmb() has been lwsync
Since the {ppc,ppc64} -> powerpc merge.
quoted
and if as a practical matter that has caused any problems?
It has not as far as I know.
quoted
If this isn't causing any problems maybe there
is some loigic we are overlooking?
The I/O accessor functions enforce the necessary ordering
already I believe.
So, is this patch desirable? 

--linas

Re: [patch 1/2] powerpc: rmb fix

From: Nick Piggin <hidden>
Date: 2007-08-22 01:16:56

On Tue, Aug 21, 2007 at 09:43:17PM +0200, Segher Boessenkool wrote:
quoted
quoted
#define mb()   __asm__ __volatile__ ("sync" : : : "memory")
-#define rmb()  __asm__ __volatile__ (__stringify(LWSYNC) : : : 
"memory")
+#define rmb()  __asm__ __volatile__ ("sync" : : : "memory")
#define wmb()  __asm__ __volatile__ ("sync" : : : "memory")
#define read_barrier_depends()  do { } while(0)
@@ -42,7 +42,7 @@
#ifdef __KERNEL__
#ifdef CONFIG_SMP
#define smp_mb()	mb()
-#define smp_rmb()	rmb()
+#define smp_rmb()	__asm__ __volatile__ (__stringify(LWSYNC) : : : 
"memory")
#define smp_wmb()	eieio()
#define smp_read_barrier_depends()	read_barrier_depends()
#else
I had to think about this one for awhile.  It looks at first glance to 
be the right
thing to do.  But I do wonder how long rmb() has been lwsync
Since the {ppc,ppc64} -> powerpc merge.
quoted
and if as a practical matter that has caused any problems?
It has not as far as I know.
quoted
If this isn't causing any problems maybe there
is some loigic we are overlooking?
The I/O accessor functions enforce the necessary ordering
already I believe.
Ah, it looks like you might be right, IO should appear to go in-order, in
which case the rmb() would simply need to order cacheable loads. Interesting
way to do things... are drivers simply not up to scratch enough to allow
out of order IO?

Anyway, this raises another question -- if IO accessors have the right
ordering, why is wmb() not an lwsync as well? There appears to be many
more wmb() calls than rmb()...

Thanks,
Nick

Re: [patch 1/2] powerpc: rmb fix

From: Nick Piggin <hidden>
Date: 2007-08-22 03:15:12

On Tue, Aug 21, 2007 at 09:43:17PM +0200, Segher Boessenkool wrote:
quoted
quoted
#define mb()   __asm__ __volatile__ ("sync" : : : "memory")
-#define rmb()  __asm__ __volatile__ (__stringify(LWSYNC) : : : 
"memory")
+#define rmb()  __asm__ __volatile__ ("sync" : : : "memory")
#define wmb()  __asm__ __volatile__ ("sync" : : : "memory")
#define read_barrier_depends()  do { } while(0)
@@ -42,7 +42,7 @@
#ifdef __KERNEL__
#ifdef CONFIG_SMP
#define smp_mb()	mb()
-#define smp_rmb()	rmb()
+#define smp_rmb()	__asm__ __volatile__ (__stringify(LWSYNC) : : : 
"memory")
#define smp_wmb()	eieio()
#define smp_read_barrier_depends()	read_barrier_depends()
#else
I had to think about this one for awhile.  It looks at first glance to 
be the right
thing to do.  But I do wonder how long rmb() has been lwsync
Since the {ppc,ppc64} -> powerpc merge.
quoted
and if as a practical matter that has caused any problems?
It has not as far as I know.
quoted
If this isn't causing any problems maybe there
is some loigic we are overlooking?
The I/O accessor functions enforce the necessary ordering
already I believe.
Hmm, I never followed those discussions last year about IO ordering, and
I can't see where (if) it was documented anywhere :(

It appears that legacy code is handled by defining the old IO accessors to
be completely ordered, and introducing new __raw_ variants that are not
(OTOH, it seems like other architectures are implementing __raw prefix as
inorder unless there is a _relaxed postfix).

Drivers are definitely using these __raw_ accessors, and from a quick
look, they do appear to be hoping that *mb() is going to order access for
them.

Re: [patch 1/2] powerpc: rmb fix

From: Segher Boessenkool <hidden>
Date: 2007-08-22 03:30:18

quoted
quoted
If this isn't causing any problems maybe there
is some loigic we are overlooking?
The I/O accessor functions enforce the necessary ordering
already I believe.
Ah, it looks like you might be right, IO should appear to go in-order, 
in
which case the rmb() would simply need to order cacheable loads. 
Interesting
way to do things... are drivers simply not up to scratch enough to 
allow
out of order IO?
The powerpc kernel needs to have full sync insns in every I/O
accessor in order to enforce all the ordering rules Linux demands.
It's a bloody shame, but the alternative would be to make the
barriers lots more expensive.  A third alternative would be to
have barrier ops that do not order everything, but just A vs. B
for various choices of A and B (coherent accesses, MMIO accesses,
etc.)
Anyway, this raises another question -- if IO accessors have the right
ordering, why is wmb() not an lwsync as well? There appears to be many
more wmb() calls than rmb()...
Input MMIO accessors are {sync, load, stall pipeline until load came 
back}.
That's a full ordering on both sides.

Output MMIO on the other hand is done with {sync, store}.  Now since
wmb() has to order MMIO writes vs. main memory writes, we need a full
sync here.  On some (most, all?) CPUs an eieio is actually enough btw.
The barrier insn could be put at the end of all MMIO write ops too,
but I believe that would be more expensive (in execution time; in code
size it definitely would be, of course).


Segher

Re: [patch 1/2] powerpc: rmb fix

From: Segher Boessenkool <hidden>
Date: 2007-08-22 03:33:26

quoted
The I/O accessor functions enforce the necessary ordering
already I believe.
Hmm, I never followed those discussions last year about IO ordering, 
and
I can't see where (if) it was documented anywhere :(
The comments in system.h weren't updated with the last fix, I think.
It appears that legacy code is handled by defining the old IO 
accessors to
be completely ordered, and introducing new __raw_ variants that are not
(OTOH, it seems like other architectures are implementing __raw prefix 
as
inorder unless there is a _relaxed postfix).
__raw_XX() is for platform code only, which can do the needed
barriers without having to use the heavy hammer like everything
else unfortunately does.
Drivers are definitely using these __raw_ accessors, and from a quick
look, they do appear to be hoping that *mb() is going to order access 
for
them.
Which drivers?


Segher

Re: [patch 1/2] powerpc: rmb fix

From: Nick Piggin <hidden>
Date: 2007-08-22 03:55:44

On Wed, Aug 22, 2007 at 05:29:50AM +0200, Segher Boessenkool wrote:
quoted
quoted
quoted
If this isn't causing any problems maybe there
is some loigic we are overlooking?
The I/O accessor functions enforce the necessary ordering
already I believe.
Ah, it looks like you might be right, IO should appear to go in-order, 
in
which case the rmb() would simply need to order cacheable loads. 
Interesting
way to do things... are drivers simply not up to scratch enough to 
allow
out of order IO?
The powerpc kernel needs to have full sync insns in every I/O
accessor in order to enforce all the ordering rules Linux demands.
It's a bloody shame, but the alternative would be to make the
barriers lots more expensive.  A third alternative would be to
Well lots more expensive compared to what you have now. But what
you have now is like having those expensive barriers between
*every* io access.

have barrier ops that do not order everything, but just A vs. B
for various choices of A and B (coherent accesses, MMIO accesses,
etc.)
 
The non-smp_ variant is supposed to order everything, AFAIK. Maybe
you could get more fancy and have PIO vs MMIO etc etc. but it looks
like this whole area is in a pretty sticky state anyway so let's
not think about that.

quoted
Anyway, this raises another question -- if IO accessors have the right
ordering, why is wmb() not an lwsync as well? There appears to be many
more wmb() calls than rmb()...
Input MMIO accessors are {sync, load, stall pipeline until load came 
back}.
That's a full ordering on both sides.

Output MMIO on the other hand is done with {sync, store}.  Now since
wmb() has to order MMIO writes vs. main memory writes, we need a full
sync here.  On some (most, all?) CPUs an eieio is actually enough btw.
The barrier insn could be put at the end of all MMIO write ops too,
but I believe that would be more expensive (in execution time; in code
size it definitely would be, of course).
Ah, that explains why wmb() is a sync. Doesn't seem like a very good
idea though, if the rationale of having fully ordered IO accessors was
because drivers didn't have enough barriers in them.

Re: [patch 1/2] powerpc: rmb fix

From: Nick Piggin <hidden>
Date: 2007-08-22 04:05:11

On Wed, Aug 22, 2007 at 05:33:16AM +0200, Segher Boessenkool wrote:
quoted
quoted
The I/O accessor functions enforce the necessary ordering
already I believe.
Hmm, I never followed those discussions last year about IO ordering, 
and
I can't see where (if) it was documented anywhere :(
The comments in system.h weren't updated with the last fix, I think.
quoted
It appears that legacy code is handled by defining the old IO 
accessors to
be completely ordered, and introducing new __raw_ variants that are not
(OTOH, it seems like other architectures are implementing __raw prefix 
as
inorder unless there is a _relaxed postfix).
__raw_XX() is for platform code only, which can do the needed
barriers without having to use the heavy hammer like everything
else unfortunately does.

npiggin@nick:~/usr/src/linux-2.6/drivers> egrep '__raw_(write|read)' -r * | wc -l
685

 
quoted
Drivers are definitely using these __raw_ accessors, and from a quick
look, they do appear to be hoping that *mb() is going to order access 
for
them.
Which drivers?
There are maybe a dozen that use the raw accessors, and use non-smp_
memory barriers. I just looked at drivers/video/tgafb.c, which
indeed appears to intermix them.

Re: [patch 1/2] powerpc: rmb fix

From: Segher Boessenkool <hidden>
Date: 2007-08-23 17:49:29

quoted
quoted
Drivers are definitely using these __raw_ accessors, and from a quick
look, they do appear to be hoping that *mb() is going to order access
for
them.
Which drivers?
There are maybe a dozen that use the raw accessors, and use non-smp_
memory barriers. I just looked at drivers/video/tgafb.c, which
indeed appears to intermix them.
Hrm yeah.  It also looks like old buggy code that could use a
cleanup or two (or three or four).  I wonder if all __raw_ users
are like that?


Segher

Re: [patch 1/2] powerpc: rmb fix

From: Segher Boessenkool <hidden>
Date: 2007-08-23 17:57:39

quoted
The powerpc kernel needs to have full sync insns in every I/O
accessor in order to enforce all the ordering rules Linux demands.
It's a bloody shame, but the alternative would be to make the
barriers lots more expensive.  A third alternative would be to
Well lots more expensive compared to what you have now. But what
you have now is like having those expensive barriers between
*every* io access.
Yeah.  But I/O reads are very expensive anyway, and the barriers
are used for more than just I/O ordering.

I/O writes are a different thing; ideally, they would use only
eieio, if anything at all.

Maybe the tradeoff isn't optimal.  The I/O primitives didn't have
all those "sync"s in there before, they got added because some bad
interaction with spinlocks was discovered, if my memory isn't failing
me.
quoted
have barrier ops that do not order everything, but just A vs. B
for various choices of A and B (coherent accesses, MMIO accesses,
etc.)
The non-smp_ variant is supposed to order everything, AFAIK. Maybe
you could get more fancy and have PIO vs MMIO etc etc. but it looks
like this whole area is in a pretty sticky state anyway so let's
not think about that.
*Thinking* about it is fun.  Trying to get the code merged would be
a different thing ;-)


Segher

Re: [patch 1/2] powerpc: rmb fix

From: Nick Piggin <hidden>
Date: 2007-08-24 02:47:29

On Thu, Aug 23, 2007 at 07:57:20PM +0200, Segher Boessenkool wrote:
quoted
quoted
The powerpc kernel needs to have full sync insns in every I/O
accessor in order to enforce all the ordering rules Linux demands.
It's a bloody shame, but the alternative would be to make the
barriers lots more expensive.  A third alternative would be to
Well lots more expensive compared to what you have now. But what
you have now is like having those expensive barriers between
*every* io access.
Yeah.  But I/O reads are very expensive anyway, and the barriers
are used for more than just I/O ordering.
rmb() should only be used when IO ordering matters. smp_rmb() is
for regular ordering.

So doesn't the fact IO reads are very expensive anyway lend more
weight _to have_ the full IO ordering in rmb()?

I/O writes are a different thing; ideally, they would use only
eieio, if anything at all.
For IO to IO ordering, yes eieio would be ideal. I don't know that
there is really such a primitive for that in Linux. io_wmb().

 
Maybe the tradeoff isn't optimal.  The I/O primitives didn't have
all those "sync"s in there before, they got added because some bad
interaction with spinlocks was discovered, if my memory isn't failing
me.
I think it may have been because IO ops are pretty strongly ordered
on x86, and it was thought that a fair amount of code was relying on
that. So the old primitives were made quite strongly ordered, and
new ones were added to avoid the overhead. Unfortunately, you can't
actually use the unordered ones unless you have working barrier
instructions. Hence why I think rmb() should be an IO barrier.

But I'm not pushing this too hard. You guys all know the gory ppc
details better than I, so I'll just leave it with you to work out
whether it is the right thing to do.

quoted
quoted
have barrier ops that do not order everything, but just A vs. B
for various choices of A and B (coherent accesses, MMIO accesses,
etc.)
The non-smp_ variant is supposed to order everything, AFAIK. Maybe
you could get more fancy and have PIO vs MMIO etc etc. but it looks
like this whole area is in a pretty sticky state anyway so let's
not think about that.
*Thinking* about it is fun.  Trying to get the code merged would be
a different thing ;-)
;)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help