[PATCH] x86: slightly shorten __ticket_spin_trylock() (v2)

[PATCH] x86: slightly shorten __ticket_spin_trylock() (v2)

[Jan Beulich]

Since the callers generally expect a boolean value, there's no need to
zero-extend the outcome of the comparison. It just requires that all
of x86' trylock implementations have their return type changed
accordingly.

Don't use bool for the return type though - this is being frowned on
and presently doesn't work with the pv-ops patching macros.

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Nick Piggin <npiggin@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>

---
 arch/x86/include/asm/paravirt.h       |    4 ++--
 arch/x86/include/asm/paravirt_types.h |    2 +-
 arch/x86/include/asm/spinlock.h       |   12 +++++-------
 arch/x86/xen/spinlock.c               |    2 +-
 4 files changed, 9 insertions(+), 11 deletions(-)

--- linux-2.6.32-rc8/arch/x86/include/asm/paravirt.h	2009-11-20 14:10:50.000000000 +0100
+++ 2.6.32-rc8-x86-spin-trylock-simplify/arch/x86/include/asm/paravirt.h	2009-12-03 10:24:51.000000000 +0100
@@ -753,9 +753,9 @@ static __always_inline void __raw_spin_l
 	PVOP_VCALL2(pv_lock_ops.spin_lock_flags, lock, flags);
 }
 
-static __always_inline int __raw_spin_trylock(struct raw_spinlock *lock)
+static __always_inline u8 __raw_spin_trylock(struct raw_spinlock *lock)
 {
-	return PVOP_CALL1(int, pv_lock_ops.spin_trylock, lock);
+	return PVOP_CALL1(u8, pv_lock_ops.spin_trylock, lock);
 }
 
 static __always_inline void __raw_spin_unlock(struct raw_spinlock *lock)
--- linux-2.6.32-rc8/arch/x86/include/asm/paravirt_types.h	2009-11-20 14:10:50.000000000 +0100
+++ 2.6.32-rc8-x86-spin-trylock-simplify/arch/x86/include/asm/paravirt_types.h	2009-12-03 10:24:58.000000000 +0100
@@ -324,7 +324,7 @@ struct pv_lock_ops {
 	int (*spin_is_contended)(struct raw_spinlock *lock);
 	void (*spin_lock)(struct raw_spinlock *lock);
 	void (*spin_lock_flags)(struct raw_spinlock *lock, unsigned long flags);
-	int (*spin_trylock)(struct raw_spinlock *lock);
+	u8 (*spin_trylock)(struct raw_spinlock *lock);
 	void (*spin_unlock)(struct raw_spinlock *lock);
 };
 
--- linux-2.6.32-rc8/arch/x86/include/asm/spinlock.h	2009-09-10 00:13:59.000000000 +0200
+++ 2.6.32-rc8-x86-spin-trylock-simplify/arch/x86/include/asm/spinlock.h	2009-12-03 10:26:07.000000000 +0100
@@ -77,7 +77,7 @@ static __always_inline void __ticket_spi
 		: "memory", "cc");
 }
 
-static __always_inline int __ticket_spin_trylock(raw_spinlock_t *lock)
+static __always_inline u8 __ticket_spin_trylock(raw_spinlock_t *lock)
 {
 	int tmp, new;
 
@@ -88,12 +88,11 @@ static __always_inline int __ticket_spin
 		     LOCK_PREFIX "cmpxchgw %w1,%2\n\t"
 		     "1:"
 		     "sete %b1\n\t"
-		     "movzbl %b1,%0\n\t"
 		     : "=&a" (tmp), "=&q" (new), "+m" (lock->slock)
 		     :
 		     : "memory", "cc");
 
-	return tmp;
+	return new;
 }
 
 static __always_inline void __ticket_spin_unlock(raw_spinlock_t *lock)
@@ -127,7 +126,7 @@ static __always_inline void __ticket_spi
 		     : "memory", "cc");
 }
 
