[patch] linux: mb() and friends again

[patch] linux: mb() and friends again

[Maciej W. Rozycki]

Hello,

 There was a discussion some time ago upon my proposal of a clean-up of
mb() and related macros.  The conclusion was a rewrite is desireable, but
the patch wasn't accepted and no alternative was proposed.

 The need for the update is more and more crucial for me as I have more
and more code to apply to the tree that needs the macros be set somehow. 
If there is a design flaw, I'd like to know of it, if there is an
implementation problem, I'm willing to fix it.  I'm aware about peripheral
bus-specific coherency problems -- the patch is orthogonal to them and
only addresses the host bus.

 Here is an updated version I'm using currently.  It's fixed to apply
against the current tree and adds <asm-mips64/wbflush.h> for source code
compatibility.

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

patch-mips-2.4.18-20020530-mb-wb-8
diff -up --recursive --new-file linux-mips-2.4.18-20020530.macro/arch/mips/config.in linux-mips-2.4.18-20020530/arch/mips/config.in
--- linux-mips-2.4.18-20020530.macro/arch/mips/config.in	2002-05-30 02:57:35.000000000 +0000
+++ linux-mips-2.4.18-20020530/arch/mips/config.in	2002-06-02 15:51:03.000000000 +0000
@@ -393,6 +393,11 @@ else
       fi
    fi
 fi
+if [ "$CONFIG_CPU_R3000" = "y" ]; then
+   define_bool CONFIG_CPU_HAS_SYNC n
+else
+   define_bool CONFIG_CPU_HAS_SYNC y
+fi
 endmenu
 
 mainmenu_option next_comment
diff -up --recursive --new-file linux-mips-2.4.18-20020530.macro/include/asm-mips/system.h linux-mips-2.4.18-20020530/include/asm-mips/system.h
--- linux-mips-2.4.18-20020530.macro/include/asm-mips/system.h	2002-05-28 10:13:21.000000000 +0000
+++ linux-mips-2.4.18-20020530/include/asm-mips/system.h	2002-06-02 15:51:03.000000000 +0000
@@ -18,9 +18,12 @@
 
 #include <linux/config.h>
 #include <asm/sgidefs.h>
-#include <asm/ptrace.h>
+
 #include <linux/kernel.h>
 
