Skip to contentSkip to conversation

: Improve atomic.h robustness

PATCH In review Joshua Kinard 2 participants 2 messages Jun 25, 2012 – Aug 14, 2012

Subsystems (2)
MIPS Atomic infrastructure
lore
Share & tools
lore

Joshua Kinard

author
#0 lore raw

PATCH 1/1 : Improve atomic.h robustness

I've maintained this patch, originally from Thiemo Seufer in 2004, for a really long time, but I think it's time for it to get a look at for possible inclusion. I have had no problems with it across various SGI systems over the years.

To quote the post here:
http://www.linux-mips.org/archives/linux-mips/2004-12/msg00000.html

"the atomic functions use so far memory references for the inline assembler to access the semaphore. This can lead to additional instructions in the ll/sc loop, because newer compilers don't expand the memory reference any more but leave it to the assembler.

The appended patch uses registers instead, and makes the ll/sc arguments more explicit. In some cases it will lead also to better register scheduling because the register isn't bound to an output any more."

Signed-off-by Joshua Kinard <[email protected]>

---

 atomic.h |   64 ++++++++++++++++++++++++++++-----------------------------------
 1 file changed, 29 insertions(+), 35 deletions(-)
arch/mips/include/asm/atomic.h +29 -35
diff -Naurp a/arch/mips/include/asm/atomic.h b/arch/mips/include/asm/atomic.h
@@ -59,8 +59,8 @@ static __inline__ void atomic_add(int i,
 		"	sc	%0, %1					\n"
 		"	beqzl	%0, 1b					\n"
 		"	.set	mips0					\n"
-		: "=&r" (temp), "=m" (v->counter)
-		: "Ir" (i), "m" (v->counter));
+		: "=&r" (temp), "+m" (v->counter)
+		: "Ir" (i));
 	} else if (kernel_uses_llsc) {
 		int temp;
 

@@ -71,8 +71,8 @@ static __inline__ void atomic_add(int i,
 			"	addu	%0, %2				\n"
 			"	sc	%0, %1				\n"
 			"	.set	mips0				\n"
-			: "=&r" (temp), "=m" (v->counter)
-			: "Ir" (i), "m" (v->counter));
+			: "=&r" (temp), "+m" (v->counter)
+			: "Ir" (i));
 		} while (unlikely(!temp));
 	} else {
 		unsigned long flags;

@@ -102,8 +102,8 @@ static __inline__ void atomic_sub(int i,
 		"	sc	%0, %1					\n"
 		"	beqzl	%0, 1b					\n"
 		"	.set	mips0					\n"
-		: "=&r" (temp), "=m" (v->counter)
-		: "Ir" (i), "m" (v->counter));
+		: "=&r" (temp), "+m" (v->counter)
+		: "Ir" (i));
 	} else if (kernel_uses_llsc) {
 		int temp;
 

@@ -114,8 +114,8 @@ static __inline__ void atomic_sub(int i,
 			"	subu	%0, %2				\n"
 			"	sc	%0, %1				\n"
 			"	.set	mips0				\n"
-			: "=&r" (temp), "=m" (v->counter)
-			: "Ir" (i), "m" (v->counter));
+			: "=&r" (temp), "+m" (v->counter)
+			: "Ir" (i));
 		} while (unlikely(!temp));
 	} else {
 		unsigned long flags;

@@ -146,9 +146,8 @@ static __inline__ int atomic_add_return(
 		"	beqzl	%0, 1b					\n"
 		"	addu	%0, %1, %3				\n"
 		"	.set	mips0					\n"
-		: "=&r" (result), "=&r" (temp), "=m" (v->counter)
-		: "Ir" (i), "m" (v->counter)
-		: "memory");
+		: "=&r" (result), "=&r" (temp), "+m" (v->counter)
+		: "Ir" (i));
 	} else if (kernel_uses_llsc) {
 		int temp;
 

@@ -159,9 +158,8 @@ static __inline__ int atomic_add_return(
 			"	addu	%0, %1, %3			\n"
 			"	sc	%0, %2				\n"
 			"	.set	mips0				\n"
-			: "=&r" (result), "=&r" (temp), "=m" (v->counter)
-			: "Ir" (i), "m" (v->counter)
-			: "memory");
+			: "=&r" (result), "=&r" (temp), "+m" (v->counter)
+			: "Ir" (i));
 		} while (unlikely(!result));
 
 		result = temp + i;

@@ -212,9 +210,8 @@ static __inline__ int atomic_sub_return(
 			"	subu	%0, %1, %3			\n"
 			"	sc	%0, %2				\n"
 			"	.set	mips0				\n"
-			: "=&r" (result), "=&r" (temp), "=m" (v->counter)
-			: "Ir" (i), "m" (v->counter)
-			: "memory");
+			: "=&r" (result), "=&r" (temp), "+m" (v->counter)
+			: "Ir" (i));
 		} while (unlikely(!result));
 
 		result = temp - i;

