Fix some (maybe) missing syncs in bitops.h

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.

Index: linux/include/asm-mips/bitops.h
===================================================================
@@ -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

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");

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

Ralf

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