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

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

[Andrea Righi]

The dispatch path can request deferred work to be run from a balance
callback and pick_task_scx() queues rq->scx.deferred_bal_cb right after
dispatching. However, a balance callback must be consumed before the rq
lock is released and pick_task_scx() is not necessarily the end of the
pick:

 - it returns RETRY_TASK when a higher priority class task has been
   enqueued while the dispatch had the rq lock released;

 - the dl_server pick can return NULL, in which case the lower classes
   are picked next.

In both cases the callback stays queued while the core keeps picking,
and the rq lock can be released again, either by prev_balance() or by
another dispatch. For example, with an RT task as prev:

  __schedule()
    __pick_next_task()
      pick_task_scx()
        dispatch_pick()
          maybe_queue_balance_callback()    <- deferred_bal_cb queued
        return RETRY_TASK
      prev_balance()
        balance_rt()
          pull_rt_task()
            double_lock_balance()           <- rq lock released

Any CPU acquiring the rq lock in that window finds a pending balance
callback and triggers the following warning in rq_pin_lock():

  WARNING: kernel/sched/sched.h:1919 at rq_lock+0x7f/0xe0, CPU#10: stress-ng-sleep/533
  CPU: 10 UID: 0 PID: 533 Comm: stress-ng-sleep Not tainted 7.2.0-rc6-virtme #61 PREEMPT(full)
  Sched_ext: qmap (enabled+all)
  Call Trace:
   <IRQ>
   try_to_wake_up+0x2c1/0x5c0
   hrtimer_wakeup+0x22/0x30
   __hrtimer_run_queues+0x1d5/0x350
   hrtimer_interrupt+0x106/0x1e0
   __sysvec_apic_timer_interrupt+0x6f/0x210
   sysvec_apic_timer_interrupt+0x71/0x90
   </IRQ>

This can be reproduced running scx_qmap -I together with a SCHED_FIFO
workload that frequently sleeps, e.g.:

  $ vng -v --cpus 16 --memory 4G -- \
      'tools/sched_ext/build/bin/scx_qmap -I >/dev/null & \
       stress-ng --sleep 4 --sched fifo --sched-prio 1 -t 30 -q'

Fix this by queueing the balance callbacks only when the pick is final.
Otherwise, defer the pending work to the irq_work path, which doesn't
depend on the current rq lock section and leave the kick sync wait
pending for the next pick.

Fixes: 4c95380701f5 ("sched/ext: Fold balance_scx() into pick_task_scx()")
Cc: stable@vger.kernel.org # v6.19+
Signed-off-by: Andrea Righi <arighi@nvidia.com>
---
 kernel/sched/ext/ext.c | 74 +++++++++++++++++++++++++++++-------------
 1 file changed, 51 insertions(+), 23 deletions(-)

diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index ac8fe70a9fb6f..10b3b95f70a1b 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -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);
-
-	/*
-	 * Defer to a balance callback which can drop rq lock and enable IRQs.
-	 * Waiting directly in the pick path would deadlock against CPUs sending
-	 * us IPIs (e.g. TLB flushes) while we wait for them.
-	 */
-	if (unlikely(rq->scx.kick_sync_pending)) {
-		rq->scx.kick_sync_pending = false;
-		queue_balance_callback(rq, &rq->scx.kick_sync_bal_cb,
-				       kick_sync_wait_bal_cb);
-	}
 
 	return verdict;
 }
@@ -3349,16 +3337,8 @@ static enum scx_dsp_verdict dispatch_core_pick(struct rq *rq, struct rq_flags *r
 
 	verdict = dispatch_one(rq, prev);
 
-	if (cpu_of(rq) == smp_processor_id()) {
-		maybe_queue_balance_callback(rq);
-
-		/* see dispatch_pick() */
-		if (unlikely(rq->scx.kick_sync_pending)) {
-			rq->scx.kick_sync_pending = false;
-			queue_balance_callback(rq, &rq->scx.kick_sync_bal_cb,
-					       kick_sync_wait_bal_cb);
-		}
-	} 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)) {
 		/*
 		 * Balance callbacks must run in the context that queued them,
 		 * so they can't be queued on another CPU's rq. Run the deferred
@@ -3384,8 +3364,45 @@ static enum scx_dsp_verdict dispatch_core_pick(struct rq *rq, struct rq_flags *r
 }
 #endif	/* CONFIG_SCHED_CORE */
 
+/*
+ * Queue the balance callbacks requested by the dispatch path. Balance callbacks
+ * must stay queued only until the end of the current rq lock section, so they
+ * can be used only when the pick is @final. Otherwise, the core is going to
+ * retry or continue the pick, which may release the rq lock (e.g.,
+ * prev_balance() or another dispatch) and expose the queued callbacks to
+ * rq_pin_lock() on other CPUs. Punt the deferred work to irq_work in that case.
+ */
+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);
+
+	/*
+	 * Defer to a balance callback which can drop rq lock and enable IRQs.
+	 * Waiting directly in the pick path would deadlock against CPUs sending
+	 * us IPIs (e.g., TLB flushes) while we wait for them.
+	 */
+	if (unlikely(rq->scx.kick_sync_pending)) {
+		rq->scx.kick_sync_pending = false;
+		queue_balance_callback(rq, &rq->scx.kick_sync_bal_cb,
+				       kick_sync_wait_bal_cb);
+	}
+}
+
 static struct task_struct *
