Re: [patch 1/3] mutex subsystem: trylock

[Ingo Molnar <mingo@elte.hu>]

* Nicolas Pitre <nico@cam.org> wrote:

> In the spirit of uniformity, this patch provides architecture specific 
> trylock implementations.  This allows for a pure xchg-based 
> implementation, as well as an optimized ARMv6 implementation.

hm, i dont really like the xchg variant:

> +static inline int
> +__mutex_trylock(atomic_t *count)
> +{
> +	int prev = atomic_xchg(count, 0);
> +
> +	if (unlikely(prev < 0)) {
> +		/*
> +		 * The lock was marked contended so we must restore that
> +		 * state. If while doing so we get back a prev value of 1
> +		 * then we just own it.
> +		 *
> +		 * IN all cases this has the potential to trigger the
> +		 * slowpath for the owner's unlock path - but this is not
> +		 * a big problem in practice.
> +		 */
> +		prev = atomic_xchg(count, -1);
> +		if (prev < 0)
> +			prev = 0;
> +	}

here we go to great trouble trying to avoid the 'slowpath', while we 
unconditionally force the next unlock into the slowpath! So we have not 
won anything. (on a cycle count basis it's probably even a net loss)

The same applies to atomic_dec_return() based trylock variant. Only the 
cmpxchg based one, and the optimized ARM variant should be used as a 
'fastpath'.

another thing is that this solution still leaves that ugly #ifdef in 
kernel/mutex.c.

so i took a different solution that solves both problems. The API 
towards architectures is now:

 static inline int
 __mutex_fastpath_trylock(atomic_t *count, int (*fn)(atomic_t *))

note the new 'fn' parameter: this enables mutex-null.h to do a simple:

 #define __mutex_fastpath_trylock(count, fn_name)        fn_name(count)

and the final ugly debugging related #ifdef is gone from kernel/mutex.c!  
Furthermore, both the mutex-xchg.h and the mutex-dec.h non-cmpxchg 
variant will fall back to the spinlock implementation.

	Ingo