-static __always_inline int __ticket_spin_trylock(raw_spinlock_t *lock)
+static __always_inline u8 __ticket_spin_trylock(raw_spinlock_t *lock)
 {
 	int tmp;
 	int new;
@@ -141,12 +140,11 @@ static __always_inline int __ticket_spin
 		     LOCK_PREFIX "cmpxchgl %1,%2\n\t"
 		     "1:"
 		     "sete %b1\n\t"
-		     "movzbl %b1,%0\n\t"
 		     : "=&a" (tmp), "=&q" (new), "+m" (lock->slock)
 		     :
 		     : "memory", "cc");
 
-	return tmp;
+	return new;
 }
 
 static __always_inline void __ticket_spin_unlock(raw_spinlock_t *lock)
@@ -190,7 +188,7 @@ static __always_inline void __raw_spin_l
 	__ticket_spin_lock(lock);
 }
 
-static __always_inline int __raw_spin_trylock(raw_spinlock_t *lock)
+static __always_inline u8 __raw_spin_trylock(raw_spinlock_t *lock)
 {
 	return __ticket_spin_trylock(lock);
 }
--- linux-2.6.32-rc8/arch/x86/xen/spinlock.c	2009-11-20 14:10:51.000000000 +0100
+++ 2.6.32-rc8-x86-spin-trylock-simplify/arch/x86/xen/spinlock.c	2009-12-03 10:26:47.000000000 +0100
@@ -136,7 +136,7 @@ static int xen_spin_is_contended(struct 
 	return xl->spinners != 0;
 }
 
-static int xen_spin_trylock(struct raw_spinlock *lock)
+static u8 xen_spin_trylock(struct raw_spinlock *lock)
 {
 	struct xen_spinlock *xl = (struct xen_spinlock *)lock;
 	u8 old = 1;

Re: [PATCH] x86: slightly shorten __ticket_spin_trylock() (v2)

[Ingo Molnar]

* Jan Beulich <JBeulich@novell.com> wrote:

> Since the callers generally expect a boolean value, there's no need to
> zero-extend the outcome of the comparison. It just requires that all
> of x86' trylock implementations have their return type changed
> accordingly.
> 
> Don't use bool for the return type though - this is being frowned on
> and presently doesn't work with the pv-ops patching macros.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Nick Piggin <npiggin@suse.de>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> 
> ---
>  arch/x86/include/asm/paravirt.h       |    4 ++--
>  arch/x86/include/asm/paravirt_types.h |    2 +-
>  arch/x86/include/asm/spinlock.h       |   12 +++++-------
>  arch/x86/xen/spinlock.c               |    2 +-
>  4 files changed, 9 insertions(+), 11 deletions(-)

Looks much cleaner - and should give us pretty much the same savings as 
the previous patch, right? (if not, do you have a size comparison?)

	Ingo

Re: [PATCH] x86: slightly shorten __ticket_spin_trylock() (v2)

[Jan Beulich]

>>> Ingo Molnar <mingo@elte.hu> 03.12.09 11:49 >>>
>Looks much cleaner - and should give us pretty much the same savings as 
>the previous patch, right? (if not, do you have a size comparison?)

With upstream code there doesn't appear to be a difference in the
generated code. I noted, however, that with the spinlock contention
patch I have pending as a potential replacement for pv-ops spin locks
(which adds a conditional in the trylock functions) the compiler I use
no longer eliminates one of the two tests of the outcome of the
locking attempt.

Jan

Re: [PATCH] x86: slightly shorten __ticket_spin_trylock() (v2)

[Linus Torvalds]

On Thu, 3 Dec 2009, Jan Beulich wrote:
>  
> -static __always_inline int __ticket_spin_trylock(raw_spinlock_t *lock)
> +static __always_inline u8 __ticket_spin_trylock(raw_spinlock_t *lock)
>  {
>  	int tmp, new;
>  
> @@ -88,12 +88,11 @@ static __always_inline int __ticket_spin
>  		     LOCK_PREFIX "cmpxchgw %w1,%2\n\t"
>  		     "1:"
>  		     "sete %b1\n\t"
> -		     "movzbl %b1,%0\n\t"
>  		     : "=&a" (tmp), "=&q" (new), "+m" (lock->slock)
>  		     :
>  		     : "memory", "cc");
>  
> -	return tmp;
> +	return new;

This is fairly pessimal register allocation.

It used to be that we returned the value in 'tmp', which is %eax, which is 
also the expected return register. Now that we use 'new', it's some random 
other register that is _not_ %eax, which means that while we avoid a 
'movzbl', the regular spin_trylock function call case will now have the 
compiler emitting a 

	movb %X,%al
	ret

at the end just to get the right return register.

Which seems a bit annoying. It looks like we could have done that last 
instruction as just

	sete %b0

instead, and then return 'tmp' instead of 'new', keeping the return value 
in %al and avoiding the unnecessary movement.

> -static __always_inline int __ticket_spin_trylock(raw_spinlock_t *lock)
> +static __always_inline u8 __ticket_spin_trylock(raw_spinlock_t *lock)

Same thing here, afaik.

NOTE! I haven't actually looked at the generated code, and if we actually 
inline it all the way to the caller, it won't matter (and picking another 
register may even help). But while these helpers are marked 
__always_inline, I _thought_ that the way we actually build the final 
'spin_trylock()' function we end up with a real function in the end.

