Messages in this thread Patch in this message | | | Date | Thu, 9 Aug 2012 15:49:53 +0100 | From | Will Deacon <> | Subject | Re: RFC: mutex: hung tasks on SMP platforms with asm-generic/mutex-xchg.h |
| |
Hi Nicolas,
Thanks for the replies.
On Thu, Aug 09, 2012 at 06:12:15AM +0100, Nicolas Pitre wrote: > On Tue, 7 Aug 2012, Will Deacon wrote: > > diff --git a/kernel/mutex.c b/kernel/mutex.c > > index a307cc9..27b7887 100644 > > --- a/kernel/mutex.c > > +++ b/kernel/mutex.c > > @@ -170,7 +170,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, > > if (owner && !mutex_spin_on_owner(lock, owner)) > > break; > > > > - if (atomic_cmpxchg(&lock->count, 1, 0) == 1) { > > + if (atomic_cmpxchg(&lock->count, 1, -1) == 1) { > > lock_acquired(&lock->dep_map, ip); > > mutex_set_owner(lock); > > preempt_enable(); > > > > > > All comments welcome. > > Well... after thinking about this for a while, I came to the conclusion > that the mutex_spin_on_owner code is indeed breaking the contract > between the xchg lock fast path and the slow path. The xchg fast path > will always set the count to 0 and rely on the slow path to restore a > possible pre-existing negative count. So the above change would be > needed for correctness in the xchg case, even if it always forces the > unlock into the slow path.
Great, so we agree on that.
> As I mentioned previously, we don't want to force the slow path in all > cases though. The atomic decrement based fast path doesn't need that, > so I'd suggest this fix instead:
One minor typo and a suggested alternative below...
> diff --git a/include/asm-generic/mutex-xchg.h b/include/asm-generic/mutex-xchg.h > index 580a6d35c7..60964844c8 100644 > --- a/include/asm-generic/mutex-xchg.h > +++ b/include/asm-generic/mutex-xchg.h > @@ -108,4 +108,6 @@ __mutex_fastpath_trylock(atomic_t *count, int (*fail_fn)(atomic_t *)) > return prev; > } > > +#define __MUTEX_XCHG_FAST_PATH > + > #endif > diff --git a/kernel/mutex.c b/kernel/mutex.c > index a307cc9c95..c6a26a4f1c 100644 > --- a/kernel/mutex.c > +++ b/kernel/mutex.c > @@ -161,6 +161,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, > > for (;;) { > struct task_struct *owner; > + int locked_val; > > /* > * If there's an owner, wait for it to either > @@ -170,7 +171,19 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, > if (owner && !mutex_spin_on_owner(lock, owner)) > break; > > - if (atomic_cmpxchg(&lock->count, 1, 0) == 1) { > +#ifdef __MUTEX_XCHG_FAST_PATH > + /* > + * The fast path based on xchg sets a transient 0 count, > + * relying on the slow path to restore a possible > + * pre-existing contended count. Without checking the > + * waiters' list we must presume possible contension here.
s/contension/contention/
> + */ > + locked_val = -1; > +#else > + locked_val = 0; > +#endif > + > + if (atomic_cmpxchg(&lock->count, 1, locked_val) == 1) { > lock_acquired(&lock->dep_map, ip); > mutex_set_owner(lock); > preempt_enable(); > > That would be needed for the stable tree as well. > > A further cleanup could remove all definitions of > __mutex_slowpath_needs_to_unlock() given that they're all set to 1 > except for the xchg fast path, in which case __MUTEX_XCHG_FAST_PATH > could be reused instead.
I think we could actually fix this entirely in mutex-xchg.h by doing something in fastpath_lock similar to what we do for trylock:
diff --git a/include/asm-generic/mutex-xchg.h b/include/asm-generic/mutex-xchg.h index 580a6d3..c082e99 100644 --- a/include/asm-generic/mutex-xchg.h +++ b/include/asm-generic/mutex-xchg.h @@ -25,8 +25,19 @@ static inline void __mutex_fastpath_lock(atomic_t *count, void (*fail_fn)(atomic_t *)) { - if (unlikely(atomic_xchg(count, 0) != 1)) - fail_fn(count); + int prev = atomic_xchg(count, 0); + + if (unlikely(prev != 1)) { + if (prev < 0) + /* + * The lock was contended, so we need to restore + * its original state to ensure that any waiting + * tasks are woken up by the unlock slow path. + */ + prev = atomic_xchg(count, prev); + if (prev != 1) + fail_fn(count); + } } /**
What do you reckon?
> Of course that might tilt the balance towards using mutex-dec.h on ARM > v6 and above instead of mutex-xchg.h. But that is an orthogonal issue, > and that doesn't remove the need for fixing the xchg based case for > correctness.
I'll do some hackbench runs against mutex-dec once we've decided on the final xchg code. If it's faster, I'll submit a patch for ARM.
Cheers,
Will
| |