Re: [PATCH v2 1/2] sched: proxy-exec: Close race causing workqueue work being delayed

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

Hello Peter,

On 5/1/2026 6:51 PM, Peter Zijlstra wrote:
> 
> Sorry for being late, I was unwell for a few days :/

Hope you are feeling better now.

>> -#define PROXY_WAKING ((struct mutex *)(-1L))
>> +#define PROXY_BLOCKED_LATCH (1UL)
>> +#define PROXY_BLOCKED_ON_MASK(x) ((struct mutex *)((unsigned long)(x) & ~PROXY_BLOCKED_LATCH))
>> +#define PROXY_WAKING ((struct mutex *)(-1L)) /* PROXY_WAKING has LATCH bit set */
> 
> Urgh, please no.
> 
> You're making it needlessly complicated. There really are two separate
> states, set by two different chains of logic:
> 
>  - the blocked_on link, set by the blocking primitive (mutex)
> 
>  - the is_blocked state, set by the scheduler when logically blocking
>    the task.
> 
> by munging them together like that, you also inherit that blocked_lock
> into contexts that really don't need it, and you're also sprinkling
> more of that sched_proxy_exec() stuff around.
> 
> If we keep them nicely separated, none of that happens, and
> additionally, we might be able to get rid of the p->se.sched_delayed
> (ab)use in the core code (eventually).

So there are cases where we want to traverse the find_proxy_task()
bits even after the task gets a wakeup to do return migration which
will break if we start clearing p->is_blocked at ttwu_do_wakeup().

More on that below ...

> 
> Does something like the below really not work?
> 
> ---
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 368c7b4d7cb5..0bd5da8360f3 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -846,7 +846,11 @@ struct task_struct {
>  	struct alloc_tag		*alloc_tag;
>  #endif
>  
> -	int				on_cpu;
> +	u8				on_cpu;
> +	u8				on_rq;
> +	u8				is_blocked;
> +	u8				__pad;
> +
>  	struct __call_single_node	wake_entry;
>  	unsigned int			wakee_flips;
>  	unsigned long			wakee_flip_decay_ts;
> @@ -861,7 +865,6 @@ struct task_struct {
>  	 */
>  	int				recent_used_cpu;
>  	int				wake_cpu;
> -	int				on_rq;
>  
>  	int				prio;
>  	int				static_prio;
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b8871449d3c6..f679d65d98a3 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -615,6 +615,12 @@ EXPORT_SYMBOL(__trace_set_current_state);
>   *   [ The astute reader will observe that it is possible for two tasks on one
>   *     CPU to have ->on_cpu = 1 at the same time. ]
>   *
> + * p->is_blocked <- { 0, 1 }:
> + *
> + *   is set by try_to_block_task() and cleared by ttwu_do_wakeup() and tracks
> + *   if the task is blocked. Tradidionally this would mirror p->on_rq, however
> + *   due things like DELAY_DEQUEUE and PROXY_EXEC, this can diverge.
> + *
>   * task_cpu(p): is changed by set_task_cpu(), the rules are:
>   *
>   *  - Don't call set_task_cpu() on a blocked task:
> @@ -3685,6 +3691,7 @@ ttwu_stat(struct task_struct *p, int cpu, int wake_flags)
>   */
>  static inline void ttwu_do_wakeup(struct task_struct *p)
>  {
> +	p->is_blocked = 0;

I don't think it is this simple at the moment because the proxy bits in
__schedule() still have to handle PROXY_WAKING and once we clear it here
task will no longer go through proxy_needs_return() path.

Clearing of ->is_blocked has to be done at the same point where
->blocked_on is cleared although they are set separately.

>  	WRITE_ONCE(p->__state, TASK_RUNNING);
>  	trace_sched_wakeup(p);
>  }
> @@ -4173,6 +4180,7 @@ int try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags)
>  		 *    it disabling IRQs (this allows not taking ->pi_lock).
>  		 */
>  		WARN_ON_ONCE(p->se.sched_delayed);
> +		WARN_ON_ONCE(p->is_blocked);
>  		if (!ttwu_state_match(p, state, &success))
>  			goto out;
>  
> @@ -4463,6 +4471,7 @@ static void __sched_fork(u64 clone_flags, struct task_struct *p)
>  
>  	/* A delayed task cannot be in clone(). */
>  	WARN_ON_ONCE(p->se.sched_delayed);
> +	WARN_ON_ONCE(p->is_blocked);
>  
>  #ifdef CONFIG_FAIR_GROUP_SCHED
>  	p->se.cfs_rq			= NULL;
> @@ -6593,6 +6602,8 @@ static bool try_to_block_task(struct rq *rq, struct task_struct *p,
>  		return false;
>  	}

If we change the set_task_blocked_on_waking() above for pending
signal to clear_task_blocked_on(), this should be fine. Since
prev is on_cpu, it doesn't need any return migration and going via
PROXY_WAKING path isn't too helpful IMO.

>  
> +	p->is_blocked = 1;
> +
>  	/*
>  	 * We check should_block after signal_pending because we
>  	 * will want to wake the task in that case. But if
> @@ -7108,7 +7119,7 @@ static void __sched notrace __schedule(int sched_mode)
>  		struct task_struct *prev_donor = rq->donor;
>  
>  		rq_set_donor(rq, next);
> -		if (unlikely(next->blocked_on)) {
> +		if (unlikely(next->is_blocked && next->blocked_on)) {

There is a race with ttwu_runnable() that happens like:

  mutex_lock_common(mutex)
    set_task_blocked_on(p, mutex)
    set_current_state(state)         mutex_unloc(mutex)
    schedule_preempt_disabled()        set_task_blocked_on_waking(p)
      ...                              try_to_wake_up(p) /* State matches; p->on_rq */
                                         ttwu_runnable(p)
                                           ttwu_do_wakeup(p);
      if (!preempt && prev_state) {
         /*
          * Never happens since
          * ->state == TASK_RUNNING.
          * -> is/_blocked is never set.
          */
      }

      next = /* Gets prev again */
      /* proxy bits are skipped since ->is_blocked is 0 */

    /*
     * Exits out of schedule_preempt_disabled()
     * in mutex_lock_common().
     */
    __set_task_blocked_on(current, lock);
      !!! SPLAT: p->blocked_on /* PROXY_WAKING */ && p->blocked != lock !!!


So that screams since we fail to clear the ->blocked_on state when
ttwu_runnable() wins over schedule().

John didn't like touching the ->blocked_on state for
(!prev_state && prev->blocked_on) so we resorted to using the lower
bits of ->blocked_on.

The p->se.sched_proxy like fix is the closest we'll get to if we go
down the separate state in task_struct path and for most part it
will mirror blocked_on which is why setting the bottom bits like
MUTEX_FLAGS made some sense when we looked at it.

>  			next = find_proxy_task(rq, next, &rf);
>  			if (!next) {
>  				zap_balance_callbacks(rq);

-- 
Thanks and Regards,
Prateek