Maybe I'm wrong.

		Linus

Re: [PATCH] x86: slightly shorten __ticket_spin_trylock() (v2)

[Jan Beulich]

>>> Linus Torvalds <torvalds@linux-foundation.org> 03.12.09 22:10 >>>
>On Thu, 3 Dec 2009, Jan Beulich wrote:
>> @@ -88,12 +88,11 @@ static __always_inline int __ticket_spin
>>  		     LOCK_PREFIX "cmpxchgw %w1,%2\n\t"
>>  		     "1:"
>>  		     "sete %b1\n\t"
>> -		     "movzbl %b1,%0\n\t"
>>  		     : "=&a" (tmp), "=&q" (new), "+m" (lock->slock)
>>  		     :
>>  		     : "memory", "cc");
>>  
>> -	return tmp;
>> +	return new;
>
>This is fairly pessimal register allocation.
>
>It used to be that we returned the value in 'tmp', which is %eax, which is 
>also the expected return register. Now that we use 'new', it's some random 
>other register that is _not_ %eax, which means that while we avoid a 
>'movzbl', the regular spin_trylock function call case will now have the 
>compiler emitting a 
>
>	movb %X,%al
>	ret
>
>at the end just to get the right return register.

No, not really, because __spin_trylock() doesn't directly pass on the
return value of _raw_spin_trylock().

>Which seems a bit annoying. It looks like we could have done that last 
>instruction as just
>
>	sete %b0
>
>instead, and then return 'tmp' instead of 'new', keeping the return value 
>in %al and avoiding the unnecessary movement.
>
>> -static __always_inline int __ticket_spin_trylock(raw_spinlock_t *lock)
>> +static __always_inline u8 __ticket_spin_trylock(raw_spinlock_t *lock)
>
>Same thing here, afaik.
>
>NOTE! I haven't actually looked at the generated code, and if we actually 
>inline it all the way to the caller, it won't matter (and picking another 
>register may even help). But while these helpers are marked 
>__always_inline, I _thought_ that the way we actually build the final 
>'spin_trylock()' function we end up with a real function in the end.

The code isn't really much different either way: In the old variant, the
compiler generated a xor/set pair, in the variant you suggest it
produces a single movzx. In the _spin_trylock_bh() case the new
variant actually produces an extra instruction (copying from %eax to
another register), but all that certainly also depends on the compiler
version.

Hence I'm really uncertain which of both methods is preferable. The
one additional benefit to the version you suggest is that it permits
relaxing the constraint of 'new' from q to r (permitting the compiler
to pick from a wider set of registers on 32-bit). But since there are
only very few direct callers of _raw_spin_trylock(), this is marginal,
as it doesn't matter for _spin_trylock() (I don't think the lock
debugging case really needs much attention on performance).

Jan