@@ -262,7 +259,7 @@ static __inline__ int atomic_sub_if_posi
 		"	.set	reorder					\n"
 		"1:							\n"
 		"	.set	mips0					\n"
-		: "=&r" (result), "=&r" (temp), "=m" (v->counter)
+		: "=&r" (result), "=&r" (temp), "+m" (v->counter)
 		: "Ir" (i), "m" (v->counter)
 		: "memory");
 	} else if (kernel_uses_llsc) {

@@ -280,9 +277,8 @@ static __inline__ int atomic_sub_if_posi
 		"	.set	reorder					\n"
 		"1:							\n"
 		"	.set	mips0					\n"
-		: "=&r" (result), "=&r" (temp), "=m" (v->counter)
-		: "Ir" (i), "m" (v->counter)
-		: "memory");
+		: "=&r" (result), "=&r" (temp), "+m" (v->counter)
+		: "Ir" (i));
 	} else {
 		unsigned long flags;
 

@@ -430,8 +426,8 @@ static __inline__ void atomic64_add(long
 		"	scd	%0, %1					\n"
 		"	beqzl	%0, 1b					\n"
 		"	.set	mips0					\n"
-		: "=&r" (temp), "=m" (v->counter)
-		: "Ir" (i), "m" (v->counter));
+		: "=&r" (temp), "+m" (v->counter)
+		: "Ir" (i));
 	} else if (kernel_uses_llsc) {
 		long temp;
 

@@ -442,8 +438,8 @@ static __inline__ void atomic64_add(long
 			"	daddu	%0, %2				\n"
 			"	scd	%0, %1				\n"
 			"	.set	mips0				\n"
-			: "=&r" (temp), "=m" (v->counter)
-			: "Ir" (i), "m" (v->counter));
+			: "=&r" (temp), "+m" (v->counter)
+			: "Ir" (i));
 		} while (unlikely(!temp));
 	} else {
 		unsigned long flags;

@@ -473,8 +469,8 @@ static __inline__ void atomic64_sub(long
 		"	scd	%0, %1					\n"
 		"	beqzl	%0, 1b					\n"
 		"	.set	mips0					\n"
-		: "=&r" (temp), "=m" (v->counter)
-		: "Ir" (i), "m" (v->counter));
+		: "=&r" (temp), "+m" (v->counter)
+		: "Ir" (i));
 	} else if (kernel_uses_llsc) {
 		long temp;
 

@@ -485,8 +481,8 @@ static __inline__ void atomic64_sub(long
 			"	dsubu	%0, %2				\n"
 			"	scd	%0, %1				\n"
 			"	.set	mips0				\n"
-			: "=&r" (temp), "=m" (v->counter)
-			: "Ir" (i), "m" (v->counter));
+			: "=&r" (temp), "+m" (v->counter)
+			: "Ir" (i));
 		} while (unlikely(!temp));
 	} else {
 		unsigned long flags;

@@ -517,9 +513,8 @@ static __inline__ long atomic64_add_retu
 		"	beqzl	%0, 1b					\n"
 		"	daddu	%0, %1, %3				\n"
 		"	.set	mips0					\n"
-		: "=&r" (result), "=&r" (temp), "=m" (v->counter)
-		: "Ir" (i), "m" (v->counter)
-		: "memory");
+		: "=&r" (result), "=&r" (temp), "+m" (v->counter)
+		: "Ir" (i));
 	} else if (kernel_uses_llsc) {
 		long temp;
 

@@ -649,9 +644,8 @@ static __inline__ long atomic64_sub_if_p
 		"	.set	reorder					\n"
 		"1:							\n"
 		"	.set	mips0					\n"
-		: "=&r" (result), "=&r" (temp), "=m" (v->counter)
-		: "Ir" (i), "m" (v->counter)
-		: "memory");
+		: "=&r" (result), "=&r" (temp), "+m" (v->counter)
+		: "Ir" (i));
 	} else {
 		unsigned long flags;
#1 lore raw
Quoted 13 lines · Joshua Kinard

I've maintained this patch, originally from Thiemo Seufer in 2004, for a really long time, but I think it's time for it to get a look at for possible inclusion. I have had no problems with it across various SGI systems over the years.

To quote the post here:
http://www.linux-mips.org/archives/linux-mips/2004-12/msg00000.html

"the atomic functions use so far memory references for the inline assembler to access the semaphore. This can lead to additional instructions in the ll/sc loop, because newer compilers don't expand the memory reference any more but leave it to the assembler.

Quoted 4 lines

The appended patch uses registers instead, and makes the ll/sc arguments more explicit. In some cases it will lead also to better register scheduling because the register isn't bound to an output any more."

I have faint memories of having tried this myself and very ancient compilers didn't like the + constraint in inline assembler; somewhat less ancient compilers did generate slightly bigger code. With gcc 4.7 I am getting exactly the same codesize as without your patch applied. The patch shouldn't do anything to robustness but sureles makes the inline assembler a bit more readable so I'm queueing this for the next release.

Thanks,

Ralf