Re: [RFC][PATCH -v2 4/4] locking/mutex: Add lock handoff to avoid starvation

[Peter Zijlstra]

On Mon, Aug 29, 2016 at 05:41:09PM +0200, Peter Zijlstra wrote:
> On Fri, Aug 26, 2016 at 07:40:34PM -0400, Waiman Long wrote:
> > On 08/26/2016 11:18 AM, Peter Zijlstra wrote:
> 
> > >Still need to look at adding spinning to the handoff case.
> > >Also need to look at writing (much) better changelogs, they stink.
> > >
> > 
> > I have looked at the handoff code and I didn't see any problem.
> 
> So I found (or rather the buildbot did) a problem with it.
> 
> locking-selftest has testcases like:
> 
> 
> 	lock(&A);
> 	if (trylock(&A))
> 		/* fail */
> 
>   and
> 
> 	ww_lock(&A)
> 	if (ww_lock(&A) != -EDEADLK)
> 		/* fail */
> 
> But with the 'trylock' accepting the lock if owner==current, in order to
> accept the hand-off, this breaks in interesting ways.
> 
> Now, ARCH_MIN_TASKALIGN is at least 8 (mips, s390, parisc) which would
> give us one more FLAG bit to play with.
> 
> 
> The below seems to make things happy again..

Much simpler solution... only accept handoffs when we're stuck in the
wait loop (which precludes doing recursive locking, since that would've
failed much earlier).

Now, let me look at that spinner patch you sent.

---
--- kernel/locking/mutex.c.mod	2016-08-30 11:08:15.410551744 +0200
+++ kernel/locking/mutex.c	2016-08-30 13:38:30.185550669 +0200
@@ -69,7 +69,7 @@
 /*
  * Actual trylock that will work on any unlocked state.
  */
-static inline bool __mutex_trylock(struct mutex *lock)
+static inline bool __mutex_trylock(struct mutex *lock, const bool handoff)
 {
 	unsigned long owner, curr = (unsigned long)current;
 
@@ -78,8 +78,10 @@
 		unsigned long old;
 
 		if (__owner_task(owner)) {
-			if ((unsigned long)__owner_task(owner) == curr)
+			if (handoff && unlikely(__owner_task(owner) == current)) {
+				smp_mb(); /* ACQUIRE */
 				return true;
+			}
 
 			return false;
 		}
@@ -134,6 +136,10 @@
 	return list_first_entry(&lock->wait_list, struct mutex_waiter, list) == waiter;
 }
 
+/*
+ * Give up ownership to a specific task, when @task = NULL, this is equivalent
+ * to a regular unlock.
+ */
 static void __mutex_handoff(struct mutex *lock, struct task_struct *task)
 {
 	unsigned long owner = atomic_long_read(&lock->owner);
@@ -148,7 +154,7 @@
 		new = (owner & MUTEX_FLAG_WAITERS);
 		new |= (unsigned long)task;
 
-		old = atomic_long_cmpxchg(&lock->owner, owner, new);
+		old = atomic_long_cmpxchg_release(&lock->owner, owner, new);
 		if (old == owner)
 			break;
 
@@ -425,7 +431,7 @@
 			break;
 
 		/* Try to acquire the mutex if it is unlocked. */
-		if (__mutex_trylock(lock)) {
+		if (__mutex_trylock(lock, false)) {
 			osq_unlock(&lock->osq);
 			return true;
 		}
@@ -570,7 +576,12 @@
 	preempt_disable();
 	mutex_acquire_nest(&lock->dep_map, subclass, 0, nest_lock, ip);
 
-	if (__mutex_trylock(lock) || mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx)) {
+	/*
+	 * The first __mutex_trylock() must not accept handoffs, otherwise its
+	 * possible to allow recursive lock attempts by accident.
+	 */
+	if (__mutex_trylock(lock, false) ||
+	    mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx)) {
 		/* got the lock, yay! */
 		lock_acquired(&lock->dep_map, ip);
 		if (use_ww_ctx) {
@@ -588,7 +599,7 @@
 	/*
 	 * Once more, try to acquire the lock.
 	 */
-	if (__mutex_trylock(lock))
+	if (__mutex_trylock(lock, false))
 		goto skip_wait;
 
 	debug_mutex_lock_common(lock, &waiter);
@@ -601,7 +612,7 @@
 	if (__mutex_waiter_is_first(lock, &waiter))
 		__mutex_set_flag(lock, MUTEX_FLAG_WAITERS);
 
-	if (__mutex_trylock(lock))
+	if (__mutex_trylock(lock, false))
 		goto remove_waiter;
 
 	lock_contended(&lock->dep_map, ip);
@@ -629,7 +640,7 @@
 		schedule_preempt_disabled();
 		spin_lock_mutex(&lock->wait_lock, flags);
 
-		if (__mutex_trylock(lock))
+		if (__mutex_trylock(lock, true))
 			break;
 
 		if (__mutex_waiter_is_first(lock, &waiter))
@@ -923,7 +934,7 @@
  */
 int __sched mutex_trylock(struct mutex *lock)
 {
-	bool locked = __mutex_trylock(lock);
+	bool locked = __mutex_trylock(lock, false);
 
 	if (locked)
 		mutex_acquire(&lock->dep_map, 0, 1, _RET_IP_);