Re: [RFC PATCH 04/16] sched/core: Activate blocked donor when no owner is found
From: K Prateek Nayak
Date: Wed Sep 16 2026 - 00:16:56 EST
Hello Andrea,
Sorry I forgot to push send on this and only realised today.
On 8/31/2026 8:37 PM, Andrea Righi wrote:
>> ============================================
>> Experiment 3: STEAL + Temporary swap to idle
>> ============================================
>>
>> the unlock will temporarily swap to rq->idle of the lock owner's CPU with
>> MUTEX_FLAG_STEAL set to allow grabbing the task until the the waiter wakes
>> up and manages to grab the task itself for !HANDOFF cases. With that,
>> numbers are very close:
>>
>> diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
>> index 8a85912d7ee6..187f95544453 100644
>> --- a/kernel/locking/mutex.c
>> +++ b/kernel/locking/mutex.c
>> @@ -92,7 +92,16 @@ static inline struct task_struct *__mutex_trylock_common(struct mutex *lock, boo
>> unsigned long task = owner & ~MUTEX_FLAGS;
>>
>> if (task) {
>> - if (flags & MUTEX_FLAG_PICKUP) {
>> + if (sched_proxy_exec() && (flags & MUTEX_FLAG_STEAL)) {
>> + /*
>> + * STEAL cannot be set after HANDOFF has been
>> + * initiated. If STEAL is set, clear it and
>> + * preserve other flags
>> + */
>> + MUTEX_WARN_ON(flags & (MUTEX_FLAG_PICKUP));
>> + flags &= ~MUTEX_FLAG_STEAL;
>> + task = curr;
>> + }else if (flags & MUTEX_FLAG_PICKUP) {
>> if (task != curr)
>> break;
>> flags &= ~MUTEX_FLAG_PICKUP;
>> @@ -104,7 +113,7 @@ static inline struct task_struct *__mutex_trylock_common(struct mutex *lock, boo
>> break;
>> }
>> } else {
>> - MUTEX_WARN_ON(flags & (MUTEX_FLAG_HANDOFF | MUTEX_FLAG_PICKUP));
>> + MUTEX_WARN_ON(flags & (MUTEX_FLAG_HANDOFF | MUTEX_FLAG_PICKUP | MUTEX_FLAG_STEAL));
>> task = curr;
>> }
>>
>> @@ -242,7 +251,41 @@ __mutex_remove_waiter(struct mutex *lock, struct mutex_waiter *waiter)
>> __must_hold(&lock->wait_lock)
>> {
>> if (list_empty(&waiter->list)) {
>> - __mutex_clear_flag(lock, MUTEX_FLAGS);
>> + /*
>> + * The last waiter can be interrupted before the full
>> + * unlock with STEAL is done.
>> + *
>> + * LOCK lock->wait_lock
>> + *
>> + * __mutex_trylock()
>> + * // Sees old owner __mutex_unlock_slowpath()
>> + * return owner; atomic_long_cmpxchg_release(&owner, idle | STEAL)
>> + * // Succeeds
>> + * if (signal_pending())
>> + * goto err;
>> + *
>> + * err:
>> + * __mutex_remove_waiter()
>> + * __mutex_clear_flag(MUTEX_FLAGS)
>> + * lock->first_waiter = NULL;
>> + *
>> + * UNLOCK lock->wait_lock LOCK lock->wait_lock
>> + * waiter = lock->first_waiter; // NULL
>> + * // No wakeup
>> + *
>> + * !!! lock->owner stuck as rq->idle without STEAL set !!!
>> + *
>> + * Persis the STEAL flag to prevent an idle task to
>> + * linger as lock owner. __mutex_trylock_fast() will
>> + * fail temporarily for first contender but following
>> + * __mutex_trylock_common() will do the right thing.
>> + *
>> + * XXX: This can also be solved by doing a
>> + * atomic_try_cmpxchg() or__mutex_clear_flag() in
>> + * __mutex_unlock_slowpath() if steal is set but no
>> + * waiter is found under lock->wait_lock.
>> + */
>> + __mutex_clear_flag(lock, MUTEX_FLAGS & ~MUTEX_FLAG_STEAL);
>> lock->first_waiter = NULL;
>> } else {
>> if (lock->first_waiter == waiter)
>> @@ -274,7 +317,6 @@ static void __mutex_handoff(struct mutex *lock, struct task_struct *task)
>> new |= (unsigned long)task;
>> if (task)
>> new |= MUTEX_FLAG_PICKUP;
>> -
>> if (atomic_long_try_cmpxchg_release(&lock->owner, &owner, new))
>> break;
>> }
>> @@ -389,7 +431,17 @@ bool mutex_spin_on_owner(struct mutex *lock, struct task_struct *owner,
>>
>> lockdep_assert_preemption_disabled();
>>
>> - while (__mutex_owner(lock) == owner) {
>> + for (;;) {
>> + unsigned long __owner = atomic_long_read(&lock->owner);
>> +
>> + /* If the owner changed, break out. */
>> + if (__owner_task(__owner) != owner)
>> + break;
>> +
>> + /* If lock can be stolen, break out. */
>> + if (sched_proxy_exec() && (__owner_flags(__owner) & MUTEX_FLAG_STEAL))
>> + break;
>> +
>> /*
>> * Ensure we emit the owner->on_cpu, dereference _after_
>> * checking lock->owner still matches owner. And we already
>> @@ -1006,19 +1058,42 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne
>> */
>> owner = atomic_long_read(&lock->owner);
>> for (;;) {
>> + unsigned long owner_flags;
>> +
>> MUTEX_WARN_ON(__owner_task(owner) != current);
>> MUTEX_WARN_ON(owner & MUTEX_FLAG_PICKUP);
>>
>> - if (sched_proxy_exec() && current->blocked_donor) {
>> - /* force handoff if we have a blocked_donor */
>> - owner = MUTEX_FLAG_HANDOFF;
>> - break;
>> - }
>> -
>> if (owner & MUTEX_FLAG_HANDOFF)
>> break;
>>
>> - if (atomic_long_try_cmpxchg_release(&lock->owner, &owner, __owner_flags(owner))) {
>> + owner_flags = __owner_flags(owner);
>> + if (sched_proxy_exec()) {
>> + if (current->blocked_donor) {
>> + /* force handoff if we have a blocked_donor */
>> + owner = MUTEX_FLAG_HANDOFF;
>> + break;
>> + }
>> +
>> + if (owner & MUTEX_FLAG_WAITERS) {
>> + unsigned long idle;
>> + /*
>> + * Swap the owner to current CPU's idle task
>> + * with a STEAL flag.
>> + *
>> + * The lock is free to be stolen and
>> + * __mutex_owner() will resolve to idle task
>> + * that is always ->on_rq on this CPU.
>> + *
>> + * Proxy donors will temporarily migrate here
>> + * before a wakeup or an optimistic spinner
>> + * can grab the lock.
>> + */
>> + idle = (unsigned long)idle_task(raw_smp_processor_id());
>> + owner_flags = idle | MUTEX_FLAG_STEAL | owner_flags;
>> + }
>> + }
>> +
>> + if (atomic_long_try_cmpxchg_release(&lock->owner, &owner, owner_flags)) {
>> if (owner & MUTEX_FLAG_WAITERS)
>> break;
>>
>> diff --git a/kernel/locking/mutex.h b/kernel/locking/mutex.h
>> index 3e263e98e5fc..eb4180745da0 100644
>> --- a/kernel/locking/mutex.h
>> +++ b/kernel/locking/mutex.h
>> @@ -33,8 +33,9 @@ struct mutex_waiter {
>> #define MUTEX_FLAG_WAITERS 0x01
>> #define MUTEX_FLAG_HANDOFF 0x02
>> #define MUTEX_FLAG_PICKUP 0x04
>> +#define MUTEX_FLAG_STEAL 0x08
>>
>> -#define MUTEX_FLAGS 0x07
>> +#define MUTEX_FLAGS 0x0F
>>
>> /*
>> * Internal helper function; C doesn't allow us to hide it :/
>> ---
>>
>> The results with temporary switch to idle + STEAL are:
>>
>> ==================================================================
>> Test : sched-messaging
>> Units : Normalized time in seconds
>> Interpretation: Lower is better
>> Statistic : AMean
>> ==================================================================
>> Test: vanilla handoff STEAL + handoff idle + STEAL
>> 1-groups: 3.12 (0.00 pct) 3.47 (-11.21 pct) 3.63 (-16.34 pct) 3.59 (-15.06 pct)
>> 2-groups: 3.43 (0.00 pct) 4.33 (-26.23 pct) 4.14 (-20.69 pct) 3.48 (-1.45 pct)
>> 4-groups: 4.05 (0.00 pct) 5.95 (-46.91 pct) 5.45 (-34.56 pct) 4.00 (1.23 pct)
>> 8-groups: 4.29 (0.00 pct) 9.56 (-122.84 pct) 7.80 (-81.81 pct) 4.31 (-0.46 pct)
>> 16-groups: 5.89 (0.00 pct) 12.29 (-108.65 pct) 11.89 (-101.86 pct) 5.91 (-0.33 pct)
>>
>> * Data points have > 10% run to run variance on all versions
>>
>>
>> My machine has held up for some time with Experiment 3 so I'm
>> fairly confident at the very least mutual exclusion is holding
>> up - I haven't seen any lockups / hung task either so hopefully
>> other bits are fine too :-)
>
> This looks much better from a performance perspective. IIUC, the idle task in
> this case is being used as a temporary owner marker, but find_proxy_task() would
> treat it as a real mutex owner and could set idle->blocked_donor, right?
>
> Should we handle the STEAL state explicitly in find_proxy_task() to avoid
> creating a donor relationship with the idle task?
That is a good point.
Originally, I had intended for it to spin in __schedule() on just one
CPU since this returns idle task from find_proxy_task() and I though
it'll be similar to porxy_resched_idle().
Now I realise, since we don't go though proxy_resched_idle(), the
NEED_RESCHED flag won't be set for the idle task and the CPU will
actually idle with possible other runnable tasks on there.
That needs fixing yes!
--
Thanks and Regards,
Prateek