Re: [PATCH sched_ext/for-7.3-fixes] sched_ext: Don't leave balance callbacks queued across a pick retry

[Tejun Heo <tj@kernel.org>]

Hello, Andrea.

On Sat, Oct 10, 2026 at 11:52:08AM +0200, Andrea Righi wrote:
> @@ -3311,18 +3311,6 @@ static enum scx_dsp_verdict dispatch_pick(struct rq *rq, struct rq_flags *rf,
>  	rq_unpin_lock(rq, rf);
>  	verdict = dispatch_one(rq, prev);
>  	rq_repin_lock(rq, rf);
> -	maybe_queue_balance_callback(rq);

The comment above dispatch_pick() still says it queues the follow-up work.
Can you update it?

> @@ -3349,16 +3337,8 @@ static enum scx_dsp_verdict dispatch_core_pick(struct rq *rq, struct rq_flags *r
> ...
>-	if (cpu_of(rq) == smp_processor_id()) {
> -		maybe_queue_balance_callback(rq);
> ...
>-	} else if (unlikely(rq->scx.flags & SCX_RQ_BAL_CB_PENDING)) {
> +	if (cpu_of(rq) != smp_processor_id() &&
> +	    unlikely(rq->scx.flags & SCX_RQ_BAL_CB_PENDING)) {

This isn't new, but under core scheduling, a local pick that returns a task
isn't final either. If the task has a cookie or the core needs a sync,
pick_next_task() goes on to pick the siblings. A sibling's dispatch can then
drop the core lock with the local rq's callbacks queued, and any CPU pinning
the local rq in that window hits the same warning. A sibling's RETRY also
restarts the selection with the callbacks still queued.

This can be a separate patch. Maybe something like:

1. Always punt the deferred work to irq_work for core picks, local and
   remote. The remote branch here runs run_deferred() in place, which can
   drop the lock and force a restart by itself.

2. The kick_sync wait can't go to irq_work. However, every lock drop during
   a core selection comes from an scx dispatch and already forces a restart
   through lock_drop_seq. dispatch_core_pick() can unlink this CPU's
   kick_sync_bal_cb before dispatch_one() and set kick_sync_pending again.
   If the lock wasn't dropped, the callback goes back right away. If it was,
   the restarted local pick queues it again. This needs a small helper next
   to queue_balance_callback() to unlink an entry.

> +static void pick_queue_balance_callbacks(struct rq *rq, bool final)
> +{
> +	lockdep_assert_rq_held(rq);
> +
> +	if (cpu_of(rq) != smp_processor_id())
> +		return;
> +
> +	if (!final) {
> +		if (rq->scx.flags & SCX_RQ_BAL_CB_PENDING) {
> +			rq->scx.flags &= ~SCX_RQ_BAL_CB_PENDING;
> +			schedule_deferred(rq);
> +		}
> +		return;
> +	}
> +
> +	maybe_queue_balance_callback(rq);
> ...
>+static struct task_struct *
> +do_pick_task_scx(struct rq *rq, struct rq_flags *rf, bool force_scx)
> +{
> +	struct task_struct *p = __do_pick_task_scx(rq, rf, force_scx);
> +
> +	/* after a NULL pick from the dl_server the lower classes still pick */
> +	pick_queue_balance_callbacks(rq, p != RETRY_TASK && (p || !force_scx));
> +
> +	return p;
> +}

Can we fold pick_queue_balance_callbacks() into do_pick_task_scx() with a
single exit, instead of splitting out __do_pick_task_scx()? The tail can
then be an if/else. Something like:

	if (verdict == SCX_DSP_RETRY) {
		p = RETRY_TASK;
		goto out;
	}
	...
out:
	if (cpu_of(rq) != smp_processor_id())
		return p;

	if (p != RETRY_TASK && (p || !force_scx)) {
		maybe_queue_balance_callback(rq);
		...
	} else if (rq->scx.flags & SCX_RQ_BAL_CB_PENDING) {
		rq->scx.flags &= ~SCX_RQ_BAL_CB_PENDING;
		schedule_deferred(rq);
	}
	return p;

Thanks.

-- 
tejun