Re: [PATCH v4 sched_ext/for-7.4-fixes] sched_ext: Fix stale pick when the donor changes during dispatch

[Andrea Righi <arighi@nvidia.com>]

Hi Tejun,

On Sat, Oct 31, 2026 at 06:28:03AM -1000, Tejun Heo wrote:
> Dispatch takes rq->donor as @prev and can drop the rq lock. With proxy
> execution, @prev may not be the running task, and another CPU can reset the
> donor to rq->curr meanwhile, through proxy_needs_return() on a wakeup or
> sched_proxy_block_task() on a class or scheduler change. The wakeup case:
> 
> 1. dispatch_one() drops the rq lock, e.g. to move a task from another CPU.
> 2. Another CPU wakes the donor, which proxy execution migrated to this CPU.
>    proxy_needs_return() dequeues it and resets the donor to rq->curr, and
>    the wakeup enqueues it, possibly on another CPU.
> 3. dispatch_one() sees @prev queued and returns SCX_DSP_PREV.
> 4. do_pick_task_scx() returns @prev, and this CPU runs a task that is queued
>    on another rq.
> 
> Under core scheduling, the core pick runs dispatch for the sibling rqs with
> each sibling's donor as @prev, and a sibling can switch @prev out while
> dispatch has the lock dropped.
> 
> Whatever dispatch concluded, and whatever it does with @prev after the drop,
> is about a task the rq no longer owns, whether it keeps @prev running or
> hands it to ops.dispatch().
> 
> rq_modified_above() catches the change only when rq->curr is in a class
> above ext, and the dl_server pick skips that check. dispatch_core_pick()
> retries after a lock drop, but by then dispatch has used @prev.
> 
> Restart the pick when @prev is no longer the donor. dispatch_one() tests
> after each step that can drop the lock, and scx_dispatch_sched() decides per
> ops.dispatch() call whether to hand @prev over, which also covers a class or
> scheduler change across a drop.
> 
> With proxy execution alone, the donor only ever changes to rq->curr during a
> pick, so the retried pick can't go stale again. Under core scheduling, the
> restart goes through the core pick, which already retries after any lock
> drop.
> 
> 7a3a6bfbd62a ("sched: Rework prev_balance() to avoid stale prev references")
> closed the same window in prev_balance() by reading rq->donor instead of
> taking @prev. That doesn't work here, as dispatch never evaluated the new
> donor, which may not even be a sched_ext task.
> 
> Verified with synthetic reproducers for both cases.
> 
> v2: Also cover rq lock drops inside dispatch (sashiko).
> 
> v3: Don't keep a stale @prev after ops.dispatch() (sashiko).
> 
> v4: Also cover a sibling switching @prev out under core scheduling. Retitle.
> 
> Fixes: 7b0888b7cc19 ("sched_ext: Implement core-sched support")
> Fixes: c6fe97c34a1a ("sched: Allow enabling proxy exec with sched_ext")
> Cc: stable@vger.kernel.org # v6.12+
> Signed-off-by: Tejun Heo <tj@kernel.org>

With the separate rq->dl_server issue to be addressed in a follow-up:

Reviewed-by: Andrea Righi <arighi@nvidia.com>

One thing about stable backports: ops.cpu_acquire()/cpu_release() are
still supported interfaces in older kernels such as 6.12, are we
considering them deprecated in general?

Thanks,
-Andrea

