Re: [PATCH 2/2] sched_ext: Block a retained donor before PI deboost routes it back to sched_ext

[Andrea Righi <arighi@nvidia.com>]

Hi Peter,

On Thu, Nov 12, 2026 at 05:01:36PM +0100, Peter Zijlstra wrote:
> On Thu, Nov 12, 2026 at 04:21:18PM +0100, Peter Zijlstra wrote:
> > On Thu, Nov 12, 2026 at 10:49:35PM +0800, Tao Cui wrote:
> > > From: Tao Cui <cuitao@kylinos.cn>
> > > 
> > > rt_mutex_setprio() is the only entry path into sched_ext that does not
> > > block a retained donor: an ext task PI-boosted by an rt waiter can
> > > block on a second kernel mutex while boosted, and on deboost is routed
> > > back to sched_ext carrying the queued+blocked state that
> > > scx_prepare_setscheduler() promises never happens; every PI cycle
> > > hands the BPF scheduler a queued+blocked donor it never admitted.
> > > 
> > > Call scx_prepare_task_sched_change() before the sched_change guard when
> > > the class change routes the task back to sched_ext, which performs the
> > > missing sched_proxy_block_task() (plus an update_rq_clock() that is
> > > harmless here: the clock was already advanced, so the second advance
> > > only applies a small delta). The boost direction (ext -> rt) needs no
> > > handling.
> > 
> > I can't understand this. And I take a very dim view of sprinking magic
> > hooks like this. So no.
> 
> Ooh, is this the thing from here:
> 
>   https://patch.msgid.link/20260924075925.GE4121620%40noisy.programming.kicks-ass.net
> 
> where you need to block a task because ext doesn't want to support
> proxy-exec?
> 
> Why can't anybody search email these days..
> 
> Something like so? Although I note the scx_block_proxy_donor() thing has
> gotten more complicated, so perhaps this needs a little more?
> 
> Andrea?

Ah yes, our emails crossed. I was proposing to reconsider passing
next_class into sched_change_begin(), but using the existing
switching_to() callback looks like a better direction, without needing
the extra argument.

A few details below.

> 
> ---
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 1a088d4576f5..1c1e669473a6 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -11252,8 +11252,13 @@ void sched_change_end(struct sched_change_ctx *ctx)
>  	 */
>  	WARN_ON_ONCE(p->sched_class != ctx->class && !(ctx->flags & ENQUEUE_CLASS));
>  
> -	if ((ctx->flags & ENQUEUE_CLASS) && p->sched_class->switching_to)
> -		p->sched_class->switching_to(rq, p);
> +	if ((ctx->flags & ENQUEUE_CLASS) && p->sched_class->switching_to) {
> +		bool block = p->sched_class->switching_to(rq, p);
> +		if (block && p->is_blocked && ctx->queued) {
> +			block_task(rq, p, READ_ONCE(p->__state));
> +			ctx->queued = false;
> +		}

So at this point sched_change_begin() has already dequeued p from the
old class, but p hasn't been enqueued in the new class yet, block_task()
would dequeue it again, through the new class, and repeat the generic
dequeue accounting. I think we should complete the block without another
dequeue here.

Also, if p is rq->donor, clearing ctx->queued isn't enough: we need to
reset the donor and clear ctx->running, otherwise sched_change_end()
would call set_next_task(rq, p) for the task we just blocked.

> +	}
>  
>  	if (ctx->queued)
>  		enqueue_task(rq, p, ctx->flags);
> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index f74ffb3b1aca..4440e32236b3 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -46,21 +46,6 @@ bool __scx_allow_proxy_exec(const struct task_struct *p)
>  	return !sch || (sch->ops.flags & SCX_OPS_ENQ_BLOCKED);
>  }
>  
> -/*
> - * End retained proxy execution before changing @p's BPF scheduler ownership.
> - * Called with @p's pi and rq locks held immediately before
> - * sched_change_begin(). The caller must pass DEQUEUE_NOCLOCK so the rq clock
> - * is updated only once.
> - */
> -void scx_prepare_task_sched_change(struct task_struct *p)
> -{
> -	lockdep_assert_held(&p->pi_lock);
> -	lockdep_assert_rq_held(task_rq(p));
> -
> -	update_rq_clock(task_rq(p));
> -	scx_block_proxy_donor(task_rq(p), p);
> -}
> -
>  /*
>   * NOTE: sched_ext is in the process of growing multiple scheduler support and
>   * scx_root usage is in a transitional state. Naked dereferences are safe if the
> @@ -5004,12 +4989,12 @@ static void prio_changed_scx(struct rq *rq, struct task_struct *p, u64 oldprio)
>  {
>  }
>  
> -static void switching_to_scx(struct rq *rq, struct task_struct *p)
> +static bool switching_to_scx(struct rq *rq, struct task_struct *p)
>  {
>  	struct scx_sched *sch = scx_task_sched(p);
>  
>  	if (task_dead_and_done(p))
> -		return;
> +		return false;
>  
>  	scx_enable_task(sch, p);
>  
> @@ -5020,6 +5005,8 @@ static void switching_to_scx(struct rq *rq, struct task_struct *p)
>  	 */
>  	if (!scx_is_cid_type() && SCX_HAS_OP(sch, set_cpumask))
>  		scx_call_op_set_cpumask(sch, rq, p, (struct cpumask *)p->cpus_ptr);
> +
> +	return !scx_allow_proxy_exec(p);
>  }
>  
>  static void switched_from_scx(struct rq *rq, struct task_struct *p)
> @@ -8947,10 +8934,8 @@ static void scx_root_enable_workfn(struct kthread_work *work)
>  
>  		if (old_class != new_class)
>  			queue_flags |= DEQUEUE_CLASS;
> -		if (new_class == &ext_sched_class) {
> -			scx_prepare_task_sched_change(p);
> +		if (new_class == &ext_sched_class)
>  			queue_flags |= DEQUEUE_NOCLOCK;
> -		}

