[PATCH] ARM: fix badly implementation of wmb

[PATCH] ARM: fix badly implementation of wmb

[tom.leiming at gmail.com]

From: Ming Lei <tom.leiming@gmail.com>

Commit e7c5650f6067f65f8e961394f376d4862808d0d2

	ARM: 5996/1: ARM: Change the mandatory barriers implementation (4/4)

implements wmb as dsb plus outer_sync, which will make wmb
much more strict than required. In fact, it is enough for dmb to
keep the partial order of two stores, so restore wmb as dmb to
fix possible performance degrade caused by the commit e7c5650f.

Also the patch defines __iowmb as mb to fix the issue which the
commit e7c5650f addressed.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 arch/arm/include/asm/io.h     |    2 +-
 arch/arm/include/asm/system.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index d66605d..b2cd9f2 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -98,7 +98,7 @@ static inline void __iomem *__typesafe_io(unsigned long addr)
 /* IO barriers */
 #ifdef CONFIG_ARM_DMA_MEM_BUFFERABLE
 #define __iormb()		rmb()
-#define __iowmb()		wmb()
+#define __iowmb()		mb()
 #else
 #define __iormb()		do { } while (0)
 #define __iowmb()		do { } while (0)
diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index 885be09..1ec2c25 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -160,7 +160,7 @@ extern unsigned int user_debug;
 #elif defined(CONFIG_ARM_DMA_MEM_BUFFERABLE) || defined(CONFIG_SMP)
 #define mb()		do { dsb(); outer_sync(); } while (0)
 #define rmb()		dmb()
-#define wmb()		mb()
+#define wmb()		dmb()
 #else
 #include <asm/memory.h>
 #define mb()	do { if (arch_is_coherent()) dmb(); else barrier(); } while (0)
-- 
1.7.3

[PATCH] ARM: fix badly implementation of wmb

[Russell King - ARM Linux]

On Sat, Apr 16, 2011 at 12:21:51AM +0800, tom.leiming at gmail.com wrote:
> From: Ming Lei <tom.leiming@gmail.com>
> 
> Commit e7c5650f6067f65f8e961394f376d4862808d0d2
> 
> 	ARM: 5996/1: ARM: Change the mandatory barriers implementation (4/4)
> 
> implements wmb as dsb plus outer_sync, which will make wmb
> much more strict than required. In fact, it is enough for dmb to
> keep the partial order of two stores, so restore wmb as dmb to
> fix possible performance degrade caused by the commit e7c5650f.
> 
> Also the patch defines __iowmb as mb to fix the issue which the
> commit e7c5650f addressed.

So I have another patch which changes rmb() from being a dmb to a dsb.

With these two patches combined, we end up with:

rmb() = dsb()
wmb() = dmb()

which makes rmb() stronger than wmb(), which means there's something
wrong.  rmb() should never be stronger than wmb().

[PATCH] ARM: fix badly implementation of wmb

[Catalin Marinas]

On 15 April 2011 19:21,  <tom.leiming@gmail.com> wrote:
> From: Ming Lei <tom.leiming@gmail.com>
>
> Commit e7c5650f6067f65f8e961394f376d4862808d0d2
>
> ? ? ? ?ARM: 5996/1: ARM: Change the mandatory barriers implementation (4/4)
>
> implements wmb as dsb plus outer_sync, which will make wmb
> much more strict than required. In fact, it is enough for dmb to
> keep the partial order of two stores, so restore wmb as dmb to
> fix possible performance degrade caused by the commit e7c5650f.
>
> Also the patch defines __iowmb as mb to fix the issue which the
> commit e7c5650f addressed.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Ming Lei <tom.leiming@gmail.com>

NAK.

I thought that after the whole discussion around my rmb() patch it was
clear that we need a dsb for both wmb and rmb.

Three points:

1. Linux I/O accessors must be ordered in relation to Normal
non-cacheable memory (DMA buffers) accesses (Linus' requirement - they
should behave like x86). Therefore they need DSBs around them.

2. You can use relaxed I/O accessors with explicit mb()/wmb()/rmb().
In this case, you also need DSB in the mandatory barriers.

3. Because Normal non-cacheable memory acceses are buffered at the L2
cache level, we need to drain its buffer in the mb()/wmb() case, hence
the outer_sync() call.

If you find alternatives to the points above I may agree with your patch.

Please note that there have been several discussions on LKML around
barriers for I/O vs Normal memory and the consensus was *not* to
introduce additional barrier types since device drivers don't use them
anyway.

Can you show many situations where you would need mandatory barriers
but not in relation to I/O?

-- 
Catalin

[PATCH] ARM: fix badly implementation of wmb

[Ming Lei]

Hi Catalin,

2011/4/17 Catalin Marinas <catalin.marinas@arm.com>:
> 3. Because Normal non-cacheable memory acceses are buffered at the L2
> cache level, we need to drain its buffer in the mb()/wmb() case, hence
> the outer_sync() call.

If you confirmed dsb plus outer_sync are needed for this case, the patch
is surely pointless. In fact, I thought dmb is enough for this case,
so introduce
the patch.  Now you have clarified it, please drop it.

>
> If you find alternatives to the points above I may agree with your patch.
>
> Please note that there have been several discussions on LKML around
> barriers for I/O vs Normal memory and the consensus was *not* to
> introduce additional barrier types since device drivers don't use them
> anyway.
>
> Can you show many situations where you would need mandatory barriers
> but not in relation to I/O?

No, thank you for the explanation, sorry for the noise.

thanks,
-- 
Ming Lei