> ---
>  kernel/sched/ext/ext.c     | 10 ++++++++++
>  kernel/sched/ext/inlines.h | 37 +++++++++++++++++++++++++++++++++----
>  2 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index cd923f9f1f2a..e533c1a5c56d 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -3407,6 +3407,12 @@ static enum scx_dsp_verdict dispatch_one(struct rq *rq, struct task_struct *prev
>  		rq->scx.cpu_released = false;
>  	}
>  
> +	/* the ecaps sync may have dropped the rq lock */
> +	if (scx_dsp_prev_stale(rq, prev)) {
> +		verdict = SCX_DSP_RETRY;
> +		goto has_tasks;
> +	}
> +
>  	if (prev->sched_class == &ext_sched_class) {
>  		update_curr_scx(rq);
>  
> @@ -3449,6 +3455,10 @@ static enum scx_dsp_verdict dispatch_one(struct rq *rq, struct task_struct *prev
>  	}
>  
>  	verdict = scx_dispatch_sched(root_sch, rq, prev, false);
> +
> +	/* scx_dispatch_sched() may have dropped the rq lock */
> +	if (scx_dsp_prev_stale(rq, prev))
> +		verdict = SCX_DSP_RETRY;
>  	if (verdict != SCX_DSP_NONE)
>  		goto has_tasks;
>  
> diff --git a/kernel/sched/ext/inlines.h b/kernel/sched/ext/inlines.h
> index 1e39d12678c4..5dce394e0813 100644
> --- a/kernel/sched/ext/inlines.h
> +++ b/kernel/sched/ext/inlines.h
> @@ -18,9 +18,32 @@ enum scx_dsp_verdict {
>  	SCX_DSP_NONE,		/* nothing to run */
>  	SCX_DSP_LOCAL,		/* local DSQ has tasks */
>  	SCX_DSP_PREV,		/* keep running @prev */
> -	SCX_DSP_RETRY,		/* pick helpers only: restart the pick */
> +	SCX_DSP_RETRY,		/* restart the pick */
>  };
>  
> +/**
> + * scx_dsp_prev_stale - Test whether dispatch's @prev went stale
> + * @rq: rq being dispatched for
> + * @prev: @rq's donor when dispatch started
> + *
> + * @prev can stop being @rq's donor while dispatch has the rq lock dropped. With
> + * proxy execution, @prev may not be the running task, and another CPU can reset
> + * the donor to rq->curr. With core scheduling, the core pick runs dispatch for
> + * the sibling rqs too, and a sibling can switch @prev out. Dispatch restarts
> + * the pick when that happened and doesn't hand the old donor to the BPF
> + * scheduler.
> + *
> + * With proxy execution alone, the donor only ever changes to rq->curr during a
> + * pick. Only __schedule() on this CPU sets a different donor, after the pick,
> + * so the restarted pick takes rq->curr as @prev and can't go stale again. Under
> + * core scheduling, the restart goes through the core pick, which already
> + * retries whenever dispatch dropped the lock.
> + */
> +static inline bool scx_dsp_prev_stale(struct rq *rq, struct task_struct *prev)
> +{
> +	return (sched_proxy_exec() || sched_core_enabled(rq)) && unlikely(prev != rq->donor);
> +}
> +
>  /*
>   * One user of this function is scx_bpf_sub_dispatch() which can be called
>   * recursively as sub-sched dispatches nest. Always inline to reduce stack usage
> @@ -33,8 +56,7 @@ scx_dispatch_sched(struct scx_sched *sch, struct rq *rq,
>  	struct scx_dsp_ctx *dspc = &this_cpu_ptr(sch->pcpu)->dsp_ctx;
>  	int nr_loops = SCX_DSP_MAX_LOOPS;
>  	s32 cpu = cpu_of(rq);
> -	bool prev_on_sch = (prev->sched_class == &ext_sched_class) &&
> -		scx_task_on_sched(sch, prev);
> +	bool prev_on_sch;
>  
>  	if (scx_consume_global_dsq(sch, rq))
>  		return SCX_DSP_LOCAL;
> @@ -106,6 +128,11 @@ scx_dispatch_sched(struct scx_sched *sch, struct rq *rq,
>  			rq->scx.sub_dispatch_prev = prev;
>  #endif
>  
> +		/* the dispatch so far may have dropped the rq lock */
> +		prev_on_sch = !scx_dsp_prev_stale(rq, prev) &&
> +			      prev->sched_class == &ext_sched_class &&
> +			      scx_task_on_sched(sch, prev);
> +
>  		SCX_CALL_OP(sch, dispatch, rq, scx_cpu_arg(cpu),
>  			    prev_on_sch ? prev : NULL);
>  
> @@ -116,7 +143,9 @@ scx_dispatch_sched(struct scx_sched *sch, struct rq *rq,
>  
>  		scx_flush_dispatch_buf(sch, rq);
>  
> -		if ((prev->scx.flags & SCX_TASK_QUEUED) && prev->scx.slice)
> +		/* ops.dispatch() and the flush may have dropped the rq lock */
> +		if (!scx_dsp_prev_stale(rq, prev) &&
> +		    (prev->scx.flags & SCX_TASK_QUEUED) && prev->scx.slice)
>  			return SCX_DSP_PREV;
>  		if (rq->scx.local_dsq.nr)
>  			return SCX_DSP_LOCAL;
> -- 
> 2.56.0
>