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

Davidlohr Bueso
2021-08-09T20:28:19Z

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