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

[Davidlohr Bueso]

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