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

From: Davidlohr Bueso
Date: Mon Aug 09 2021 - 16:28:19 EST


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