scx_prepare_task_sched_change() also calls update_rq_clock(). With
that call removed, keeping DEQUEUE_NOCLOCK makes sched_change_begin()
skip the clock update even though we haven't done it here.

Maybe we should drop this flag too and let sched_change_begin() update
the clock.

>  
>  		scoped_guard (sched_change, p, queue_flags) {
>  			scx_set_task_slice(p, READ_ONCE(sch->slice_dfl));
> diff --git a/kernel/sched/ext/internal.h b/kernel/sched/ext/internal.h
> index ff0a3d508533..961694e4c27c 100644
> --- a/kernel/sched/ext/internal.h
> +++ b/kernel/sched/ext/internal.h
> @@ -2300,7 +2300,6 @@ struct task_struct *scx_task_iter_next_locked(struct scx_task_iter *iter);
>  bool scx_set_task_slice(struct task_struct *p, u64 slice);
>  void scx_task_slice_ended(struct rq *rq, struct task_struct *p);
>  void scx_task_unlink_from_dsq(struct task_struct *p, struct scx_dispatch_q *dsq);
> -void scx_prepare_task_sched_change(struct task_struct *p);
>  void scx_dispatch_dequeue(struct rq *rq, struct task_struct *p);
>  void scx_do_enqueue_task(struct rq *rq, struct task_struct *p, u64 enq_flags,
>  			 int sticky_cpu);
> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index b5d92d89629b..e73f3474d685 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c
> @@ -1647,7 +1647,6 @@ static void scx_rehome_task(struct scx_sched *to, struct task_struct *p)
>  	lockdep_assert_held(&p->pi_lock);
>  	lockdep_assert_rq_held(task_rq(p));
>  
> -	scx_prepare_task_sched_change(p);
>  	scoped_guard (sched_change, p,
>  		      DEQUEUE_SAVE | DEQUEUE_MOVE | DEQUEUE_NOCLOCK) {

With sub-schedulers these paths can move a task between BPF schedulers
without changing p->sched_class, so switching_to() won't run. We still
need to end the old proxy session before changing ownership, even if the
new BPF scheduler also supports proxy-exec.

Can we keep scx_prepare_task_sched_change() for these ownership changes
and use switching_to() for the actual class transitions?

>  		scx_disable_and_exit_task(scx_task_sched(p), p);
> @@ -1680,7 +1679,6 @@ static void scx_punt_task(struct scx_sched *to, struct task_struct *p)
>  	lockdep_assert_rq_held(task_rq(p));
>  	WARN_ON_ONCE(!READ_ONCE(to->bypass_depth));
>  
> -	scx_prepare_task_sched_change(p);
>  	scoped_guard (sched_change, p,
>  		      DEQUEUE_SAVE | DEQUEUE_MOVE | DEQUEUE_NOCLOCK) {

And ditto here.

>  		scx_disable_and_exit_task(scx_task_sched(p), p);
> @@ -2459,7 +2457,6 @@ void scx_sub_enable_workfn(struct kthread_work *work)
>  		if (!(p->scx.flags & SCX_TASK_SUB_INIT))
>  			continue;
>  
> -		scx_prepare_task_sched_change(p);
>  		scoped_guard (sched_change, p,
>  			      DEQUEUE_SAVE | DEQUEUE_MOVE | DEQUEUE_NOCLOCK) {
>  			/*
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index e0c10a5dbda2..cda367bd92af 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -547,7 +547,7 @@ static void task_tick_idle(struct rq *rq, struct task_struct *curr, int queued)
>  	update_curr_idle(rq);
>  }
>  
> -static void switching_to_idle(struct rq *rq, struct task_struct *p)
> +static bool switching_to_idle(struct rq *rq, struct task_struct *p)
>  {
>  	BUG();
>  }
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 1d76950dbb0a..979c520034b1 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -2782,7 +2782,7 @@ struct sched_class {
>  	 */
>  	void (*switching_from)(struct rq *this_rq, struct task_struct *task);
>  	void (*switched_from) (struct rq *this_rq, struct task_struct *task);
> -	void (*switching_to)  (struct rq *this_rq, struct task_struct *task);
> +	bool (*switching_to)  (struct rq *this_rq, struct task_struct *task);
>  	void (*switched_to)   (struct rq *this_rq, struct task_struct *task);
>  	u64  (*get_prio)     (struct rq *this_rq, struct task_struct *task);
>  	void (*prio_changed) (struct rq *this_rq, struct task_struct *task,
> diff --git a/kernel/sched/stop_task.c b/kernel/sched/stop_task.c
> index 1e0109ec36b3..b7541bb7b593 100644
> --- a/kernel/sched/stop_task.c
> +++ b/kernel/sched/stop_task.c
> @@ -78,7 +78,7 @@ static void task_tick_stop(struct rq *rq, struct task_struct *curr, int queued)
>  {
>  }
>  
> -static void switching_to_stop(struct rq *rq, struct task_struct *p)
> +static bool switching_to_stop(struct rq *rq, struct task_struct *p)
>  {
>  	BUG(); /* its impossible to change to this class */
>  }

Thanks,
-Andrea