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

[Peter Zijlstra]

Sorry for being late, I was unwell for a few days :/

On Thu, Apr 30, 2026 at 09:50:46PM +0000, John Stultz wrote:

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 368c7b4d7cb51..8b9e971d98f67 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2183,18 +2183,56 @@ extern int __cond_resched_rwlock_write(rwlock_t *lock) __must_hold(lock);
>  #ifndef CONFIG_PREEMPT_RT
>  
>  /*
> - * With proxy exec, if a task has been proxy-migrated, it may be a donor
> - * on a cpu that it can't actually run on. Thus we need a special state
> - * to denote that the task is being woken, but that it needs to be
> - * evaluated for return-migration before it is run. So if the task is
> - * blocked_on PROXY_WAKING, return migrate it before running it.
> + * The proxy exec blocked_on pointer value uses the low bit as a latch
> + * value which clarifies if the blocked_on value is used for proxying or
> + * not.
> + *
> + * The state machine looks something like
> + *   NULL -> ptr:unlatched -> ptr:latched -> PROXY_WAKING -> NULL
> + *
> + * With some additional transitions:
> + *   ptr:unlatched -> NULL (done on current, or via set_task_blocked_on_waking())
> + *   ptr:latched -> NULL (done only on current)
> + *
> + * 1) NULL and ptr:unlatched are effectively equivalent, no proxying will occur
> + * 2) ptr:latched is the state when proxying will occur
> + * 3) PROXY_WAKING is used when the task is being woken to  ensure we
> + *    return-migrate proxy-migrated tasks before running them (note it has
> + *    the latch bit set).
>   */
> -#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).

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;
 	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;
 	}
 
+	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)) {
 			next = find_proxy_task(rq, next, &rf);
 			if (!next) {
 				zap_balance_callbacks(rq);