locking/mutex: Rewrite basic mutex

Poking at lock internals is not cool. Since I'm going to change the implementation this will break, take it out.

So something like the below would serve as a replacement for your previous hacks. Is this API something acceptable to people? Ingo, Thomas?

Ugh. I think the concept is fine, but can we place make these enum's be all upper case or something to make them really stand out visually.

The other choice would be to just make the choices be negative (== recursive), zero (== failed) or positive (== got lock), which allows for the same value re-use for the non-recursive case, and you could avoid the enum entirely.

Quoted 4 lines · Linus Torvalds

The other choice would be to just make the choices be negative (== recursive), zero (== failed) or positive (== got lock), which allows for the same value re-use for the non-recursive case, and you could avoid the enum entirely.

I thought about that, but liked the enum better for having to then spell it out.

I'll go make the enum shout and add comment as you suggest.

Quoted 2 lines · Peter Zijlstra

Poking at lock internals is not cool. Since I'm going to change the implementation this will break, take it out.

So something like the below would serve as a replacement for your previous hacks. Is this API something acceptable to people? Ingo, Thomas?

---
 include/linux/mutex.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

include/linux/mutex.h +0 -0

index 4d3bccabbea5..afcff2c85957 100644

include/linux/mutex.h +10 -0

+};
+
+/**
+ * mutex_trylock_recursive - trylock variant that allows recursive locking
+ * @lock: mutex to be locked
+ *
+ *
+ * Returns:
+ *  mutex_trylock_failed    - trylock failed,
+ *  mutex_trylock_success   - lock acquired,

Hmm. I'm not a great fan of this, because that requires an conditional unlock mechanism.

       res = trylock_recursive(lock);
       if (res == FAILED)
       	       goto out;
       .....

       if (res == SUCCESS)
       	       unlock(lock);

While if you actually keep track of recursion you can do:

      if (!trylock_recursive(lock))
		goto out;

      ....

      unlock_recursive(lock);

or even:

     lock_recursive(lock);

     unlock_recursive(lock);

That's making lock/trylock and unlock symetric, so its obvious in the source what's going on and the recursion tracking allows for better debugability.

Quoted 22 lines · Thomas Gleixner

Hmm. I'm not a great fan of this, because that requires an conditional unlock mechanism.

       res = trylock_recursive(lock);
       if (res == FAILED)
       	       goto out;
       .....

       if (res == SUCCESS)
       	       unlock(lock);

While if you actually keep track of recursion you can do:

      if (!trylock_recursive(lock))
		goto out;

      ....

      unlock_recursive(lock);

or even:

Quoted 7 lines

     lock_recursive(lock);

     unlock_recursive(lock);

That's making lock/trylock and unlock symetric, so its obvious in the source what's going on and the recursion tracking allows for better debugability.

Hurm,. so I thought that in general we disliked recursive locking because it quickly turns in to a horrible mess.

Adding such primitives makes it 'easy' to use recursive locking and then where does it stop?

Well, when you add just trylock_recursive then people are going to use it anyway no matter whether it is easy or not.

So if we decide to provide something which supports recursive locking for mutexes then we are better off doing it with a proper set of functions and not just a single undebugable wrapper.

Quoted 2 lines · Peter Zijlstra

Poking at lock internals is not cool. Since I'm going to change the implementation this will break, take it out.

So something like the below would serve as a replacement for your previous hacks. Is this API something acceptable to people?

Poking at lock internals is not cool. Since I'm going to change the implementation this will break, take it out.

Cc Daniel Vetter <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Rob Clark <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_shrinker.c |   26 +++-----------------------
 drivers/gpu/drm/msm/msm_gem_shrinker.c   |   23 +++--------------------
 2 files changed, 6 insertions(+), 43 deletions(-)

Quoted 11 lines · Peter Zijlstra

Poking at lock internals is not cool. Since I'm going to change the implementation this will break, take it out.

Cc Daniel Vetter <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Rob Clark <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_shrinker.c |   26 +++-----------------------
 drivers/gpu/drm/msm/msm_gem_shrinker.c   |   23 +++--------------------
 2 files changed, 6 insertions(+), 43 deletions(-)

OK, so it appears that i915 changed their locking around and got rid of this thing entirely. Much appreciated Chris!!

Quoted 13 lines · Peter Zijlstra

Quoted 11 lines · Peter Zijlstra

Poking at lock internals is not cool. Since I'm going to change the implementation this will break, take it out.

Cc Daniel Vetter <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Rob Clark <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_shrinker.c |   26 +++-----------------------
 drivers/gpu/drm/msm/msm_gem_shrinker.c   |   23 +++--------------------
 2 files changed, 6 insertions(+), 43 deletions(-)

Quoted 2 lines

OK, so it appears that i915 changed their locking around and got rid of this thing entirely. Much appreciated Chris!!

Hmm, I might have spoken too soon. My patch conflicted and I seem to have read too much in the Changelog of 3b4e896f14b1 ("drm/i915: Remove unused no-shrinker-steal").

Once all your locking rework is assembled it might be good to have a topic branch I could pull in. Both for testing and to handle conflicts before it goes boom in the merge window ;-) Not necessary ofc, but I think it'd be useful.

Quoted 4 lines · Daniel Vetter

Once all your locking rework is assembled it might be good to have a topic branch I could pull in. Both for testing and to handle conflicts before it goes boom in the merge window ;-) Not necessary ofc, but I think it'd be useful.

Everything except the trylock_recursive is already in tip/locking/core.

Ingo, is that all there is in that branch?

Could you send a final version of trylock_recursive() patch for me to apply?

Quoted 1 line · Ingo Molnar

Could you send a final version of trylock_recursive() patch for me to apply?

The latest lives here:

lkml.kernel.org/r/[email protected]

But I would really like someone to actually test that before you stick it in.

Quoted 11 lines · Peter Zijlstra

Poking at lock internals is not cool. Since I'm going to change the implementation this will break, take it out.

Cc Daniel Vetter <[email protected]>

Cc Chris Wilson <[email protected]>

Cc Rob Clark <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_shrinker.c |   26 +++-----------------------
 drivers/gpu/drm/msm/msm_gem_shrinker.c   |   23 +++--------------------
 2 files changed, 6 insertions(+), 43 deletions(-)

OK, so it appears that i915 changed their locking around and got rid of this thing entirely. Much appreciated Chris!!

kernel/locking/mutex.c line 111

Quoted 10 lines · Peter Zijlstra

kernel/locking/mutex.c +10 -0

+/*
+ * Optimistic trylock that only works in the uncontended case. Make sure to
+ * follow with a __mutex_trylock() before failing.
+ */
+static __always_inline bool __mutex_trylock_fast(struct mutex *lock)
+{
+	unsigned long curr = (unsigned long)current;
+
+	if (!atomic_long_cmpxchg_acquire(&lock->owner, 0UL, curr))
+		return true;

Do we want to do a ccas check for !lock->owner? Although I can see a possible case of 'optimizing for the contended' reasons for nay.

The current mutex implementation has an atomic lock word and a non-atomic owner field.

This disparity leads to a number of issues with the current mutex code as it means that we can have a locked mutex without an explicit owner (because the owner field has not been set, or already cleared).

This leads to a number of weird corner cases, esp. between the optimistic spinning and debug code. Where the optimistic spinning code needs the owner field updated inside the lock region, the debug code is more relaxed because the whole lock is serialized by the wait_lock.

Also, the spinning code itself has a few corner cases where we need to deal with a held lock without an owner field.

Furthermore, it becomes even more of a problem when trying to fix starvation cases in the current code. We end up stacking special case on special case.

To solve this rework the basic mutex implementation to be a single atomic word that contains the owner and uses the low bits for extra state.

This matches how PI futexes and rt_mutex already work. By having the owner an integral part of the lock state a lot of the problems dissapear and we get a better option to deal with starvation cases, direct owner handoff.

Changing the basic mutex does however invalidate all the arch specific mutex code; this patch leaves that unused in-place, a later patch will remove that.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/mutex-debug.h  |   24 --
 include/linux/mutex.h        |   46 +++--
 kernel/locking/mutex-debug.c |   13 -
 kernel/locking/mutex-debug.h |   10 -
 kernel/locking/mutex.c       |  371 ++++++++++++++++++-------------------------
 kernel/locking/mutex.h       |   26 ---
 kernel/sched/core.c          |    2 
 7 files changed, 187 insertions(+), 305 deletions(-)

kernel/locking/mutex.c +13 -2

@@ -54,8 +54,10 @@ EXPORT_SYMBOL(__mutex_init);
  * bits to store extra state.
  *
  * Bit0 indicates a non-empty waiter list; unlock must issue a wakeup.
+ * Bit1 indicates unlock needs to hand the lock to the top-waiter
  */
 #define MUTEX_FLAG_WAITERS	0x01
+#define MUTEX_FLAG_HANDOFF	0x02
 
 #define MUTEX_FLAGS		0x03
 

@@ -71,20 +73,48 @@ static inline unsigned long __owner_flag
 
 /*
  * Actual trylock that will work on any unlocked state.
+ *
+ * When setting the owner field, we must preserve the low flag bits.
+ *
+ * Be careful with @handoff, only set that in a wait-loop (where you set
+ * HANDOFF) to avoid recursive lock attempts.
  */
-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;
 
 	owner = atomic_long_read(&lock->owner);
 	for (;;) { /* must loop, can race against a flag */
-		unsigned long old;
+		unsigned long old, flags = __owner_flags(owner);
+
+		if (__owner_task(owner)) {
+			if (handoff && unlikely(__owner_task(owner) == current)) {
+				/*

Quoted 10 lines

kernel/locking/mutex.c +10 -0

+		if (__owner_task(owner)) {
+			if (handoff && unlikely(__owner_task(owner) == current)) {
+				/*
+				 * Provide ACQUIRE semantics for the lock-handoff.
+				 *
+				 * We cannot easily use load-acquire here, since
+				 * the actual load is a failed cmpxchg, which
+				 * doesn't imply any barriers.
+				 *
+				 * Also, this is a fairly unlikely scenario, and

kernel/locking/mutex.c static inline unsigned long __owner_flag · line 102

Quoted 3 lines

kernel/locking/mutex.c +3 -0

+				 * this contains the cost.
+				 */
+				smp_mb(); /* ACQUIRE */

As we discussed on another thread recently, a failed cmpxchg_acquire will always give you ACQUIRE semantics in practice. Maybe we should update the documentation to allow this? The only special case is the full-barrier version.

/*
* Actual trylock that will work on any unlocked state.
+ *
+ * When setting the owner field, we must preserve the low flag bits.
+ *
+ * Be careful with @handoff, only set that in a wait-loop (where you set
+ * HANDOFF) to avoid recursive lock attempts.
*/

kernel/locking/mutex.c +5 -2

-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;
 
  	owner = atomic_long_read(&lock->owner);
  	for (;;) { /* must loop, can race against a flag */
-		unsigned long old;
+		unsigned long old, flags = __owner_flags(owner);
+
+		if (__owner_task(owner)) {
+			if (handoff&&  unlikely(__owner_task(owner) == current)) {

kernel/locking/mutex.c +5 -3

+				smp_mb(); /* ACQUIRE */
+				return true;
+			}
 
-		if (__owner_task(owner))
  			return false;
+		}
 
-		old = atomic_long_cmpxchg_acquire(&lock->owner, owner,
-						  curr | __owner_flags(owner));
+		/*

Quoted 22 lines · Peter Zijlstra

/*
* Actual trylock that will work on any unlocked state.
+ *
+ * When setting the owner field, we must preserve the low flag bits.
+ *
+ * Be careful with @handoff, only set that in a wait-loop (where you set
+ * HANDOFF) to avoid recursive lock attempts.
*/

(quoted diff) +1 -1

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

      owner = atomic_long_read(&lock->owner);
      for (;;) { /* must loop, can race against a flag */

kernel/locking/mutex.c +4 -1

-        unsigned long old;
+        unsigned long old, flags = __owner_flags(owner);
+
+        if (__owner_task(owner)) {
+            if (handoff&&  unlikely(__owner_task(owner) == current)) {

kernel/locking/mutex.c static inline unsigned long __owner_flag · line 101

Quoted 10 lines

kernel/locking/mutex.c +10 -0

+                /*
+                 * Provide ACQUIRE semantics for the lock-handoff.
+                 *
+                 * We cannot easily use load-acquire here, since
+                 * the actual load is a failed cmpxchg, which
+                 * doesn't imply any barriers.
+                 *
+                 * Also, this is a fairly unlikely scenario, and
+                 * this contains the cost.
+                 */

I am not so sure about your comment here. I guess you are referring to the atomic_long_cmpxchg_acquire below for the failed cmpxchg. However, it is also possible that the path can be triggered on the first round without cmpxchg. Maybe we can do a load_acquire on the owner again to satisfy this requirement without a smp_mb().

Quoted 19 lines

kernel/locking/mutex.c +13 -3

+                smp_mb(); /* ACQUIRE */
+                return true;
+            }
 
-        if (__owner_task(owner))
              return false;
+        }
 
-        old = atomic_long_cmpxchg_acquire(&lock->owner, owner,
-                          curr | __owner_flags(owner));
+        /*
+         * We set the HANDOFF bit, we must make sure it doesn't live
+         * past the point where we acquire it. This would be possible
+         * if we (accidentally) set the bit on an unlocked mutex.
+         */
+        if (handoff)
+            flags&= ~MUTEX_FLAG_HANDOFF;
+
+        old = atomic_long_cmpxchg_acquire(&lock->owner, owner, curr 

Quoted 5 lines

| flags);
          if (old == owner)
              return true;

Other than that, the code is fine.

Reviewed-by Waiman Long <[email protected]>

One more thing, I think it may be worthwhile to add another comment about what happens when the HANDOFF bit was set while we take the error path (goto err). As the actual handoff is serialized by the wait_lock, the code will still do the right thing. Either the next one in the queue will be handed off or it will be unlocked if the queue is empty.

Quoted 28 lines · Peter Zijlstra

/*
* Actual trylock that will work on any unlocked state.
+ *
+ * When setting the owner field, we must preserve the low flag bits.
+ *
+ * Be careful with @handoff, only set that in a wait-loop (where you set
+ * HANDOFF) to avoid recursive lock attempts.
*/

kernel/locking/mutex.c +13 -2

-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;
 
 	owner = atomic_long_read(&lock->owner);
 	for (;;) { /* must loop, can race against a flag */
-		unsigned long old;
+		unsigned long old, flags = __owner_flags(owner);
+
+		if (__owner_task(owner)) {
+			if (handoff&&  unlikely(__owner_task(owner) == current)) {
+				/*
+				 * Provide ACQUIRE semantics for the lock-handoff.
+				 *
+				 * We cannot easily use load-acquire here, since
+				 * the actual load is a failed cmpxchg, which
+				 * doesn't imply any barriers.
+				 *
+				 * Also, this is a fairly unlikely scenario, and

Review

Quoted 2 lines

(quoted diff) +2 -0

+				 * this contains the cost.
+				 */

I am not so sure about your comment here. I guess you are referring to the atomic_long_cmpxchg_acquire below for the failed cmpxchg. However, it is also possible that the path can be triggered on the first round without cmpxchg. Maybe we can do a load_acquire on the owner again to satisfy this requirement without a smp_mb().

Quoted 14 lines

kernel/locking/mutex.c +5 -3

+				smp_mb(); /* ACQUIRE */
+				return true;
+			}
 
-		if (__owner_task(owner))
 			return false;
+		}
 
-		old = atomic_long_cmpxchg_acquire(&lock->owner, owner,
-						  curr | __owner_flags(owner));
+		/*

kernel/locking/mutex.c static inline unsigned long __owner_flag · line 119

Quoted 10 lines

kernel/locking/mutex.c +8 -0

+		 * We set the HANDOFF bit, we must make sure it doesn't live
+		 * past the point where we acquire it. This would be possible
+		 * if we (accidentally) set the bit on an unlocked mutex.
+		 */
+		if (handoff)
+			flags&= ~MUTEX_FLAG_HANDOFF;
+
+		old = atomic_long_cmpxchg_acquire(&lock->owner, owner, curr | flags);
 		if (old == owner)
 			return true;

Other than that, the code is fine.

Reviewed-by Waiman Long <[email protected]>

Implement lock handoff to avoid lock starvation.

Lock starvation is possible because mutex_lock() allows lock stealing, where a running (or optimistic spinning) task beats the woken waiter to the acquire.

Lock stealing is an important performance optimization because waiting for a waiter to wake up and get runtime can take a significant time, during which everyboy would stall on the lock.

The down-side is of course that it allows for starvation.

This patch has the waiter requesting a handoff if it fails to acquire the lock upon waking. This re-introduces some of the wait time, because once we do a handoff we have to wait for the waiter to wake up again.

A future patch will add a round of optimistic spinning to attempt to alleviate this penalty, but if that turns out to not be enough, we can add a counter and only request handoff after multiple failed wakeups.

There are a few tricky implementation details:

- accepting a handoff must only be done in the wait-loop. Since the handoff condition is owner == current, it can easily cause recursive locking trouble.

- accepting the handoff must be careful to provide the ACQUIRE semantics.

- having the HANDOFF bit set on unlock requires care, we must not clear the owner.

- we must be careful to not leave HANDOFF set after we've acquired the lock. The tricky scenario is setting the HANDOFF bit on an unlocked mutex.

If we abort the mutex_lock() due to an interrupt, or other error from

ww_mutex, we need to relinquish the handoff flag if we applied it. Otherwise, we may cause missed wakeups as the current owner may try to handoff to a new thread that is not expecting the handoff and so sleep thinking the lock is already claimed (and since the owner unlocked there may never be a new wakeup).

Quoted 1 line · Chris Wilson

If we abort the mutex_lock() due to an interrupt, or other error from

s/interrupt/signal/, right?

Quoted 5 lines

ww_mutex, we need to relinquish the handoff flag if we applied it. Otherwise, we may cause missed wakeups as the current owner may try to handoff to a new thread that is not expecting the handoff and so sleep thinking the lock is already claimed (and since the owner unlocked there may never be a new wakeup).

Isn't that the exact same scenario as Nicolai fixed here:

http://lkml.kernel.org/r/[email protected]

Did you, like Nicolai, find this by inspection, or can you reproduce?

FWIW, I have the below patch that should also solve this problem afaict.

Quoted 1 line · Peter Zijlstra

FWIW, I have the below patch that should also solve this problem afaict.

Thanks, I shall see if makes my machines happy.

kernel/locking/mutex.c +11 -4

@@ -631,13 +631,21 @@ __mutex_lock_common(struct mutex *lock,
 
 	lock_contended(&lock->dep_map, ip);
 
+	set_task_state(task, state);
 	for (;;) {
+		/*
+		 * Once we hold wait_lock, we're serialized against
+		 * mutex_unlock() handing the lock off to us, do a trylock
+		 * before testing the error conditions to make sure we pick up
+		 * the handoff.
+		 */
 		if (__mutex_trylock(lock, first))
-			break;
+			goto acquired;
 
 		/*
-		 * got a signal? (This code gets eliminated in the
-		 * TASK_UNINTERRUPTIBLE case.)
+		 * Check for signals and wound conditions while holding
+		 * wait_lock. This ensures the lock cancellation is ordered
+		 * against mutex_unlock() and wake-ups do not go missing.
 		 */
 		if (unlikely(signal_pending_state(state, task))) {
 			ret = -EINTR;

@@ -650,16 +658,27 @@ __mutex_lock_common(struct mutex *lock,
 				goto err;
 		}
 
-		__set_task_state(task, state);

Hi Peter,

I'm struggling to get my head around the handoff code after this change...

Quoted 32 lines · Peter Zijlstra

kernel/locking/mutex.c +11 -4

@@ -631,13 +631,21 @@ __mutex_lock_common(struct mutex *lock,
 
 	lock_contended(&lock->dep_map, ip);
 
+	set_task_state(task, state);
 	for (;;) {
+		/*
+		 * Once we hold wait_lock, we're serialized against
+		 * mutex_unlock() handing the lock off to us, do a trylock
+		 * before testing the error conditions to make sure we pick up
+		 * the handoff.
+		 */
 		if (__mutex_trylock(lock, first))
-			break;
+			goto acquired;
 
 		/*
-		 * got a signal? (This code gets eliminated in the
-		 * TASK_UNINTERRUPTIBLE case.)
+		 * Check for signals and wound conditions while holding
+		 * wait_lock. This ensures the lock cancellation is ordered
+		 * against mutex_unlock() and wake-ups do not go missing.
 		 */
 		if (unlikely(signal_pending_state(state, task))) {
 			ret = -EINTR;

@@ -650,16 +658,27 @@ __mutex_lock_common(struct mutex *lock,
 				goto err;
 		}
 
-		__set_task_state(task, state);

kernel/locking/mutex.c __mutex_lock_common(struct mutex *lock, · line 669

Quoted 10 lines

kernel/locking/mutex.c +2 -1

 		spin_unlock_mutex(&lock->wait_lock, flags);
 		schedule_preempt_disabled();
-		spin_lock_mutex(&lock->wait_lock, flags);
 
 		if (!first && __mutex_waiter_is_first(lock, &waiter)) {
 			first = true;
 			__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
 		}
+
+		set_task_state(task, state);

With this change, we no longer hold the lock wit_hen we set the task state, and it's ordered strictly *after* setting the HANDOFF flag. Doesn't that mean that the unlock code can see the HANDOFF flag, issue the wakeup, but then we come in and overwrite the task state?

I'm struggling to work out whether that's an issue, but it certainly feels odd and is a change from the previous behaviour.

Quoted 53 lines · Will Deacon

Hi Peter,

I'm struggling to get my head around the handoff code after this change...

Quoted 32 lines · Peter Zijlstra

kernel/locking/mutex.c +11 -4

@@ -631,13 +631,21 @@ __mutex_lock_common(struct mutex *lock,
 
 	lock_contended(&lock->dep_map, ip);
 
+	set_task_state(task, state);
 	for (;;) {
+		/*
+		 * Once we hold wait_lock, we're serialized against
+		 * mutex_unlock() handing the lock off to us, do a trylock
+		 * before testing the error conditions to make sure we pick up
+		 * the handoff.
+		 */
 		if (__mutex_trylock(lock, first))
-			break;
+			goto acquired;
 
 		/*
-		 * got a signal? (This code gets eliminated in the
-		 * TASK_UNINTERRUPTIBLE case.)
+		 * Check for signals and wound conditions while holding
+		 * wait_lock. This ensures the lock cancellation is ordered
+		 * against mutex_unlock() and wake-ups do not go missing.
 		 */
 		if (unlikely(signal_pending_state(state, task))) {
 			ret = -EINTR;

@@ -650,16 +658,27 @@ __mutex_lock_common(struct mutex *lock,
 				goto err;
 		}
 
-		__set_task_state(task, state);

kernel/locking/mutex.c __mutex_lock_common(struct mutex *lock, · line 669

Quoted 10 lines

kernel/locking/mutex.c +2 -1

 		spin_unlock_mutex(&lock->wait_lock, flags);
 		schedule_preempt_disabled();
-		spin_lock_mutex(&lock->wait_lock, flags);
 
 		if (!first && __mutex_waiter_is_first(lock, &waiter)) {
 			first = true;
 			__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
 		}
+
+		set_task_state(task, state);

With this change, we no longer hold the lock wit_hen we set the task state, and it's ordered strictly *after* setting the HANDOFF flag. Doesn't that mean that the unlock code can see the HANDOFF flag, issue the wakeup, but then we come in and overwrite the task state?

Quoted 2 lines

I'm struggling to work out whether that's an issue, but it certainly feels odd and is a change from the previous behaviour.

Right, so I think the code is fine, since in that case the __mutex_trylock() must see the handoff and we'll break the loop and (re)set the state to RUNNING.

But you're right in that its slightly odd. I'll reorder them and put the set_task_state() above the !first thing.

Quoted 61 lines · Peter Zijlstra

Quoted 53 lines · Will Deacon

Hi Peter,

I'm struggling to get my head around the handoff code after this change...

Quoted 32 lines · Peter Zijlstra

kernel/locking/mutex.c +11 -4

@@ -631,13 +631,21 @@ __mutex_lock_common(struct mutex *lock,
 
 	lock_contended(&lock->dep_map, ip);
 
+	set_task_state(task, state);
 	for (;;) {
+		/*
+		 * Once we hold wait_lock, we're serialized against
+		 * mutex_unlock() handing the lock off to us, do a trylock
+		 * before testing the error conditions to make sure we pick up
+		 * the handoff.
+		 */
 		if (__mutex_trylock(lock, first))
-			break;
+			goto acquired;
 
 		/*
-		 * got a signal? (This code gets eliminated in the
-		 * TASK_UNINTERRUPTIBLE case.)
+		 * Check for signals and wound conditions while holding
+		 * wait_lock. This ensures the lock cancellation is ordered
+		 * against mutex_unlock() and wake-ups do not go missing.
 		 */
 		if (unlikely(signal_pending_state(state, task))) {
 			ret = -EINTR;

@@ -650,16 +658,27 @@ __mutex_lock_common(struct mutex *lock,
 				goto err;
 		}
 
-		__set_task_state(task, state);

kernel/locking/mutex.c __mutex_lock_common(struct mutex *lock, · line 669

Quoted 10 lines

kernel/locking/mutex.c +2 -1

 		spin_unlock_mutex(&lock->wait_lock, flags);
 		schedule_preempt_disabled();
-		spin_lock_mutex(&lock->wait_lock, flags);
 
 		if (!first && __mutex_waiter_is_first(lock, &waiter)) {
 			first = true;
 			__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
 		}
+
+		set_task_state(task, state);

With this change, we no longer hold the lock wit_hen we set the task state, and it's ordered strictly *after* setting the HANDOFF flag. Doesn't that mean that the unlock code can see the HANDOFF flag, issue the wakeup, but then we come in and overwrite the task state?

Quoted 2 lines

I'm struggling to work out whether that's an issue, but it certainly feels odd and is a change from the previous behaviour.

Right, so I think the code is fine, since in that case the __mutex_trylock() must see the handoff and we'll break the loop and (re)set the state to RUNNING.

Quoted 2 lines

But you're right in that its slightly odd. I'll reorder them and put the set_task_state() above the !first thing.

Humm,.. we might actually rely on this order, since the MB implied by set_task_state() is the only thing that separates the store of __mutex_set_flag() from the load of __mutex_trylock(), and those should be ordered I think.

Argh, completely messed up my brain. I'll not touch it and think on this again tomorrow.

But __mutex_set_flag() and __mutex_trylock() actually touch the same atomic word? So we don't need extra things to order them?

Review

Quoted 6 lines

(quoted diff) +2 -0

 		if (!first && __mutex_waiter_is_first(lock, &waiter)) {
 			first = true;
 			__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
 		}
+
+		set_task_state(task, state);

With this change, we no longer hold the lock wit_hen we set the task state, and it's ordered strictly *after* setting the HANDOFF flag. Doesn't that mean that the unlock code can see the HANDOFF flag, issue the wakeup, but then we come in and overwrite the task state?

I'm struggling to work out whether that's an issue, but it certainly feels odd and is a change from the previous behaviour.

Subject: sched: Better explain sleep/wakeup
From: Peter Zijlstra <[email protected]>
Date: Wed Oct 19 15:45:27 CEST 2016

There were a few questions wrt how sleep-wakeup works. Try and explain it more.

Requested-by Will Deacon <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---

include/linux/sched.h |   52 ++++++++++++++++++++++++++++++++++----------------
kernel/sched/core.c   |   15 +++++++-------
2 files changed, 44 insertions(+), 23 deletions(-)

include/linux/sched.h +1 -12

@@ -262,20 +262,9 @@ extern char ___assert_task_state[1 - 2*!
 #define set_task_state(tsk, state_value)			\
 	do {							\
 		(tsk)->task_state_change = _THIS_IP_;		\
-		smp_store_mb((tsk)->state, (state_value));		\
+		smp_store_mb((tsk)->state, (state_value));	\
 	} while (0)
 
-/*
- * set_current_state() includes a barrier so that the write of current->state
- * is correctly serialised wrt the caller's subsequent test of whether to
- * actually sleep:
- *
- *	set_current_state(TASK_UNINTERRUPTIBLE);
- *	if (do_i_need_to_sleep())
- *		schedule();
- *
- * If the caller does not need such serialisation then use __set_current_state()
- */
 #define __set_current_state(state_value)			\
 	do {							\
 		current->task_state_change = _THIS_IP_;		\

@@ -284,11 +273,19 @@ extern char ___assert_task_state[1 - 2*!
 #define set_current_state(state_value)				\

include/linux/sched.h +32 -3

+ *
+ * The only reason is that computing current can be more expensive than
+ * using a pointer that's already available.
+ *
+ * Therefore, see set_current_state().
+ */
 #define __set_task_state(tsk, state_value)		\
 	do { (tsk)->state = (state_value); } while (0)
 #define set_task_state(tsk, state_value)		\

@@ -299,11 +296,34 @@ extern char ___assert_task_state[1 - 2*!
 * is correctly serialised wrt the caller's subsequent test of whether to
 * actually sleep:
 *
+ *   for (;;) {
 *	set_current_state(TASK_UNINTERRUPTIBLE);
- *	if (do_i_need_to_sleep())
- *		schedule();
+ *	if (!need_sleep)
+ *		break;
+ *
+ *	schedule();
+ *   }
+ *   __set_current_state(TASK_RUNNING);
+ *
+ * If the caller does not need such serialisation (because, for instance, the
+ * condition test and condition change and wakeup are under the same lock) then
+ * use __set_current_state().
+ *
+ * The above is typically ordered against the wakeup, which does:
+ *
+ *	need_sleep = false;
+ *	wake_up_state(p, TASK_UNINTERRUPTIBLE);
+ *
+ * Where wake_up_state() (and all other wakeup primitives) imply enough
+ * barriers to order the store of the variable against wakeup.
+ *
+ * Wakeup will do: if (@state & p->state) p->state = TASK_RUNNING, that is,
+ * once it observes the TASK_UNINTERRUPTIBLE store the waking CPU can issue a
+ * TASK_RUNNING store which can collide with __set_current_state(TASK_RUNNING).
+ *
+ * This is obviously fine, since they both store the exact same value.
 *
- * If the caller does not need such serialisation then use __set_current_state()
+ * Also see the comments of try_to_wake_up().
 */
 #define __set_current_state(state_value)		\
 	do { current->state = (state_value); } while (0)

kernel/sched/core.c +8 -7

@@ -2000,14 +2000,15 @@ static void ttwu_queue(struct task_struc
 * @state: the mask of task states that can be woken
 * @wake_flags: wake modifier flags (WF_*)
 *
- * Put it on the run-queue if it's not already there. The "current"
- * thread is always on the run-queue (except when the actual
- * re-schedule is in progress), and as such you're allowed to do
- * the simpler "current->state = TASK_RUNNING" to mark yourself
- * runnable without the overhead of this.
+ * If (@state & @p->state) @p->state = TASK_RUNNING.
 *
- * Return: %true if @p was woken up, %false if it was already running.
- * or @state didn't match @p's state.
+ * If the task was not queued/runnable, also place it back on a runqueue.
+ *
+ * Atomic against schedule() which would dequeue a task, also see
+ * set_current_state().
+ *
+ * Return: %true if @p->state changes (an actual wakeup was done),
+ *	   %false otherwise.
 */

static int
try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)

Quoted 40 lines · Peter Zijlstra

Subject: sched: Better explain sleep/wakeup
From: Peter Zijlstra <[email protected]>
Date: Wed Oct 19 15:45:27 CEST 2016

There were a few questions wrt how sleep-wakeup works. Try and explain it more.

Requested-by Will Deacon <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---

include/linux/sched.h |   52 ++++++++++++++++++++++++++++++++++----------------
kernel/sched/core.c   |   15 +++++++-------
2 files changed, 44 insertions(+), 23 deletions(-)

include/linux/sched.h +1 -12

@@ -262,20 +262,9 @@ extern char ___assert_task_state[1 - 2*!
 #define set_task_state(tsk, state_value)			\
 	do {							\
 		(tsk)->task_state_change = _THIS_IP_;		\
-		smp_store_mb((tsk)->state, (state_value));		\
+		smp_store_mb((tsk)->state, (state_value));	\
 	} while (0)
 
-/*
- * set_current_state() includes a barrier so that the write of current->state
- * is correctly serialised wrt the caller's subsequent test of whether to
- * actually sleep:
- *
- *	set_current_state(TASK_UNINTERRUPTIBLE);
- *	if (do_i_need_to_sleep())
- *		schedule();
- *
- * If the caller does not need such serialisation then use __set_current_state()
- */
 #define __set_current_state(state_value)			\
 	do {							\
 		current->task_state_change = _THIS_IP_;		\

@@ -284,11 +273,19 @@ extern char ___assert_task_state[1 - 2*!
 #define set_current_state(state_value)				\

include/linux/sched.h extern char ___assert_task_state[1 - 2*! · line 282

Quoted 10 lines

include/linux/sched.h +3 -1

 	do {							\
 		current->task_state_change = _THIS_IP_;		\
-		smp_store_mb(current->state, (state_value));		\
+		smp_store_mb(current->state, (state_value));	\
 	} while (0)
 
 #else
 
+/*
+ * @tsk had better be current, or you get to keep the pieces.

That reminds me we were getting rid of the set_task_state() calls. Bcache was pending, being only user in the kernel that doesn't actually use current; but instead breaks newly (yet blocked/uninterruptible) created garbage collection kthread. I cannot figure out why this is done (ie purposely accounting the load avg. Furthermore gc kicks in in very specific scenarios obviously, such as as by the allocator task, so I don't see why bcache gc should want to be interruptible.

Kent, Jens, can we get rid of this?

drivers/md/bcache/btree.c +0 -0

index 76f7534d1dd1..6e3c358b5759 100644

Quoted 40 lines · Peter Zijlstra

Subject: sched: Better explain sleep/wakeup
From: Peter Zijlstra <[email protected]>
Date: Wed Oct 19 15:45:27 CEST 2016

There were a few questions wrt how sleep-wakeup works. Try and explain it more.

Requested-by Will Deacon <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---

include/linux/sched.h |   52 ++++++++++++++++++++++++++++++++++----------------
kernel/sched/core.c   |   15 +++++++-------
2 files changed, 44 insertions(+), 23 deletions(-)

include/linux/sched.h +1 -12

@@ -262,20 +262,9 @@ extern char ___assert_task_state[1 - 2*!
 #define set_task_state(tsk, state_value)			\
 	do {							\
 		(tsk)->task_state_change = _THIS_IP_;		\
-		smp_store_mb((tsk)->state, (state_value));		\
+		smp_store_mb((tsk)->state, (state_value));	\
 	} while (0)
 
-/*
- * set_current_state() includes a barrier so that the write of current->state
- * is correctly serialised wrt the caller's subsequent test of whether to
- * actually sleep:
- *
- *	set_current_state(TASK_UNINTERRUPTIBLE);
- *	if (do_i_need_to_sleep())
- *		schedule();
- *
- * If the caller does not need such serialisation then use __set_current_state()
- */
 #define __set_current_state(state_value)			\
 	do {							\
 		current->task_state_change = _THIS_IP_;		\

@@ -284,11 +273,19 @@ extern char ___assert_task_state[1 - 2*!
 #define set_current_state(state_value)				\

include/linux/sched.h extern char ___assert_task_state[1 - 2*! · line 282

Quoted 10 lines

include/linux/sched.h +3 -1

 	do {							\
 		current->task_state_change = _THIS_IP_;		\
-		smp_store_mb(current->state, (state_value));		\
+		smp_store_mb(current->state, (state_value));	\
 	} while (0)
 
 #else
 
+/*
+ * @tsk had better be current, or you get to keep the pieces.

That reminds me we were getting rid of the set_task_state() calls. Bcache was pending, being only user in the kernel that doesn't actually use current; but instead breaks newly (yet blocked/uninterruptible) created garbage collection kthread. I cannot figure out why this is done (ie purposely accounting the load avg. Furthermore gc kicks in in very specific scenarios obviously, such as as by the allocator task, so I don't see why bcache gc should want to be interruptible.

Kent, Jens, can we get rid of this?

Quoted 61 lines · Davidlohr Bueso

Quoted 40 lines · Peter Zijlstra

Subject: sched: Better explain sleep/wakeup
From: Peter Zijlstra <[email protected]>
Date: Wed Oct 19 15:45:27 CEST 2016

There were a few questions wrt how sleep-wakeup works. Try and explain it more.

Requested-by Will Deacon <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---

include/linux/sched.h |   52 ++++++++++++++++++++++++++++++++++----------------
kernel/sched/core.c   |   15 +++++++-------
2 files changed, 44 insertions(+), 23 deletions(-)

include/linux/sched.h +1 -12

@@ -262,20 +262,9 @@ extern char ___assert_task_state[1 - 2*!
 #define set_task_state(tsk, state_value)			\
 	do {							\
 		(tsk)->task_state_change = _THIS_IP_;		\
-		smp_store_mb((tsk)->state, (state_value));		\
+		smp_store_mb((tsk)->state, (state_value));	\
 	} while (0)
 
-/*
- * set_current_state() includes a barrier so that the write of current->state
- * is correctly serialised wrt the caller's subsequent test of whether to
- * actually sleep:
- *
- *	set_current_state(TASK_UNINTERRUPTIBLE);
- *	if (do_i_need_to_sleep())
- *		schedule();
- *
- * If the caller does not need such serialisation then use __set_current_state()
- */
 #define __set_current_state(state_value)			\
 	do {							\
 		current->task_state_change = _THIS_IP_;		\

@@ -284,11 +273,19 @@ extern char ___assert_task_state[1 - 2*!
 #define set_current_state(state_value)				\

include/linux/sched.h extern char ___assert_task_state[1 - 2*! · line 282

Quoted 10 lines

include/linux/sched.h +3 -1

 	do {							\
 		current->task_state_change = _THIS_IP_;		\
-		smp_store_mb(current->state, (state_value));		\
+		smp_store_mb(current->state, (state_value));	\
 	} while (0)
 
 #else
 
+/*
+ * @tsk had better be current, or you get to keep the pieces.

That reminds me we were getting rid of the set_task_state() calls. Bcache was pending, being only user in the kernel that doesn't actually use current; but instead breaks newly (yet blocked/uninterruptible) created garbage collection kthread. I cannot figure out why this is done (ie purposely accounting the load avg. Furthermore gc kicks in in very specific scenarios obviously, such as as by the allocator task, so I don't see why bcache gc should want to be interruptible.

Quoted 1 line

Kent, Jens, can we get rid of this?

Here's a patch that just fixes the way gc gets woken up. Eric, you want to take this?

-- >8 --
Subject: [PATCH] bcache: Make gc wakeup saner

This lets us ditch a set_task_state() call.

Signed-off-by Kent Overstreet <[email protected]>

---
 drivers/md/bcache/bcache.h  |  4 ++--
 drivers/md/bcache/btree.c   | 39 ++++++++++++++++++++-------------------
 drivers/md/bcache/btree.h   |  3 +--
 drivers/md/bcache/request.c |  4 +---
 drivers/md/bcache/super.c   |  2 ++
 5 files changed, 26 insertions(+), 26 deletions(-)

drivers/md/bcache/bcache.h +2 -2

index 6b420a55c7..c3ea03c9a1 100644
@@ -425,7 +425,7 @@ struct cache {
 	 * until a gc finishes - otherwise we could pointlessly burn a ton of
 	 * cpu
 	 */
-	unsigned		invalidate_needs_gc:1;
+	unsigned		invalidate_needs_gc;
 
 	bool			discard; /* Get rid of? */
 

@@ -593,8 +593,8 @@ struct cache_set {
 
 	/* Counts how many sectors bio_insert has added to the cache */
 	atomic_t		sectors_to_gc;
+	wait_queue_head_t	gc_wait;
 
-	wait_queue_head_t	moving_gc_wait;
 	struct keybuf		moving_gc_keys;
 	/* Number of moving GC bios in flight */
 	struct semaphore	moving_in_flight;

drivers/md/bcache/btree.c +20 -19

index 81d3db40cd..2efdce0724 100644
@@ -1757,32 +1757,34 @@ static void bch_btree_gc(struct cache_set *c)
 	bch_moving_gc(c);
 }
 
-static int bch_gc_thread(void *arg)
+static bool gc_should_run(struct cache_set *c)
 {
-	struct cache_set *c = arg;
 	struct cache *ca;
 	unsigned i;
 
-	while (1) {
-again:
-		bch_btree_gc(c);
+	for_each_cache(ca, c, i)
+		if (ca->invalidate_needs_gc)
+			return true;
 
-		set_current_state(TASK_INTERRUPTIBLE);
-		if (kthread_should_stop())
-			break;
+	if (atomic_read(&c->sectors_to_gc) < 0)
+		return true;
 
-		mutex_lock(&c->bucket_lock);
+	return false;
+}
 
-		for_each_cache(ca, c, i)
-			if (ca->invalidate_needs_gc) {
-				mutex_unlock(&c->bucket_lock);
-				set_current_state(TASK_RUNNING);
-				goto again;
-			}
+static int bch_gc_thread(void *arg)
+{
+	struct cache_set *c = arg;
 
-		mutex_unlock(&c->bucket_lock);
+	while (1) {
+		wait_event_interruptible(c->gc_wait,
+			   kthread_should_stop() || gc_should_run(c));
 
-		schedule();
+		if (kthread_should_stop())
+			break;
+
+		set_gc_sectors(c);
+		bch_btree_gc(c);
 	}
 
 	return 0;

@@ -1790,11 +1792,10 @@ static int bch_gc_thread(void *arg)
 
 int bch_gc_thread_start(struct cache_set *c)
 {
-	c->gc_thread = kthread_create(bch_gc_thread, c, "bcache_gc");
+	c->gc_thread = kthread_run(bch_gc_thread, c, "bcache_gc");
 	if (IS_ERR(c->gc_thread))
 		return PTR_ERR(c->gc_thread);
 
-	set_task_state(c->gc_thread, TASK_INTERRUPTIBLE);
 	return 0;
 }
 

drivers/md/bcache/btree.h +1 -2

index 5c391fa01b..9b80417cd5 100644
@@ -260,8 +260,7 @@ void bch_initial_mark_key(struct cache_set *, int, struct bkey *);
 
 static inline void wake_up_gc(struct cache_set *c)
 {
-	if (c->gc_thread)
-		wake_up_process(c->gc_thread);
+	wake_up(&c->gc_wait);
 }
 
 #define MAP_DONE	0

drivers/md/bcache/request.c +1 -3

index 40ffe5e424..a37c1776f2 100644
@@ -196,10 +196,8 @@ static void bch_data_insert_start(struct closure *cl)
 	struct data_insert_op *op = container_of(cl, struct data_insert_op, cl);
 	struct bio *bio = op->bio, *n;
 
-	if (atomic_sub_return(bio_sectors(bio), &op->c->sectors_to_gc) < 0) {
-		set_gc_sectors(op->c);
+	if (atomic_sub_return(bio_sectors(bio), &op->c->sectors_to_gc) < 0)
 		wake_up_gc(op->c);
-	}
 
 	if (op->bypass)
 		return bch_data_invalidate(cl);

drivers/md/bcache/super.c +2 -0

index 849ad441cd..66669c8f41 100644
@@ -1491,6 +1491,7 @@ struct cache_set *bch_cache_set_alloc(struct cache_sb *sb)
 	mutex_init(&c->bucket_lock);
 	init_waitqueue_head(&c->btree_cache_wait);
 	init_waitqueue_head(&c->bucket_wait);
+	init_waitqueue_head(&c->gc_wait);
 	sema_init(&c->uuid_write_mutex, 1);
 
 	spin_lock_init(&c->btree_gc_time.lock);

@@ -1550,6 +1551,7 @@ static void run_cache_set(struct cache_set *c)
 
 	for_each_cache(ca, c, i)
 		c->nbuckets += ca->sb.nbuckets;
+	set_gc_sectors(c);
 
 	if (CACHE_SYNC(&c->sb)) {
 		LIST_HEAD(journal);

Signature · 6 lines

2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sure, I'll put it up with my -rc2 pull request to Jens.

A couple of sanity checks (for my understanding at least):

Why does bch_data_insert_start() no longer need to call set_gc_sectors(op->c) now that bch_cache_set_alloc and bch_gc_thread do?

Does bch_cache_set_alloc() even need to call set_gc_sectors since bch_gc_thread() does before calling bch_btree_gc?

Also I'm curious, why change invalidate_needs_gc from a bitfield?

Doesn't really matter yet, but pull the HANDOFF and trylock out from under the wait_lock.

The intention is to add an optimistic spin loop here, which requires we do not hold the wait_lock, so shuffle code around in preparation.

Also clarify the purpose of taking the wait_lock in the wait loop, its tempting to want to avoid it altogether, but the cancellation cases need to to avoid losing wakeups.

Suggested-by Waiman Long<[email protected]>

Signed-off-by Peter Zijlstra (Intel)<[email protected]>

---
  kernel/locking/mutex.c |   30 +++++++++++++++++++++++++-----
  1 file changed, 25 insertions(+), 5 deletions(-)

kernel/locking/mutex.c +10 -4

  	for (;;) {
+		/*
+		 * Once we hold wait_lock, we're serialized against
+		 * mutex_unlock() handing the lock off to us, do a trylock
+		 * before testing the error conditions to make sure we pick up
+		 * the handoff.
+		 */
  		if (__mutex_trylock(lock, first))
-			break;
+			goto acquired;
 
  		/*
-		 * got a signal? (This code gets eliminated in the
-		 * TASK_UNINTERRUPTIBLE case.)
+		 * Check for signals and wound conditions while holding
+		 * wait_lock. This ensures the lock cancellation is ordered
+		 * against mutex_unlock() and wake-ups do not go missing.
  		 */
  		if (unlikely(signal_pending_state(state, task))) {
  			ret = -EINTR;

@@ -650,16 +658,27 @@ __mutex_lock_common(struct mutex *lock,
  				goto err;
  		}
 
-		__set_task_state(task, state);

kernel/locking/mutex.c +3 -0

+		spin_lock_mutex(&lock->wait_lock, flags);
  	}
+	spin_lock_mutex(&lock->wait_lock, flags);
+acquired:
  	__set_task_state(task, TASK_RUNNING);
 
  	mutex_remove_waiter(lock,&waiter, task);

kernel/locking/mutex.c __mutex_lock_common(struct mutex *lock, · line 634

Quoted 6 lines

+++ b/kernel/locking/mutex.c

kernel/locking/mutex.c +1 -0

@@ -631,13 +631,21 @@ __mutex_lock_common(struct mutex *lock,
 
 	lock_contended(&lock->dep_map, ip);
 
+	set_task_state(task, state);

Do we want to set the state here? I am not sure if it is OK to set the task state without ever calling schedule().

Quoted 29 lines

kernel/locking/mutex.c +10 -5

 	for (;;) {
+		/*
+		 * Once we hold wait_lock, we're serialized against
+		 * mutex_unlock() handing the lock off to us, do a trylock
+		 * before testing the error conditions to make sure we pick up
+		 * the handoff.
+		 */
 		if (__mutex_trylock(lock, first))
-			break;
+			goto acquired;
 
 		/*
-		 * got a signal? (This code gets eliminated in the
-		 * TASK_UNINTERRUPTIBLE case.)
+		 * Check for signals and wound conditions while holding
+		 * wait_lock. This ensures the lock cancellation is ordered
+		 * against mutex_unlock() and wake-ups do not go missing.
 		 */
 		if (unlikely(signal_pending_state(state, task))) {
 			ret = -EINTR;

@@ -650,16 +658,27 @@ __mutex_lock_common(struct mutex *lock,
 				goto err;
 		}
 
-		__set_task_state(task, state);
 		spin_unlock_mutex(&lock->wait_lock, flags);
 		schedule_preempt_disabled();
-		spin_lock_mutex(&lock->wait_lock, flags);
 

kernel/locking/mutex.c __mutex_lock_common(struct mutex *lock, · line 669

Quoted 6 lines

kernel/locking/mutex.c +2 -0

 		if (!first&&  __mutex_waiter_is_first(lock,&waiter)) {
 			first = true;
 			__mutex_set_flag(lock, MUTEX_FLAG_HANDOFF);
 		}
+
+		set_task_state(task, state);

I would suggest keep the __set_task_state() above and change set_task_state(task, state) to set_task_state(task, TASK_RUNNING) to provide the memory barrier. Then we don't need adding __set_task_state() calls below.

kernel/locking/mutex.c __mutex_lock_common(struct mutex *lock, · line 677

Quoted 8 lines

kernel/locking/mutex.c +8 -0

+		/*
+		 * Here we order against unlock; we must either see it change
+		 * state back to RUNNING and fall through the next schedule(),
+		 * or we must see its unlock and acquire.
+		 */
+		if (__mutex_trylock(lock, first))
+			break;
+

I don't think we need a trylock here since we are going to do it at the top of the loop within wait_lock anyway.

From: Waiman Long <[email protected]>

This patch makes the waiter that sets the HANDOFF flag start spinning instead of sleeping until the handoff is complete or the owner sleeps. Otherwise, the handoff will cause the optimistic spinners to abort spinning as the handed-off owner may not be running.

Cc Ingo Molnar <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Tim Chen <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Imre Deak <[email protected]>

Cc Jason Low <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Ding Tianhong <[email protected]>

Cc Davidlohr Bueso <[email protected]>

Cc Will Deacon <[email protected]>

Signed-off-by Waiman Long <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/locking/mutex.c |   77 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 54 insertions(+), 23 deletions(-)

kernel/locking/mutex.c +27 -12

@@ -416,24 +416,39 @@ static inline int mutex_can_spin_on_owne
  *
  * Returns true when the lock was taken, otherwise false, indicating
  * that we need to jump to the slowpath and sleep.
+ *
+ * The waiter flag is set to true if the spinner is a waiter in the wait
+ * queue. The waiter-spinner will spin on the lock directly and concurrently
+ * with the spinner at the head of the OSQ, if present, until the owner is
+ * changed to itself.
  */
 static bool mutex_optimistic_spin(struct mutex *lock,
-				  struct ww_acquire_ctx *ww_ctx, const bool use_ww_ctx)
+				  struct ww_acquire_ctx *ww_ctx,
+				  const bool use_ww_ctx, const bool waiter)
 {
 	struct task_struct *task = current;
 
-	if (!mutex_can_spin_on_owner(lock))
-		goto done;
+	if (!waiter) {
+		/*
+		 * The purpose of the mutex_can_spin_on_owner() function is
+		 * to eliminate the overhead of osq_lock() and osq_unlock()
+		 * in case spinning isn't possible. As a waiter-spinner
+		 * is not going to take OSQ lock anyway, there is no need
+		 * to call mutex_can_spin_on_owner().
+		 */
+		if (!mutex_can_spin_on_owner(lock))
+			goto fail;
 
-	/*
-	 * In order to avoid a stampede of mutex spinners trying to
-	 * acquire the mutex all at once, the spinners need to take a
-	 * MCS (queued) lock first before spinning on the owner field.
-	 */
-	if (!osq_lock(&lock->osq))
-		goto done;
+		/*
+		 * In order to avoid a stampede of mutex spinners trying to
+		 * acquire the mutex all at once, the spinners need to take a
+		 * MCS (queued) lock first before spinning on the owner field.
+		 */
+		if (!osq_lock(&lock->osq))
+			goto fail;
+	}
 
-	while (true) {
+	for (;;) {
 		struct task_struct *owner;
 
 		if (use_ww_ctx && ww_ctx->acquired > 0) {

@@ -449,7 +464,7 @@ static bool mutex_optimistic_spin(struct
 			 * performed the optimistic spinning cannot be done.
 			 */
 			if (READ_ONCE(ww->ctx))
-				break;
+				goto fail_unlock;
 		}
 
 		/*