Re: [RFC PATCH 04/16] sched/core: Activate blocked donor when no owner is found

[K Prateek Nayak <kprateek.nayak@amd.com>]

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