In testing with the full Proxy Execution patch stack, I found
I would occasionally trip over the !RB_EMPTY_NODE() WARN_ON in
enqueue_pushable_dl_task(), where the task we're adding to the
pushable list is already enqueued.

This triggers from put_prev_task_dl(), where it seems we go into
put_prev_task_dl()
-> update_curr_dl()
   -> update_curr_dl_se() [hitting the dl_runtime_exceeded() case]
      -> enqueue_task_dl()
         -> enqueue_pushable_dl_task()

Adding the task to the pushable the first time.

Then we back up the call stack to put_prev_task_dl(), which at
the end again calls enqueue_pushable_dl_task(), trying to add it
a second time, tripping the warning.

To avoid this, add a dl_task_pushable() helper which we can use
to replace the RB_EMPTY_NODE checks elsewhere, and then before
enqueueing in put_prev_task_dl(), we can first check
dl_task_pushable() to avoid the double enqueue.

I've only really tripped this while stress testing the full
Proxy Execution patch series, and I haven't had time to do the
same level of stress testing with vanilla, so its possible this
isn't likely to occur with the current upstream, but it seems
like it could happen, so I wanted to send this out for comment.

Fixes: 63ba8422f876e ("sched/deadline: Introduce deadline servers")
Signed-off-by: John Stultz <jstultz@google.com>
---
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
CC: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Segall <bsegall@google.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Valentin Schneider <vschneid@redhat.com>
Cc: Suleiman Souhlal <suleiman@google.com>
Cc: K Prateek Nayak <kprateek.nayak@amd.com>
Cc: kernel-team@android.com
---
 kernel/sched/deadline.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index d08b004293234..7a97147560eb3 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -571,6 +571,11 @@ static inline int has_pushable_dl_tasks(struct rq *rq)
 	return !RB_EMPTY_ROOT(&rq->dl.pushable_dl_tasks_root.rb_root);
 }
 
+static inline bool dl_task_pushable(struct task_struct *p)
+{
+	return !RB_EMPTY_NODE(&p->pushable_dl_tasks);
+}
+
 /*
  * The list of pushable -deadline task is not a plist, like in
  * sched_rt.c, it is an rb-tree with tasks ordered by deadline.
@@ -579,7 +584,7 @@ static void enqueue_pushable_dl_task(struct rq *rq, struct task_struct *p)
 {
 	struct rb_node *leftmost;
 
-	WARN_ON_ONCE(!RB_EMPTY_NODE(&p->pushable_dl_tasks));
+	WARN_ON_ONCE(dl_task_pushable(p));
 
 	leftmost = rb_add_cached(&p->pushable_dl_tasks,
 				 &rq->dl.pushable_dl_tasks_root,
@@ -599,7 +604,7 @@ static void dequeue_pushable_dl_task(struct rq *rq, struct task_struct *p)
 	struct rb_root_cached *root = &dl_rq->pushable_dl_tasks_root;
 	struct rb_node *leftmost;
 
-	if (RB_EMPTY_NODE(&p->pushable_dl_tasks))
+	if (!dl_task_pushable(p))
 		return;
 
 	leftmost = rb_erase_cached(&p->pushable_dl_tasks, root);
@@ -2646,6 +2651,21 @@ static void put_prev_task_dl(struct rq *rq, struct task_struct *p, struct task_s
 	if (task_is_blocked(p))
 		return;
 
+	/*
+	 * its possible the following call chain from
+	 * update_curr_dl() called above has already
+	 * added us to the pushable list:
+	 * update_curr_dl()
+	 * -> update_curr_dl_se()
+	 *    -> enqueue_task_dl()
+	 *       -> enqueue_pushable_dl_task()
+	 *
+	 * So check dl_task_pushable() first to make sure we don't
+	 * get added twice
+	 */
+	if (dl_task_pushable(p))
+		return;
+
 	if (on_dl_rq(&p->dl) && p->nr_cpus_allowed > 1)
 		enqueue_pushable_dl_task(rq, p);
 }
