Re: [RESEND][PATCH v21 2/6] sched/locking: Add blocked_on_state to provide necessary tri-state for proxy return-migration

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

Hello John,

On 9/4/2025 5:51 AM, John Stultz wrote:
>  static inline void __clear_task_blocked_on(struct task_struct *p, struct mutex *m)
>  {
> +	/* The task should only be clearing itself */
> +	WARN_ON_ONCE(p != current);
>  	/* Currently we serialize blocked_on under the task::blocked_lock */
>  	lockdep_assert_held_once(&p->blocked_lock);
> -	/*
> -	 * There may be cases where we re-clear already cleared
> -	 * blocked_on relationships, but make sure we are not
> -	 * clearing the relationship with a different lock.
> -	 */
> -	WARN_ON_ONCE(m && p->blocked_on && p->blocked_on != m);
> +	/* Make sure we are clearing the relationship with the right lock */
> +	WARN_ON_ONCE(m && p->blocked_on != m);
>  	p->blocked_on = NULL;
> +	p->blocked_on_state = BO_RUNNABLE;
>  }
>  

Maybe it is just me, but I got confused a couple of time only to
realize __clear_task_blocked_on() clears the "blocked_on" and sets
"blocked_on_state" to BO_RUNNABLE.

Can we decouple the two and only set "p->blocked_on" in
*_task_blocked_on_* and "p->blocked_on_state" in
*{set,clear,force}_blocked_on* functions so it becomes easier to
follow in the mutex path as:

    __set_task_blocked_on(p, mutex); // blocked on mutex
    __force_blocked_on_blocked(p); // blocked from running on CPU

   ...

    __clear_task_blocked_on(p, mutex); // p is no longer blocked on a mutex
    __set_blocked_on_runnable(p); // p is now runnable

>  static inline void clear_task_blocked_on(struct task_struct *p, struct mutex *m)

Of the three {set,clear}_task_blcoked_on() usage:

    $ grep -r "\(set\|clear\)_task_blocked_on" include/
    kernel/locking/mutex.c: __set_task_blocked_on(current, lock);
    kernel/locking/mutex.c: __clear_task_blocked_on(current, lock);
    kernel/locking/mutex.c: clear_task_blocked_on(current, lock);

two can be converted directly and perhaps clear_task_blocked_on() can be
renamed as clear_task_blocked_on_set_runnable()?

This is just me rambling on so feel free to ignore. I probably have to
train my mind enough to see __clear_task_blocked_on() not only clears
"blocked_on" but also sets task to runnable :)

[..snip..]

> @@ -6749,6 +6776,15 @@ find_proxy_task(struct rq *rq, struct task_struct *donor, struct rq_flags *rf)
>  
>  	WARN_ON_ONCE(owner && !owner->on_rq);
>  	return owner;
> +
> +	/*
> +	 * NOTE: This logic is down here, because we need to call
> +	 * the functions with the mutex wait_lock and task
> +	 * blocked_lock released, so we have to get out of the
> +	 * guard() scope.
> +	 */

I didn't know that was possible! Neat. Since cleanup.h has a note
reading:

    ... the expectation is that usage of "goto" and cleanup helpers is
    never mixed in the same function.

are there any concerns w.r.t. compiler versions etc. or am I just being
paranoid?

> +deactivate_donor:
> +	return proxy_deactivate(rq, donor);
>  }
>  #else /* SCHED_PROXY_EXEC */
>  static struct task_struct *
-- 
Thanks and Regards,
Prateek