+#include <asm/addrspace.h>
+#include <asm/ptrace.h>
+
 __asm__ (
 	".macro\t__sti\n\t"
 	".set\tpush\n\t"
@@ -166,32 +169,58 @@ extern void __global_restore_flags(unsig
 #define local_irq_disable()	__cli()
 #define local_irq_enable()	__sti()
 
-/*
- * These are probably defined overly paranoid ...
- */
+#ifdef CONFIG_CPU_HAS_SYNC
+#define __sync()				\
+	__asm__ __volatile__(			\
+		".set	push\n\t"		\
+		".set	noreorder\n\t"		\
+		".set	mips2\n\t"		\
+		"sync\n\t"			\
+		".set	pop"			\
+		: /* no output */		\
+		: /* no input */		\
+		: "memory")
+#else
+#define __sync()	do { } while(0)
+#endif
+
+#define __fast_iob()				\
+	__asm__ __volatile__(			\
+		".set	push\n\t"		\
+		".set	noreorder\n\t"		\
+		"lw	$0,%0\n\t"		\
+		"nop\n\t"			\
+		".set	pop"			\
+		: /* no output */		\
+		: "m" (*(int *)KSEG1)		\
+		: "memory")
+
+#define fast_wmb()	__sync()
+#define fast_rmb()	__sync()
+#define fast_mb()	__sync()
+#define fast_iob()				\
+	do {					\
+		__sync();			\
+		__fast_iob();			\
+	} while (0)
+
 #ifdef CONFIG_CPU_HAS_WB
 
 #include <asm/wbflush.h>
-#define rmb()	do { } while(0)
-#define wmb()	wbflush()
-#define mb()	wbflush()
-
-#else /* CONFIG_CPU_HAS_WB  */
-
-#define mb()						\
-__asm__ __volatile__(					\
-	"# prevent instructions being moved around\n\t"	\
-	".set\tnoreorder\n\t"				\
-	"# 8 nops to fool the R4400 pipeline\n\t"	\
-	"nop;nop;nop;nop;nop;nop;nop;nop\n\t"		\
-	".set\treorder"					\
-	: /* no output */				\
-	: /* no input */				\
-	: "memory")
-#define rmb() mb()
-#define wmb() mb()
 
-#endif /* CONFIG_CPU_HAS_WB  */
+#define wmb()		fast_wmb()
+#define rmb()		fast_rmb()
+#define mb()		wbflush();
+#define iob()		wbflush();
+
+#else /* !CONFIG_CPU_HAS_WB */
+
+#define wmb()		fast_wmb()
+#define rmb()		fast_rmb()
+#define mb()		fast_mb()
+#define iob()		fast_iob()
+
+#endif /* !CONFIG_CPU_HAS_WB */
 
 #ifdef CONFIG_SMP
 #define smp_mb()	mb()
diff -up --recursive --new-file linux-mips-2.4.18-20020530.macro/include/asm-mips/wbflush.h linux-mips-2.4.18-20020530/include/asm-mips/wbflush.h
--- linux-mips-2.4.18-20020530.macro/include/asm-mips/wbflush.h	2001-09-07 04:26:33.000000000 +0000
+++ linux-mips-2.4.18-20020530/include/asm-mips/wbflush.h	2002-02-04 02:52:11.000000000 +0000
@@ -6,29 +6,30 @@
  * for more details.
  *
  * Copyright (c) 1998 Harald Koerfgen
+ * Copyright (C) 2002 Maciej W. Rozycki
  */
 #ifndef __ASM_MIPS_WBFLUSH_H
 #define __ASM_MIPS_WBFLUSH_H
 
 #include <linux/config.h>
 
-#if defined(CONFIG_CPU_HAS_WB)
-/*
- * R2000 or R3000
- */
-extern void (*__wbflush) (void);
+#ifdef CONFIG_CPU_HAS_WB
 
-#define wbflush() __wbflush()
+extern void (*__wbflush)(void);
+extern void wbflush_setup(void);
 
-#else
-/*
- * we don't need no stinkin' wbflush
- */
+#define wbflush()			\
+	do {				\
+		__sync();		\
+		__wbflush();		\
+	} while (0)
 
-#define wbflush()  do { } while(0)
+#else /* !CONFIG_CPU_HAS_WB */
 
-#endif
+#define wbflush_setup() do { } while (0)
 
-extern void wbflush_setup(void);
+#define wbflush() fast_iob()
+
+#endif /* !CONFIG_CPU_HAS_WB */
 
 #endif /* __ASM_MIPS_WBFLUSH_H */
diff -up --recursive --new-file linux-mips-2.4.18-20020530.macro/include/asm-mips64/system.h linux-mips-2.4.18-20020530/include/asm-mips64/system.h
--- linux-mips-2.4.18-20020530.macro/include/asm-mips64/system.h	2002-05-28 10:13:22.000000000 +0000
+++ linux-mips-2.4.18-20020530/include/asm-mips64/system.h	2002-06-02 15:55:32.000000000 +0000
@@ -12,9 +12,12 @@
 
 #include <linux/config.h>
 #include <asm/sgidefs.h>
-#include <asm/ptrace.h>
+
 #include <linux/kernel.h>
 
+#include <asm/addrspace.h>
+#include <asm/ptrace.h>
+
 __asm__ (
 	".macro\t__sti\n\t"
 	".set\tpush\n\t"
@@ -161,20 +164,37 @@ extern void __global_restore_flags(unsig
 #define local_irq_disable()	__cli()
 #define local_irq_enable()	__sti()
 
-/*
- * These are probably defined overly paranoid ...
- */
-#define mb()						\
-__asm__ __volatile__(					\
-	"# prevent instructions being moved around\n\t"	\
-	".set\tnoreorder\n\t"				\
-	"sync\n\t"					\
-	".set\treorder"					\
-	: /* no output */				\
-	: /* no input */				\
-	: "memory")
-#define rmb() mb()
-#define wmb() mb()
+#define __sync()				\
+	__asm__ __volatile__(			\
+		".set	push\n\t"		\
+		".set	noreorder\n\t"		\
+		"sync\n\t"			\
+		".set	pop"			\
+		: /* no output */		\
+		: /* no input */		\
+		: "memory")
+
+#define fast_wmb()	__sync()
+#define fast_rmb()	__sync()
+#define fast_mb()	__sync()
+#define fast_iob()				\
+	do {					\
+		__sync();			\
+		__asm__ __volatile__(		\
+			".set	push\n\t"	\
+			".set	noreorder\n\t"	\
+			"lw	$0,%0\n\t"	\
+			"nop\n\t"		\
+			".set	pop"		\
+			: /* no output */	\
+			: "m" (*(int *)KSEG1)	\
+			: "memory");		\
+	} while (0)
+
+#define wmb()		fast_wmb()
+#define rmb()		fast_rmb()
+#define mb()		fast_mb()
+#define iob()		fast_iob()
 
 #ifdef CONFIG_SMP
 #define smp_mb()	mb()
diff -up --recursive --new-file linux-mips-2.4.18-20020530.macro/include/asm-mips64/wbflush.h linux-mips-2.4.18-20020530/include/asm-mips64/wbflush.h
--- linux-mips-2.4.18-20020530.macro/include/asm-mips64/wbflush.h	1970-01-01 00:00:00.000000000 +0000
+++ linux-mips-2.4.18-20020530/include/asm-mips64/wbflush.h	2002-05-31 11:43:50.000000000 +0000
@@ -0,0 +1,18 @@
+/*
+ * Header file for using the wbflush routine
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * Copyright (c) 1998 Harald Koerfgen
+ * Copyright (C) 2002 Maciej W. Rozycki
+ */
+#ifndef __ASM_MIPS64_WBFLUSH_H
+#define __ASM_MIPS64_WBFLUSH_H
+
+#define wbflush_setup() do { } while (0)
+
+#define wbflush() fast_iob()
+
+#endif /* __ASM_MIPS64_WBFLUSH_H */

Re: [patch] linux: mb() and friends again

[Gleb O. Raiko]

Maciej,

In previous version of your patch there was the change in mm/c-r3k.c:

static void r3k_dma_cache_wback_inv(unsigned long start, unsigned long
size)
{
-       wbflush();
+       iob();
        r3k_flush_dcache_range(start, start + size);
}

Why did you drop it? It's definetely required.

While you patch operates in unusual terms from hw point of view, it does
right thins by stating that external wbs do differ from internal wb.

Regards,
Gleb.

Ah. The patch shall be applied, certainly.

Re: [patch] linux: mb() and friends again

[Maciej W. Rozycki]

On Tue, 4 Jun 2002, Gleb O. Raiko wrote:

> In previous version of your patch there was the change in mm/c-r3k.c:
> 
> static void r3k_dma_cache_wback_inv(unsigned long start, unsigned long
> size)
> {
> -       wbflush();
> +       iob();
>         r3k_flush_dcache_range(start, start + size);
> }
> 
> Why did you drop it? It's definetely required.

 Nope, it wasn't dropped.  It's included in a different patch, namely
"patch-mips-2.4.18-20020412-wbflush-5".  The patch depends on the
"patch-mips-2.4.18-20020530-mb-wb-8" one, so I am not going to resubmit
the former one for discussion here until (unless) we decide on the latter
one.

> While you patch operates in unusual terms from hw point of view, it does
> right thins by stating that external wbs do differ from internal wb.

 What do you mean by "unusual terms"?  The names of the macros?  Well,
they are based on what's used for other platforms and if treated as
abstract names (as they should be) they actually reflect reality. 

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +

Re: [patch] linux: mb() and friends again

[Gleb O. Raiko]

"Maciej W. Rozycki" wrote:
> > Why did you drop it? It's definetely required.
> 
>  Nope, it wasn't dropped.  It's included in a different patch, namely
> "patch-mips-2.4.18-20020412-wbflush-5".  The patch depends on the
> "patch-mips-2.4.18-20020530-mb-wb-8" one, so I am not going to resubmit
> the former one for discussion here until (unless) we decide on the latter
> one.

Don't forget the latter one. :-)

> 
> > While you patch operates in unusual terms from hw point of view, it does
> > right thins by stating that external wbs do differ from internal wb.
> 
>  What do you mean by "unusual terms"?  The names of the macros?  Well,
> they are based on what's used for other platforms and if treated as
> abstract names (as they should be) they actually reflect reality.
> 

Basically, the patch logically allows combination of a CPU with internal
write-buffer and an external wb chip. It's impossible if hw designers
don't smoke hard. :-)

In fact, CONFIG_CPU_HAS_WB means !CONFIG_CPU_HAS_WB, i.e. CPU don't have
built-in write-buffer logic and there is an external write-buffer chip
somewhere in the box.
("Somewhere" means a place on the path from the local bus to a memory
controller.)

Then, __fast_iob just flushes internal wb while wbflush flushes an
external wb.

That's why I call it "unusual terms from hw POV". 

But, don't reimplement the patch, please. It's OK. Just from software
point of view.

Regards,
Gleb.

Re: [patch] linux: mb() and friends again

[Maciej W. Rozycki]

On Tue, 4 Jun 2002, Gleb O. Raiko wrote:

> Basically, the patch logically allows combination of a CPU with internal
> write-buffer and an external wb chip. It's impossible if hw designers
> don't smoke hard. :-)

 Well, I believe it might be useful -- if a CPU uses a higher clock for
its pipeline and a lower one for its external bus, it might be useful to
buffer a few words internally to avoid stalls at two consecutive writes.
Then you may have an additional buffer externally to lower the number of
stalls on memory or I/O (generally the rest of a system) accesses.
Essentially a buffer every time you cross a frequency domains' border,
leaving the faster one.  You need at least a single-word buffer at each
such border anyway if you don't want to stall the whole system for any
cycle accessing slower domains. 

 Consider it a complement of a hierarchical cache organization -- I've
seen (and actually used) systems with up to three levels of caches. 

> In fact, CONFIG_CPU_HAS_WB means !CONFIG_CPU_HAS_WB, i.e. CPU don't have
> built-in write-buffer logic and there is an external write-buffer chip
> somewhere in the box.
> ("Somewhere" means a place on the path from the local bus to a memory
> controller.)

 That's a bit awkward possibly, but "has" has a wide meaning and does not
necessarily state "contains".  It might mean "owns" as well.  For R[23]k
CPUs the option originally corresponded to external R2020 and R3220 chips
(just like floating point units may be external but still be considered a
part of a CPU) that were treated as a part of coprocessor 0 (with "bc0f"
or "bc0t" instructions testing their state). 

 Besides, who says discrete CPUs are forbidden? ;-)

> Then, __fast_iob just flushes internal wb while wbflush flushes an
> external wb.

 Well, that's used by __wbflush internally if it knows there is no
external buffer that needs explicit handling (for the DECstation, at least
-- other systems might make use of it as well).  That's unused in this
patch but is needed in the other one -- well, I had to split these patches
logically somehow and this one only contains system-independent code.

> That's why I call it "unusual terms from hw POV". 

 Hopefully, I clarified the terms a bit.

  Maciej

-- 
+  Maciej W. Rozycki, Technical University of Gdansk, Poland   +
+--------------------------------------------------------------+
+        e-mail: macro@ds2.pg.gda.pl, PGP key available        +