-- 
2.53.0.473.g4a7958ca14-goog

Hello,

On 03/03/26 19:41, John Stultz wrote:
> In testing with the full Proxy Execution patch stack, I found
> I would occasionally trip over the !RB_EMPTY_NODE() WARN_ON in
> enqueue_pushable_dl_task(), where the task we're adding to the
> pushable list is already enqueued.
> 
> This triggers from put_prev_task_dl(), where it seems we go into
> put_prev_task_dl()
> -> update_curr_dl()
>    -> update_curr_dl_se() [hitting the dl_runtime_exceeded() case]
>       -> enqueue_task_dl()
>          -> enqueue_pushable_dl_task()
> 
> Adding the task to the pushable the first time.

Ah, so in case the task is boosted (or we fail to start the
replenishment timer).

> Then we back up the call stack to put_prev_task_dl(), which at
> the end again calls enqueue_pushable_dl_task(), trying to add it
> a second time, tripping the warning.
> 
> To avoid this, add a dl_task_pushable() helper which we can use
> to replace the RB_EMPTY_NODE checks elsewhere, and then before
> enqueueing in put_prev_task_dl(), we can first check
> dl_task_pushable() to avoid the double enqueue.

Can't we just return early (as we do already in dequeue_pushable
_dl_task()) in enqueue_pushable_dl_task() instead of checking before
calling that function?

Thanks,
Juri

On Wed, Mar 04, 2026 at 08:06:53AM +0100, Juri Lelli wrote:
> Hello,
> 
> On 03/03/26 19:41, John Stultz wrote:
> > In testing with the full Proxy Execution patch stack, I found
> > I would occasionally trip over the !RB_EMPTY_NODE() WARN_ON in
> > enqueue_pushable_dl_task(), where the task we're adding to the
> > pushable list is already enqueued.
> > 
> > This triggers from put_prev_task_dl(), where it seems we go into
> > put_prev_task_dl()
> > -> update_curr_dl()
> >    -> update_curr_dl_se() [hitting the dl_runtime_exceeded() case]
> >       -> enqueue_task_dl()
> >          -> enqueue_pushable_dl_task()
> > 
> > Adding the task to the pushable the first time.
> 
> Ah, so in case the task is boosted (or we fail to start the
> replenishment timer).
> 
> > Then we back up the call stack to put_prev_task_dl(), which at
> > the end again calls enqueue_pushable_dl_task(), trying to add it
> > a second time, tripping the warning.
> > 
> > To avoid this, add a dl_task_pushable() helper which we can use
> > to replace the RB_EMPTY_NODE checks elsewhere, and then before
> > enqueueing in put_prev_task_dl(), we can first check
> > dl_task_pushable() to avoid the double enqueue.
> 
> Can't we just return early (as we do already in dequeue_pushable
> _dl_task()) in enqueue_pushable_dl_task() instead of checking before
> calling that function?

So I was mightily confused for a moment by all this.

But it turns out DL keeps current *in* the tree; since ->deadline is a
lot less mutable than ->vruntime this is possible.

But that also means that set_next_task() / put_prev_task() are
'simpler'.

However, in this case I think they are too simple, and its leading to
problems.

So update_curr_dl() needs to dequeue+enqueue because it is pushing
->deadline (because current is in tree). Then because of that, it also
ends up doing enqueue_pushable, but that is actively wrong, you must not
do that on current.

Only once you're doing put_prev_task() must you do that.

Imagine this happens because of an update_curr() that is not from
put_prev_task(), then you end up with current on the pushable list,
which is a big no-no.

So I think I'd like to see dl_rq->curr tracking, similar to what we have
for fair.

If you do that (see below), you'll probably find this case was already
handled 'correctly' but was broken by the PE thing ;-)

Note the !task_current() clause in enqueue_task_dl() guarding
enqueue_pushable_dl_task().

Hmm?