-do_pick_task_scx(struct rq *rq, struct rq_flags *rf, bool force_scx)
+__do_pick_task_scx(struct rq *rq, struct rq_flags *rf, bool force_scx)
 {
 	struct task_struct *prev = rq->curr;
 	enum scx_dsp_verdict verdict;
@@ -3447,6 +3464,17 @@ do_pick_task_scx(struct rq *rq, struct rq_flags *rf, bool force_scx)
 	return p;
 }
 
+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;
+}
+
 static struct task_struct *pick_task_scx(struct rq *rq, struct rq_flags *rf)
 {
 	return do_pick_task_scx(rq, rf, false);
-- 
2.56.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sched/ext: Balance callback queued on NULL pick risks lockdep warning during newidle_balance
--

commit 82685b13d2a91f21d4676e7837dbf23573a2e3ac
Author: Andrea Righi <arighi@nvidia.com>

    sched_ext: Don't leave balance callbacks queued across a pick retry

This commit modifies the dispatch path in sched_ext to only queue balance
callbacks when the pick is final. If the core needs to retry or continue the
pick, which may release the runqueue lock, it instead punts the deferred work
to irq_work.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 3f79156467799..54fa382ad8d78 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c

[ ... ]

> @@ -3433,6 +3450,17 @@ do_pick_task_scx(struct rq *rq, struct rq_flags *rf, bool force_scx)
>  	return p;
>  }
>  
> +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));
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Does this logic incorrectly identify a NULL pick as final when force_scx is
false, leading to premature queueing of balance callbacks?

When a normal sched_ext pick (where force_scx == false) fails to find a task,
it returns NULL. The boolean logic evaluates to true because p != RETRY_TASK
is true (since p is NULL) and !force_scx is true. This marks the pick as final
and queues balance callbacks to rq->balance_callback.

However, a NULL pick from sched_ext is not final for the CPU, as the core
scheduler loop in __pick_next_task() will fall through to lower-priority
scheduling classes, such as fair_sched_class.

If fair also has no runnable tasks, it triggers newidle_balance(), which
unconditionally drops the runqueue lock via rq_unpin_lock() and
raw_spin_rq_unlock().

If another CPU acquires the lock during this window, rq_pin_lock() will
trigger a SCHED_WARN_ON because a balance callback is present on the runqueue
while the lock was unpinned. This defeats the patch's purpose and triggers the
exact lockdep warning it was intended to fix.

> +
> +	return p;
> +}
> +
>  static struct task_struct *pick_task_scx(struct rq *rq, struct rq_flags *rf)
>  {
>  	return do_pick_task_scx(rq, rf, false);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010095208.276806-1-arighi@nvidia.com?part=1

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

[Andrea Righi]

On Sat, Oct 10, 2026 at 10:03:11AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] sched/ext: Balance callback queued on NULL pick risks lockdep warning during newidle_balance
> --
> 
> commit 82685b13d2a91f21d4676e7837dbf23573a2e3ac
> Author: Andrea Righi <arighi@nvidia.com>
> 
>     sched_ext: Don't leave balance callbacks queued across a pick retry
> 
> This commit modifies the dispatch path in sched_ext to only queue balance
> callbacks when the pick is final. If the core needs to retry or continue the
> pick, which may release the runqueue lock, it instead punts the deferred work
> to irq_work.
> 
> > diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> > index 3f79156467799..54fa382ad8d78 100644
> > --- a/kernel/sched/ext/ext.c
> > +++ b/kernel/sched/ext/ext.c
> 
> [ ... ]
> 
> > @@ -3433,6 +3450,17 @@ do_pick_task_scx(struct rq *rq, struct rq_flags *rf, bool force_scx)
> >  	return p;
> >  }
> >  
> > +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));
>                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> [Severity: High]
> Does this logic incorrectly identify a NULL pick as final when force_scx is
> false, leading to premature queueing of balance callbacks?
> 
> When a normal sched_ext pick (where force_scx == false) fails to find a task,
> it returns NULL. The boolean logic evaluates to true because p != RETRY_TASK
> is true (since p is NULL) and !force_scx is true. This marks the pick as final
> and queues balance callbacks to rq->balance_callback.
> 
> However, a NULL pick from sched_ext is not final for the CPU, as the core
> scheduler loop in __pick_next_task() will fall through to lower-priority
> scheduling classes, such as fair_sched_class.

This looks like a false positive: fair_sched_class is not a lower priority class
than ext_sched_class, it's the other way around.

Neither pick_task_idle() nor set_next_task_idle() release the rq lock, so the
pick is final at that point and the callbacks are consumed before the rq lock is
released.

> If fair also has no runnable tasks, it triggers newidle_balance(), which
> unconditionally drops the runqueue lock via rq_unpin_lock() and
> raw_spin_rq_unlock().
> 
> If another CPU acquires the lock during this window, rq_pin_lock() will
> trigger a SCHED_WARN_ON because a balance callback is present on the runqueue
> while the lock was unpinned. This defeats the patch's purpose and triggers the
> exact lockdep warning it was intended to fix.

The fair newidle balance runs from pick_task_fair(), which is called before
pick_task_scx() in the same pick iteration. If pick_task_scx() returns
RETRY_TASK and the loop restarts, the pick is not considered final and nothing
is left queued, which is exactly the case this patch is addressing.

The only case where a NULL pick is followed by lower classes is the dl_server
one, that is the force_scx == true case and it's already handled as non-final.

-Andrea