Re: [PATCH 1/19] MUTEX: Introduce simple mutex implementation

[Nick Piggin <nickpiggin@yahoo.com.au>]

David Howells wrote:
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> 
> 
>>> (1) If it's using spinlocks, then it's pointless to use atomic_cmpxchg.
>>>
>>
>>Why?
> 
> 
> Because you're going to end up with loops around the cmpxchg bit in certain
> places, and if you do, then you've effectively got this:
> 
> 	spin_lock_irqsave(mutexlock, flags);
> 	do {
> 		new = calc_state(orig, oldstate);
> 		spin_lock_irqsave(atomiclock, flags2);
> 		oldstate = __cmpxchg(&mutex->count, orig, new)
> 		spin_unlock_irqrestore(atomiclock, flags2);
+> 	} while (oldstate != orig);
> 	spin_unlock_irqrestore(mutexlock, flags);
> 
> which is very bad. You _should_ have:
> 
> 	spin_lock_irqsave(mutexlock, flags);
> 	oldstate = mutex->count;
> 	mutex->count = modify_state(mutex->count);
> 	spin_unlock_irqrestore(mutexlock, flags);
> 
> instead.

I was under the impression that with cmpxchg, you don't need the mutex lock.
If you do then sure, cmpxchg doesn't buy you anything (even if the arch does
natively support it).

> 
> No. If you have XCHG/TAS/BSET/SWAP, but not CMPXCHG/CAS then you can do a lot
> better by not pretending that cmpxchg exists. That way the fast paths don't
> have to take any spinlocks at all.
> 
> And if you've got LLD/SCD or LDARX/STDCX or similar then you can probably do
> better than CMPXCHG also.
> 
> If you want an illustration, then consider this:
> 
> 	#define __mutex_trylock(mutex)					\
> 	({								\
> 		int oldstate;						\
> 									\
> 		asm volatile("swap%I0 %M0,%1"				\
> 			     : "+m"(mutex->state), "=r"(oldstate)	\
> 			     : "1"(1)					\
> 			     : "memory");				\
> 									\
> 		oldstate == 0;						\
> 	})
> 
> 	static inline int down_trylock(struct mutex *mutex)
> 	{
> 		if (likely(__mutex_trylock(mutex))) {
> 			/* success */
> 			return 0;
> 		}
> 
> 		/* failure */
> 		return 1;
> 	}
> 
> 	void fastcall __sched down(struct mutex *mutex)
> 	{
> 		if (down_trylock(mutex) == 1)
> 			__down(mutex);
> 	}
> 
> 	EXPORT_SYMBOL(down);
> 
> On FRV, this can be made to map to:
> 
> 	setlos	0x1,gr4
> 	ori	gr4,0,gr5
> 	swap	@(gr8,gr0),gr5
> 	subicc	gr5,0,gr0,icc0
> 	beqlr	icc0,0x2	<-- probable-rated conditional return
> 	sethi.p	0xc01c,gr14
> 	setlo	0x9df0,gr14
> 	jmpl	@(gr14,gr0)
> 
> That's an out-of-line fast path of _5_ instructions. Attempting to emulate
> CMPXCHG requires a lot more. On FRV, the case is alleviated somewhat since it
> doesn't yet provide spinlocks and support SMP, but you'd be very hard pressed
> to squeeze it down to just five instructions.
> 

I think all of about parisc and sparc32 "emulate" cmpxchg with spinlocks.
For architectures like i386, x86_64, ppc, ia64, etc. the cmpxchg will
give good code.

Then if FRV was still unhappy, then you could override the mutex in that
architecture. This just seemed better to me than having a crappy simple
implementation that *everyone* will want to override (and I see FRV
overrides it as well, I don't see how you can complain about that).

But I guess that's moot if you can't to do a lockless version using
cmpxchg.

> 
>>> (2) atomic_t is a 32-bit type, and on a 64-bit platform I will want a
>>>     64-bit type so that I can stick the owner address in there (I've got
>>>     a second variant not yet released).
>>>
>>
>>I'm sure you could use a seperate field as it would be a debug
>>option, right?
> 
> 
> True. Ingo suggested this, and it seems reasonable. OTOH, shrinking the count
> by 4 bytes would allow the whole structure to shrink by 8 on a 64-bit platform
> with a 4-byte spinlock, which would be even better.
> 

I'm sure you'd manage. spinlocks can get pretty large with debugging on too.

-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com