---
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 9869025941a0..a7db81d17082 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -2295,7 +2295,10 @@ static void dequeue_dl_entity(struct sched_dl_entity *dl_se, int flags)
 
 static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 {
-	if (is_dl_boosted(&p->dl)) {
+	struct sched_dl_entity *dl_se = &p->dl;
+	struct dl_rq *dl_rq = &rq->dl;
+
+	if (is_dl_boosted(dl_se)) {
 		/*
 		 * Because of delays in the detection of the overrun of a
 		 * thread's runtime, it might be the case that a thread
@@ -2308,14 +2311,14 @@ static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 		 *
 		 * In this case, the boost overrides the throttle.
 		 */
-		if (p->dl.dl_throttled) {
+		if (dl_se->dl_throttled) {
 			/*
 			 * The replenish timer needs to be canceled. No
 			 * problem if it fires concurrently: boosted threads
 			 * are ignored in dl_task_timer().
 			 */
-			cancel_replenish_timer(&p->dl);
-			p->dl.dl_throttled = 0;
+			cancel_replenish_timer(dl_se);
+			dl_se->dl_throttled = 0;
 		}
 	} else if (!dl_prio(p->normal_prio)) {
 		/*
@@ -2327,7 +2330,7 @@ static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 		 * being boosted again with no means to replenish the runtime and clear
 		 * the throttle.
 		 */
-		p->dl.dl_throttled = 0;
+		dl_se->dl_throttled = 0;
 		if (!(flags & ENQUEUE_REPLENISH))
 			printk_deferred_once("sched: DL de-boosted task PID %d: REPLENISH flag missing\n",
 					     task_pid_nr(p));
@@ -2336,20 +2339,23 @@ static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 	}
 
 	check_schedstat_required();
-	update_stats_wait_start_dl(dl_rq_of_se(&p->dl), &p->dl);
+	update_stats_wait_start_dl(dl_rq, dl_se);
 
 	if (p->on_rq == TASK_ON_RQ_MIGRATING)
 		flags |= ENQUEUE_MIGRATING;
 
-	enqueue_dl_entity(&p->dl, flags);
+	enqueue_dl_entity(dl_se, flags);
 
-	if (dl_server(&p->dl))
+	if (dl_server(dl_se))
 		return;
 
 	if (task_is_blocked(p))
 		return;
 
-	if (!task_current(rq, p) && !p->dl.dl_throttled && p->nr_cpus_allowed > 1)
+	if (dl_rq->curr == dl_se)
+		return;
+
+	if (!p->dl.dl_throttled && p->nr_cpus_allowed > 1)
 		enqueue_pushable_dl_task(rq, p);
 }
 
@@ -2565,6 +2571,10 @@ static void start_hrtick_dl(struct rq *rq, struct sched_dl_entity *dl_se)
 }
 #endif /* !CONFIG_SCHED_HRTICK */
 
+/*
+ * DL keeps current in tree, because ->deadline is not typically changed while
+ * a task is runnable.
+ */
 static void set_next_task_dl(struct rq *rq, struct task_struct *p, bool first)
 {
 	struct sched_dl_entity *dl_se = &p->dl;
@@ -2585,6 +2595,9 @@ static void set_next_task_dl(struct rq *rq, struct task_struct *p, bool first)
 
 	deadline_queue_push_tasks(rq);
 
+	WARN_ON_ONCE(dl_rq->curr);
+	dl_rq->curr = dl_se;
+
 	if (hrtick_enabled_dl(rq))
 		start_hrtick_dl(rq, &p->dl);
 }
@@ -2640,17 +2653,20 @@ static void put_prev_task_dl(struct rq *rq, struct task_struct *p, struct task_s
 	struct sched_dl_entity *dl_se = &p->dl;
 	struct dl_rq *dl_rq = &rq->dl;
 
-	if (on_dl_rq(&p->dl))
+	if (on_dl_rq(dl_se))
 		update_stats_wait_start_dl(dl_rq, dl_se);
 
 	update_curr_dl(rq);
 
 	update_dl_rq_load_avg(rq_clock_pelt(rq), rq, 1);
 
+	WARN_ON_ONCE(dl_rq->curr != dl_se);
+	dl_rq->curr = NULL;
+
 	if (task_is_blocked(p))
 		return;
 
-	if (on_dl_rq(&p->dl) && p->nr_cpus_allowed > 1)
+	if (on_dl_rq(dl_se) && p->nr_cpus_allowed > 1)
 		enqueue_pushable_dl_task(rq, p);
 }
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index fd36ae390520..fb7e6c1f31e2 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -880,6 +880,7 @@ struct dl_rq {
 
 	bool			overloaded;
 
+	struct sched_dl_entity	*curr;
 	/*
 	 * Tasks on this rq that can be pushed away. They are kept in
 	 * an rb-tree, ordered by tasks' deadlines, with caching

On Wed, Mar 4, 2026 at 1:51 AM Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Mar 04, 2026 at 08:06:53AM +0100, Juri Lelli wrote:
> > On 03/03/26 19:41, John Stultz wrote:
> > > In testing with the full Proxy Execution patch stack, I found
> > > I would occasionally trip over the !RB_EMPTY_NODE() WARN_ON in
> > > enqueue_pushable_dl_task(), where the task we're adding to the
> > > pushable list is already enqueued.
> > >
> > > This triggers from put_prev_task_dl(), where it seems we go into
> > > put_prev_task_dl()
> > > -> update_curr_dl()
> > >    -> update_curr_dl_se() [hitting the dl_runtime_exceeded() case]
> > >       -> enqueue_task_dl()
> > >          -> enqueue_pushable_dl_task()
> > >
> > > Adding the task to the pushable the first time.
> >
> > Ah, so in case the task is boosted (or we fail to start the
> > replenishment timer).
> >
> > > Then we back up the call stack to put_prev_task_dl(), which at
> > > the end again calls enqueue_pushable_dl_task(), trying to add it
> > > a second time, tripping the warning.
> > >
> > > To avoid this, add a dl_task_pushable() helper which we can use
> > > to replace the RB_EMPTY_NODE checks elsewhere, and then before
> > > enqueueing in put_prev_task_dl(), we can first check
> > > dl_task_pushable() to avoid the double enqueue.
> >
> > Can't we just return early (as we do already in dequeue_pushable
> > _dl_task()) in enqueue_pushable_dl_task() instead of checking before
> > calling that function?
>
> So I was mightily confused for a moment by all this.
>
> But it turns out DL keeps current *in* the tree; since ->deadline is a
> lot less mutable than ->vruntime this is possible.
>
> But that also means that set_next_task() / put_prev_task() are
> 'simpler'.
>
> However, in this case I think they are too simple, and its leading to
> problems.
>
> So update_curr_dl() needs to dequeue+enqueue because it is pushing
> ->deadline (because current is in tree). Then because of that, it also
> ends up doing enqueue_pushable, but that is actively wrong, you must not
> do that on current.
>
> Only once you're doing put_prev_task() must you do that.
>
> Imagine this happens because of an update_curr() that is not from
> put_prev_task(), then you end up with current on the pushable list,
> which is a big no-no.
>
> So I think I'd like to see dl_rq->curr tracking, similar to what we have
> for fair.
>
> If you do that (see below), you'll probably find this case was already
> handled 'correctly' but was broken by the PE thing ;-)
>
> Note the !task_current() clause in enqueue_task_dl() guarding
> enqueue_pushable_dl_task().
>
> Hmm?

So yes, you're right it is really a proxy-exec thing. Apologies for my
confusion. :)

After digging further, the specific case I was hitting it with was
with the full proxy stack, when the lock owner releases the lock and
wake's the donor.

In try_to_wake_up() we'd hit the proxy_needs_return(), which since
we're waking the donor, we proxy_resched_idle() to temporarily make
idle the donor, and call put_prev_task() on rq->donor.

So it was then in put_prev_task_dl() that we'd update_curr_dl(), do
the dequeue+enqueue and since its the donor, not task_current() we'd
get added to the pushable list. Then at the end of put_prev_task_dl()
we'd again call enqueue_pushable_dl() and hit the warning as the
pushable_dl_task node isn't empty.

And because without proxy-exec put_prev_task() is only called on
current, the task_current() check in enqueue_task_dl() would prevent
the first enqueue_pushable_dl().

Now your patch does resolve this for proxy-exec, as since the donor
will be dl_rq->curr we'll skip enquing it as pushable from
update_curr_dl(), and we clear dl_rq->curr right afterwards so that we
only make it pushable once.

That said, I had to make some modifications:
1) As mentioned earlier, I needed to move the dl_rq->curr assignment
up before the !first check in set_next_task_dl(), so that we properly
set the curr value when tasks switches to DL (via
__sched_setscheduler)

2) I needed to still preserve the task_current() check in
enqueue_task_dl() that you dropped (replacing with the dl_rq->curr
check),  as we'd trip a similar problem when the current task was
switched to DL, since the rq->curr would be enqueued to DL and since
set_next_task() hasn't run (and won't if rq->curr isn't the donor as
well), rq_dl->curr would go unset and it would be made pushable.

With those two tweaks it has been running ok in testing.

I've also split out your cleanups into a separate patch just so the
functional change is easier to review.

Since this is proxy-exec specific, I'll include these both in my next
submission of the donor-migration logic.

Let me know if you think I'm still missing anything subtle here.

thanks
-john

On Fri, Mar 06, 2026 at 03:45:12PM -0800, John Stultz wrote:
> With those two tweaks it has been running ok in testing.
> 
> I've also split out your cleanups into a separate patch just so the
> functional change is easier to review.
> 
> Since this is proxy-exec specific, I'll include these both in my next
> submission of the donor-migration logic.
> 
> Let me know if you think I'm still missing anything subtle here.

Sounds about right.

Thanks!

On Wed, Mar 4, 2026 at 1:51 AM Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Mar 04, 2026 at 08:06:53AM +0100, Juri Lelli wrote:
> > On 03/03/26 19:41, John Stultz wrote:
> > > To avoid this, add a dl_task_pushable() helper which we can use
> > > to replace the RB_EMPTY_NODE checks elsewhere, and then before
> > > enqueueing in put_prev_task_dl(), we can first check
> > > dl_task_pushable() to avoid the double enqueue.
> >
> > Can't we just return early (as we do already in dequeue_pushable
> > _dl_task()) in enqueue_pushable_dl_task() instead of checking before
> > calling that function?
>
> So I was mightily confused for a moment by all this.
>
> But it turns out DL keeps current *in* the tree; since ->deadline is a
> lot less mutable than ->vruntime this is possible.
>
> But that also means that set_next_task() / put_prev_task() are
> 'simpler'.
>
> However, in this case I think they are too simple, and its leading to
> problems.
>
> So update_curr_dl() needs to dequeue+enqueue because it is pushing
> ->deadline (because current is in tree). Then because of that, it also
> ends up doing enqueue_pushable, but that is actively wrong, you must not
> do that on current.
>
> Only once you're doing put_prev_task() must you do that.
>
> Imagine this happens because of an update_curr() that is not from
> put_prev_task(), then you end up with current on the pushable list,
> which is a big no-no.
>
> So I think I'd like to see dl_rq->curr tracking, similar to what we have
> for fair.
>
> If you do that (see below), you'll probably find this case was already
> handled 'correctly' but was broken by the PE thing ;-)
>
> Note the !task_current() clause in enqueue_task_dl() guarding
> enqueue_pushable_dl_task().
>
> Hmm?
>
> ---
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 9869025941a0..a7db81d17082 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -2565,6 +2571,10 @@ static void start_hrtick_dl(struct rq *rq, struct sched_dl_entity *dl_se)
>  }
>  #endif /* !CONFIG_SCHED_HRTICK */
>
> +/*
> + * DL keeps current in tree, because ->deadline is not typically changed while
> + * a task is runnable.
> + */
>  static void set_next_task_dl(struct rq *rq, struct task_struct *p, bool first)
>  {
>         struct sched_dl_entity *dl_se = &p->dl;
> @@ -2585,6 +2595,9 @@ static void set_next_task_dl(struct rq *rq, struct task_struct *p, bool first)
>
>         deadline_queue_push_tasks(rq);
>
> +       WARN_ON_ONCE(dl_rq->curr);
> +       dl_rq->curr = dl_se;
> +

So I think this part assigning the curr ptr needs to be set above the
"if (!first) return;" logic, otherwise when a running task swtiches to
DL (via __sched_setscheduler()), we will trip the newly added warning
when we later put_prev and dl_rq->curr is still NULL.

But with that tweak, so far this is looking ok.   I'll continue
testing tonight and then resend it to the list.

thanks
-john

On 04/03/26 10:51, Peter Zijlstra wrote:
> On Wed, Mar 04, 2026 at 08:06:53AM +0100, Juri Lelli wrote:
> > Hello,
> > 
> > On 03/03/26 19:41, John Stultz wrote:
> > > In testing with the full Proxy Execution patch stack, I found
> > > I would occasionally trip over the !RB_EMPTY_NODE() WARN_ON in
> > > enqueue_pushable_dl_task(), where the task we're adding to the
> > > pushable list is already enqueued.
> > > 
> > > This triggers from put_prev_task_dl(), where it seems we go into
> > > put_prev_task_dl()
> > > -> update_curr_dl()
> > >    -> update_curr_dl_se() [hitting the dl_runtime_exceeded() case]
> > >       -> enqueue_task_dl()
> > >          -> enqueue_pushable_dl_task()
> > > 
> > > Adding the task to the pushable the first time.
> > 
> > Ah, so in case the task is boosted (or we fail to start the
> > replenishment timer).
> > 
> > > Then we back up the call stack to put_prev_task_dl(), which at
> > > the end again calls enqueue_pushable_dl_task(), trying to add it
> > > a second time, tripping the warning.
> > > 
> > > To avoid this, add a dl_task_pushable() helper which we can use
> > > to replace the RB_EMPTY_NODE checks elsewhere, and then before
> > > enqueueing in put_prev_task_dl(), we can first check
> > > dl_task_pushable() to avoid the double enqueue.
> > 
> > Can't we just return early (as we do already in dequeue_pushable
> > _dl_task()) in enqueue_pushable_dl_task() instead of checking before
> > calling that function?
> 
> So I was mightily confused for a moment by all this.
> 
> But it turns out DL keeps current *in* the tree; since ->deadline is a
> lot less mutable than ->vruntime this is possible.
> 
> But that also means that set_next_task() / put_prev_task() are
> 'simpler'.
> 
> However, in this case I think they are too simple, and its leading to
> problems.
> 
> So update_curr_dl() needs to dequeue+enqueue because it is pushing
> ->deadline (because current is in tree). Then because of that, it also
> ends up doing enqueue_pushable, but that is actively wrong, you must not
> do that on current.
> 
> Only once you're doing put_prev_task() must you do that.
> 
> Imagine this happens because of an update_curr() that is not from
> put_prev_task(), then you end up with current on the pushable list,
> which is a big no-no.
> 
> So I think I'd like to see dl_rq->curr tracking, similar to what we have
> for fair.
> 
> If you do that (see below), you'll probably find this case was already
> handled 'correctly' but was broken by the PE thing ;-)
> 
> Note the !task_current() clause in enqueue_task_dl() guarding
> enqueue_pushable_dl_task().

Ah, indeed. The below makes sense to me. Let's see if John's tests are
happy as well.

Thanks,
Juri