Fix some (maybe) missing syncs in bitops.h

Fix some (maybe) missing syncs in bitops.h

[Daniel Jacobowitz]

If I'm reading the broadcom documentation right, the semantics of set_bit
and test_and_set_bit require a sync at the end on this architecture.

I've been trying to track down a nasty signal delivery bug that I thought
was a TIF_SIGPENDING not being visible on the other CPU early enough.  Turns
out that wasn't the problem, but I still think the syncs are correct, so I'm
posting the patch.

Signed-off-by: Daniel Jacobowitz <dan@codesourcery.com>

Index: linux/include/asm-mips/bitops.h
===================================================================
--- linux.orig/include/asm-mips/bitops.h	2005-01-20 16:31:45.921742674 -0500
+++ linux/include/asm-mips/bitops.h	2005-01-20 19:56:37.420056584 -0500
@@ -34,6 +34,7 @@
 #include <asm/interrupt.h>
 #include <asm/sgidefs.h>
 #include <asm/war.h>
+#include <asm/system.h>
 
 /*
  * clear_bit() doesn't provide any barrier for the compiler.
@@ -76,6 +77,9 @@
 		"	or	%0, %2					\n"
 		"	"__SC	"%0, %1					\n"
 		"	beqzl	%0, 1b					\n"
+#ifdef CONFIG_SMP
+		"sync							\n"
+#endif
 		: "=&r" (temp), "=m" (*m)
 		: "ir" (1UL << (nr & SZLONG_MASK)), "m" (*m));
 	} else if (cpu_has_llsc) {
@@ -84,6 +88,9 @@
 		"	or	%0, %2					\n"
 		"	"__SC	"%0, %1					\n"
 		"	beqz	%0, 1b					\n"
+#ifdef CONFIG_SMP
+		"sync							\n"
+#endif
 		: "=&r" (temp), "=m" (*m)
 		: "ir" (1UL << (nr & SZLONG_MASK)), "m" (*m));
 	} else {
@@ -195,6 +204,9 @@
 		"	xor	%0, %2				\n"
 		"	"__SC	"%0, %1				\n"
 		"	beqzl	%0, 1b				\n"
+#ifdef CONFIG_SMP
+		"sync						\n"
+#endif
 		: "=&r" (temp), "=m" (*m)
 		: "ir" (1UL << (nr & SZLONG_MASK)), "m" (*m));
 	} else if (cpu_has_llsc) {
@@ -206,6 +218,9 @@
 		"	xor	%0, %2				\n"
 		"	"__SC	"%0, %1				\n"
 		"	beqz	%0, 1b				\n"
+#ifdef CONFIG_SMP
+		"sync						\n"
+#endif
 		: "=&r" (temp), "=m" (*m)
 		: "ir" (1UL << (nr & SZLONG_MASK)), "m" (*m));
 	} else {


-- 
Daniel Jacobowitz

Re: Fix some (maybe) missing syncs in bitops.h

[Ralf Baechle]

On Thu, Jan 20, 2005 at 08:04:03PM -0500, Daniel Jacobowitz wrote:

> If I'm reading the broadcom documentation right, the semantics of set_bit
> and test_and_set_bit require a sync at the end on this architecture.

Linux semantics of the bit functions are less than obvious.  The functions
set_bit, change_bit and clear_bit may be atomic but they don't have memory
barrier semantics, that is memory accesses before the function call may be
reordered to be executed after the function has been completed or vice
versa.  The test_and_{set,clear,change}_bit functions however have memory
barrier semantics.  The intended use is something like:

	if (!test_and_set_bit(bitnr, bitmap)) {
		/* we got the bit */

		... do something ...

		smp_mb__before_clear_bit();
		clear_bit(bitnr, bitmap);
		smp_mb__after_clear_bit();
	} else
		printk("Bit was already set by somebody else\n");

> I've been trying to track down a nasty signal delivery bug that I thought
> was a TIF_SIGPENDING not being visible on the other CPU early enough.  Turns
> out that wasn't the problem, but I still think the syncs are correct, so I'm
> posting the patch.

And the code indeed seems to be ok as it is.

  Ralf

Re: Fix some (maybe) missing syncs in bitops.h

[Daniel Jacobowitz]

On Mon, Jan 24, 2005 at 09:05:35PM +0000, Ralf Baechle wrote:
> On Thu, Jan 20, 2005 at 08:04:03PM -0500, Daniel Jacobowitz wrote:
> 
> > If I'm reading the broadcom documentation right, the semantics of set_bit
> > and test_and_set_bit require a sync at the end on this architecture.
> 
> Linux semantics of the bit functions are less than obvious.  The functions
> set_bit, change_bit and clear_bit may be atomic but they don't have memory
> barrier semantics, that is memory accesses before the function call may be
> reordered to be executed after the function has been completed or vice
> versa.  The test_and_{set,clear,change}_bit functions however have memory
> barrier semantics.  The intended use is something like:
> 
> 	if (!test_and_set_bit(bitnr, bitmap)) {
> 		/* we got the bit */
> 
> 		... do something ...
> 
> 		smp_mb__before_clear_bit();
> 		clear_bit(bitnr, bitmap);
> 		smp_mb__after_clear_bit();
> 	} else
> 		printk("Bit was already set by somebody else\n");

I know that clear_bit has these semantics.  But are you sure about
set_bit/change_bit?  The comments in clear_bit in every bitops.h
explicitly say it doesn't have a memory barrier, but those on set_bit
don't.  At least some platforms use acquire semantics.

I don't see where there might be a barrier on the signal_wake_up path
after the flag is set, but since the patch didn't fix my problems,
you're probably right that there is one somewhere :-)

-- 
Daniel Jacobowitz