Skip to contentSkip to conversation

x86: simplify sync_test_bit()

PATCH Applied Jan Beulich 3 participants 7 messages Mar 14, 2008 – Mar 26, 2008

lore
Share & tools
lore

Jan Beulich

author
#0 lore raw

PATCH 1/1 x86: simplify sync_test_bit()

There really is no need for a redundant implementation here, just keep the alternative name for allowing consumers to use consistent naming.

Signed-off-by Jan Beulich <[email protected]>
include/asm-x86/sync_bitops.h +1 -20
@@ -130,26 +130,7 @@ static inline int sync_test_and_change_b
 	return oldbit;
 }
 
-static __always_inline int sync_constant_test_bit(int nr, const volatile unsigned long *addr)
-{
-	return ((1UL << (nr & 31)) &
-		(((const volatile unsigned int *)addr)[nr >> 5])) != 0;
-}
-
-static inline int sync_var_test_bit(int nr, const volatile unsigned long * addr)
-{
-	int oldbit;
-
-	__asm__ __volatile__("btl %2,%1\n\tsbbl %0,%0"
-			     :"=r" (oldbit)
-			     :"m" (ADDR),"Ir" (nr));
-	return oldbit;
-}
-
-#define sync_test_bit(nr,addr)			\
-	(__builtin_constant_p(nr) ?		\
-	 sync_constant_test_bit((nr),(addr)) :	\
-	 sync_var_test_bit((nr),(addr)))
+#define sync_test_bit test_bit
 
 #undef ADDR
#1 lore raw
Quoted 22 lines · Jan Beulich

There really is no need for a redundant implementation here, just keep the alternative name for allowing consumers to use consistent naming.

Signed-off-by Jan Beulich <[email protected]>
include/asm-x86/sync_bitops.h +0 -11
@@ -130,26 +130,7 @@ static inline int sync_test_and_change_b
 	return oldbit;
 }
 
-static __always_inline int sync_constant_test_bit(int nr, const volatile unsigned long *addr)
-{
-	return ((1UL << (nr & 31)) &
-		(((const volatile unsigned int *)addr)[nr >> 5])) != 0;
-}
-
-static inline int sync_var_test_bit(int nr, const volatile unsigned long * addr)
-{
-	int oldbit;
-
-	__asm__ __volatile__("btl %2,%1\n\tsbbl %0,%0"

include/asm-x86/sync_bitops.h static inline int sync_test_and_change_b · line 133

Quoted 10 lines
include/asm-x86/sync_bitops.h +1 -9
-			     :"=r" (oldbit)
-			     :"m" (ADDR),"Ir" (nr));
-	return oldbit;
-}
-
-#define sync_test_bit(nr,addr)			\
-	(__builtin_constant_p(nr) ?		\
-	 sync_constant_test_bit((nr),(addr)) :	\
-	 sync_var_test_bit((nr),(addr)))
+#define sync_test_bit test_bit

Hm,

#define sync_test_bit(nr, addr) test_bit(nr, addr)

would be better, but seems reasonable to me. Or even an inline for consistency.

J

author
#2 lore raw
Quoted 4 lines

#define sync_test_bit(nr, addr) test_bit(nr, addr)

would be better, but seems reasonable to me. Or even an inline for consistency.

I'm usually intentionally using just the names, without parameters and not as inline, in such alias definitions so that in case the name gets used as a function pointer (arguably unlikely here) there's not going to be any missing definition or duplicate function instantiation. But from a functionality point of view, either of the alternatives you suggest is of course as good.

Jan

#3 lore raw
Quoted 6 lines · Jan Beulich

I'm usually intentionally using just the names, without parameters and not as inline, in such alias definitions so that in case the name gets used as a function pointer (arguably unlikely here) there's not going to be any missing definition or duplicate function instantiation. But from a functionality point of view, either of the alternatives you suggest is of course as good.

I'm especially wary of the naked #define, since it will affect any instance of sync_test_bit:

struct bit_thingy {
	unsigned sync_test_bit:1;
	unsigned other_test_bit:1;
	...
};

J

Applied
#4 lore raw
In reply to Jan Beulich

include/asm-x86/sync_bitops.h static inline int sync_test_and_change_b

Quoted 8 lines · Jan Beulich

There really is no need for a redundant implementation here, just keep the alternative name for allowing consumers to use consistent naming.

include/asm-x86/sync_bitops.h +1 -3
-	__asm__ __volatile__("btl %2,%1\n\tsbbl %0,%0"
-			     :"=r" (oldbit)
-			     :"m" (ADDR),"Ir" (nr));
 
+#define sync_test_bit test_bit

thanks, applied.

Ingo

#5 lore raw
Quoted 13 lines · Ingo Molnar
Quoted 8 lines · Jan Beulich

There really is no need for a redundant implementation here, just keep the alternative name for allowing consumers to use consistent naming.

(quoted diff) +0 -3
-	__asm__ __volatile__("btl %2,%1\n\tsbbl %0,%0"
-			     :"=r" (oldbit)
-			     :"m" (ADDR),"Ir" (nr));
Quoted 4 lines
Quoted 2 lines

+#define sync_test_bit test_bit

thanks, applied.

Please use this instead.

J

Subject: x86: use macro parameters on sync_test_bit

Using a naked parameterless macro could lead to other tokens being unexpectedly replaced.

Signed-off-by Jeremy Fitzhardinge <[email protected]>

---
include/asm-x86/sync_bitops.h | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

===================================================================

include/asm-x86/sync_bitops.h +0 -0
@@ -123,7 +123,7 @@

	return oldbit;
}

-#define sync_test_bit test_bit
+#define sync_test_bit(nr, addr) test_bit(nr, addr)

#undef ADDR

#6 lore raw

Review

Quoted 7 lines · Jeremy Fitzhardinge

Subject: x86: use macro parameters on sync_test_bit

Using a naked parameterless macro could lead to other tokens being unexpectedly replaced.

(quoted diff) +1 -1
-#define sync_test_bit test_bit
+#define sync_test_bit(nr, addr) test_bit(nr, addr)

thanks Jeremy, applied.

Ingo