Re: [patch V3 64/64] locking/rtmutex: Add adaptive spinwait mechanism

[Davidlohr Bueso <dave@stgolabs.net>]

On Thu, 05 Aug 2021, Thomas Gleixner wrote:

>+#ifdef CONFIG_SMP
>+/*
>+ * Note that owner is a speculative pointer and dereferencing relies
>+ * on rcu_read_lock() and the check against the lock owner.
>+ */

I think the description we have in mutex.c is better.

>+static bool rtmutex_adaptive_spinwait(struct rt_mutex_base *lock,
>+				     struct task_struct *owner)

I realize that adaptive spinning is the original term from Greg Haskins,
but as Peter suggested, rt_mutex_spin_on_owner() would probably be a
better name upstream considering all our other sleeping locks.

>+{
>+	bool res = true;
>+
>+	rcu_read_lock();
>+	for (;;) {
>+		/* Owner changed. Trylock again */
>+		if (owner != rt_mutex_owner(lock))
>+			break;
>+		/*
>+		 * Ensure that owner->on_cpu is dereferenced _after_
>+		 * checking the above to be valid.
>+		 */
>+		barrier();
>+		if (!owner->on_cpu || need_resched() ||
>+		    vcpu_is_preempted(task_cpu(owner))) {

I'm thinking we should also check for if the spinning waiter is no
longer the top-waiter, which could have changed while busy waiting.

Thanks,
Davidlohr

diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 7522c3abacb6..c6925ebb3c9f 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1284,12 +1284,9 @@ static __always_inline void __rt_mutex_unlock(struct rt_mutex_base *lock)
  }
  
  #ifdef CONFIG_SMP
-/*
- * Note that owner is a speculative pointer and dereferencing relies
- * on rcu_read_lock() and the check against the lock owner.
- */
  static bool rtmutex_adaptive_spinwait(struct rt_mutex_base *lock,
-				     struct task_struct *owner)
+				   struct rt_mutex_waiter *waiter,
+				   struct task_struct *owner)
  {
  	bool res = true;
  
@@ -1299,11 +1296,18 @@ static bool rtmutex_adaptive_spinwait(struct rt_mutex_base *lock,
  		if (owner != rt_mutex_owner(lock))
  			break;
  		/*
-		 * Ensure that owner->on_cpu is dereferenced _after_
-		 * checking the above to be valid.
+		 * Ensure we emit the owner->on_cpu, dereference _after_
+		 * checking lock->owner still matches owner. If that fails,
+		 * owner might point to freed memory. If it still matches,
+		 * the rcu_read_lock() ensures the memory stays valid.
+		 *
+		 * Also account for changes in the lock's top-waiter, if
+		 * it's not us, it was updated while busy waiting.
  		 */
  		barrier();
+
  		if (!owner->on_cpu || need_resched() ||
+		    waiter != rt_mutex_top_waiter(lock) ||
  		    vcpu_is_preempted(task_cpu(owner))) {
  			res = false;
  			break;
@@ -1315,7 +1319,8 @@ static bool rtmutex_adaptive_spinwait(struct rt_mutex_base *lock,
  }
  #else
  static bool rtmutex_adaptive_spinwait(struct rt_mutex_base *lock,
-				     struct task_struct *owner)
+				   struct rt_mutex_waiter *waiter,
+				   struct task_struct *owner)
  {
  	return false;
  }
@@ -1434,7 +1439,7 @@ static int __sched rt_mutex_slowlock_block(struct rt_mutex_base *lock,
  			owner = NULL;
  		raw_spin_unlock_irq(&lock->wait_lock);
  
-		if (!owner || !rtmutex_adaptive_spinwait(lock, owner))
+		if (!owner || !rtmutex_adaptive_spinwait(lock, waiter, owner))
  			schedule();
  
  		raw_spin_lock_irq(&lock->wait_lock);
@@ -1616,7 +1621,7 @@ static void __sched rtlock_slowlock_locked(struct rt_mutex_base *lock)
  			owner = NULL;
  		raw_spin_unlock_irq(&lock->wait_lock);
  
-		if (!owner || !rtmutex_adaptive_spinwait(lock, owner))
+		if (!owner || !rtmutex_adaptive_spinwait(lock, waiter, owner))
  			schedule_rtlock();
  
  		raw_spin_lock_irq(&lock->wait_lock);