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:

quoted

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...

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