Re: [PATCH v30 7/7] sched: Add deactivated (sleeping) owner handling to find_proxy_task()

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

Hello John,

On 7/8/2026 10:04 AM, John Stultz wrote:
> On Thu, Jul 2, 2026 at 8:07 PM K Prateek Nayak <kprateek.nayak@amd.com> wrote:
>> On 7/2/2026 3:16 AM, John Stultz wrote:
>>> @@ -6852,6 +7073,28 @@ static void proxy_migrate_task(struct rq *rq, struct rq_flags *rf,
>>>       proxy_reacquire_rq_lock(rq, rf);
>>>  }
>>>
>>> +static void proxy_enqueue_on_owner(struct rq *rq, struct task_struct *owner,
>>> +                                struct task_struct *p)
>>> +{
>>> +     lockdep_assert_rq_held(rq);
>>> +     lockdep_assert_held(&owner->blocked_lock);
>>> +     /*
>>> +      * ttwu_activate() will pick them up and place them on whatever rq
>>> +      * @owner will run next.
>>> +      */
>>> +     WARN_ON(p == owner);
>>> +     WARN_ON(!p->on_rq);
>>> +     WARN_ON(p->sleeping_owner);
>>> +     get_task_struct(owner);
>>> +     WRITE_ONCE(p->sleeping_owner, owner);
>>> +     /*
>>> +      * ttwu_do_activate must not have a chance to activate p
>>> +      * elsewhere before it's fully extricated from its old rq.
>>> +      */
>>> +     list_add(&p->blocked_node, &owner->blocked_head);
>>
>> I'll refer to this list_add() as (1) below ...
>>
>>> +     block_task(rq, p, READ_ONCE(p->__state));
>>> +}
>>> +
>>>  /*
>>>   * Find runnable lock owner to proxy for mutex blocked donor
>>>   *
>>> @@ -6938,11 +7181,31 @@ find_proxy_task(struct rq *rq, struct task_struct *donor, struct rq_flags *rf)
>>>               }
>>>
>>>               if (!READ_ONCE(owner->on_rq) || owner->se.sched_delayed) {
>>
>> I'm having a sneaky feeling that, with enough bad luck, we
>> can go from seeing !owner->on_rq and here to actually finish
>> enqueuing the owner by the time we get to (1) ...
>>
>>> -                     /* XXX Don't handle blocked owners/delayed dequeue yet */
>>> +                     /*
>>> +                      * rq->curr must not be added to the blocked_head list or else
>>> +                      * ttwu_do_activate could enqueue it elsewhere before it switches
>>> +                      * out here. The approach to avoid this is the same as in the
>>> +                      * migrate_task case.
>>> +                      */
>>>                       if (curr_in_chain)
>>>                               return proxy_resched_idle(rq);
>>> -                     __clear_task_blocked_on(p, NULL);
>>> -                     goto deactivate;
>>> +                     /*
>>> +                      * If !@owner->on_rq, holding @rq->lock will not pin the task,
>>> +                      * so we cannot drop @mutex->wait_lock until we're sure its a blocked
>>> +                      * task on this rq.
>>> +                      *
>>> +                      * We use @owner->blocked_lock to serialize against ttwu_activate().
>>> +                      * Either we see its new owner->on_rq or it will see our list_add().
>>> +                      */
>>> +                     WARN_ON(owner == p);
>>> +                     raw_spin_unlock(&p->blocked_lock);
>>> +                     raw_spin_lock(&owner->blocked_lock);
>>
>> ... by the time we get here, activate_blocked_waiters() could have
>> already bailed out for:
>>
>>   !list_empty(&owner->blocked_activation_node)
>>
>> and then we go and add the task to list. I feel, like most conditionally
>> adding to list patterns, we should add the task to list, check the
>> condition once again under owner->blocked_lock, and then proceed.
>>
>> That way we can sure the activate will definitely see the queued task.
>> Sorry in advance in case I've been blind and missed something
>> obvious :-)
> 
> Hrm.  That's a good point!
> 
> Let me take a stab at fixing that.  I'm thinking it might be easier to
> check on_rq again after we take the blocked lock before calling
> proxy_enqueue_on_owner(), because otherwise unwinding that is a little
> complicated (pulling it off the list isn't, but the task will have
> been blocked, which isn't what we want if we are going to pick again
> to figure out if we can proxy or have to migrate the waiter).  Am I
> still missing a race with that approach (instead of adding to the list
> before the check)?

I think it is safe since the unlock on owner->blocked_on will act as a
release barrier ordering owner->on_rq = TASK_ON_RQ_QUEUED  when the
owner wakes up and the grabbing the owner->blocked_lock in
find_proxy_task() adds acquire semantic before the onwer->on_rq load.

Only downside is you'll need to check list_empty() within the
blocked_lock lock critical section.

> 
>> On a separate note, is it worth tracking if the task is a mutex / lock
>> owner and only go through the activate_blocked_waiters() path only when
>> a __mutex_owner() somewhere can return the task being activated? Perhaps
>> a per-task counter that increments at every mutex_lock() and decrements
>> at a mutex_unlock()?
> 
> This as an optimization to avoid taking the owner->blocked_lock and
> the list checking?

Ack! I had some more interesting (read stupid) ideas on top with that
but I'll try to see if it survives a boot before bombarding  you with
them :-)

> Let me think on that a bit.
> For normal mutexes it seems easy enough but I want to make sure it
> doesn't get complicated with ww_mutexes or the later proxy-rwsem
> support from the full series.

So, with rwsem, I recently realized there is {down,up}_read_non_owner()
which is interesting, but I don't think we set a p->blocked_on for those
cases and since the "owner" is always NULL, I doubt the limit usage of
it in the kernel even cares for priority inheritance.

Apart from that case, I don't think there is any other extra creative
locking patterns possible that don't have symmetry for lock and unlock.

> 
> thanks
> -john

-- 
Thanks and Regards,
Prateek