Re: [PATCH v2 12/12] sched: Remove sched_class::balance()

[Kayra Cizmeci <kayracizmeci@gmail.com>]

Hi Peter!

I was just going to sleep but I thought I'll finish reviewing this, then
sleep or maybe don't sleep.

Big mistake. Nevermind let me have a look...

On Wed, Nov 11, 2026 at 02:11:52PM +0100, Peter Zijlstra wrote:
> On Wed, Nov 11, 2026 at 02:05:12PM +0100, Peter Zijlstra wrote:
> > On Wed, Nov 11, 2026 at 01:18:42AM -1000, Tejun Heo wrote:
> > > Hello,
> > > 
> > > On Sat, Nov 07, 2026 at 12:59:25PM +0100, Peter Zijlstra wrote:
> > > >  static struct task_struct *pick_task_rt(struct rq *rq, struct rq_flags *rf)
> > > >  {
> > > > -	struct task_struct *p;
> > > > +	rq_modified_begin(rq, &rt_sched_class);
> > > > +	balance_rt(rq, rf);
> > > > +	if (rq_modified_above(rq, &rt_sched_class))
> > > > +		return RETRY_TASK;
> > > 
> > > This exposes an underlying problem on the DL side. rq->next_class is
> > > raised only by wakeup_preempt(), but dl_server_start(), dl_task_timer()
> > > and dl_server_timer() enqueue a DL entity and open-code resched_curr()
> > > without going through it. prev_balance() ran before any class was
> > > picked, so it didn't matter. Now, pick_task_dl() has already returned
> > > NULL when balance_rt() drops the lock, and rq_modified_above() is the
> > > only thing which can notice a DL entity queued in the meantime:
> > > 
> > >   CPU0 RT task blocks, pick_task_dl() returns NULL
> > >        pick_task_rt(), balance_rt() drops the rq lock
> > >   CPU1 wakes a fair task onto CPU0, dl_server_start()
> > >   CPU0 relocks, rq_modified_above() is false
> > >        picks an RT task with the fair server queued
> > 
> > A very nice catch, but I'm not entirely convinced it wasn't a problem
> > before.
> > 
> > Similar argument could be made for FAIR vs EXT, and picking a FAIR task
> > while an EXT server is pickable.
> > 
> > > resched_curr() doesn't help as __schedule() clears it on prev after the
> > > pick. I can trigger this with NO_RT_PUSH_IPI and a delay added in the
> > > lock drop. With RT_PUSH_IPI, pull_rt_task() doesn't drop the lock.
> > > 
> > > Maybe the DL sites should raise rq->next_class where they enqueue and
> > > resched, so that they behave the same as regular wakeups?
> > 
> > Yes, something like that. I think the below captures all enqueues,
> > irrespective of rescheds. I'll add it as fix at the start of the series.
> > 
> > (although maybe I'll massage this a little more and create a helper,
> > doing the same thing over and over like this is daft)
> > 
> > Thanks!
> 
> ---
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -715,8 +715,7 @@ static struct rq *dl_task_offline_migrat
>  	return later_rq;
>  }
>  
> -static void
> -enqueue_dl_entity(struct sched_dl_entity *dl_se, int flags);
> +static void enqueue_dl_entity(struct rq *rq, struct sched_dl_entity *dl_se, int flags);
>  static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags);
>  static void dequeue_dl_entity(struct sched_dl_entity *dl_se, int flags);
>  static void wakeup_preempt_dl(struct rq *rq, struct task_struct *p, int flags);
> @@ -1184,7 +1183,7 @@ static enum hrtimer_restart dl_server_ti
>  			dl_se->dl_defer_running = 1;
>  		}
>  
> -		enqueue_dl_entity(dl_se, ENQUEUE_REPLENISH);
> +		enqueue_dl_entity(rq, dl_se, ENQUEUE_REPLENISH);
>  
>  		if (!dl_task(dl_se->rq->curr) || dl_entity_preempt(dl_se, &dl_se->rq->curr->dl))
>  			resched_curr(rq);
> @@ -1520,7 +1519,7 @@ static void update_curr_dl_se(struct rq
>  					replenish_dl_new_period(dl_se, rq);
>  					start_dl_timer(dl_se);
>  				} else {
> -					enqueue_dl_entity(dl_se, ENQUEUE_REPLENISH);
> +					enqueue_dl_entity(rq, dl_se, ENQUEUE_REPLENISH);
>  				}
>  			} else {
>  				enqueue_task_dl(rq, dl_task_of(dl_se), ENQUEUE_REPLENISH);
> @@ -1812,7 +1811,7 @@ void dl_server_start(struct sched_dl_ent
>  
>  	trace_sched_dl_server_start_tp(dl_se, cpu_of(rq), dl_get_type(dl_se, rq));
>  	dl_se->dl_server_active = 1;
> -	enqueue_dl_entity(dl_se, ENQUEUE_WAKEUP);
> +	enqueue_dl_entity(rq, dl_se, ENQUEUE_WAKEUP);
>  	if (!dl_task(dl_se->rq->curr) || dl_entity_preempt(dl_se, &rq->curr->dl))
>  		resched_curr(dl_se->rq);
>  }
> @@ -2379,8 +2378,8 @@ static void __dequeue_dl_entity(struct s
>  	dec_dl_tasks(dl_se, dl_rq);
>  }
>  
> -static void
> -enqueue_dl_entity(struct sched_dl_entity *dl_se, int flags)
> +static bool
> +_enqueue_dl_entity(struct sched_dl_entity *dl_se, int flags)
>  {
>  	WARN_ON_ONCE(on_dl_rq(dl_se));
>  
> @@ -2418,7 +2417,7 @@ enqueue_dl_entity(struct sched_dl_entity
>  		if (flags & ENQUEUE_WAKEUP)
>  			task_contending(dl_se, flags);
>  
> -		return;
> +		return false;
>  	}
>  
>  	/*
> @@ -2442,7 +2441,7 @@ enqueue_dl_entity(struct sched_dl_entity
>  	 * deferred task and still got to wait, don't enqueue.
>  	 */
>  	if (dl_se->dl_throttled && start_dl_timer(dl_se))
> -		return;
> +		return false;
>  
>  	/*
>  	 * We're about to enqueue, make sure we're not ->dl_throttled!
> @@ -2457,6 +2456,13 @@ enqueue_dl_entity(struct sched_dl_entity
>  	}
>  
>  	__enqueue_dl_entity(dl_se);
> +	return true;
> +}
> +
> +static void enqueue_dl_entity(struct rq *rq, struct sched_dl_entity *dl_se, int flags)
> +{
> +	if (_enqueue_dl_entity(dl_se, flags))
> +		rq->next_class = &dl_sched_class;
>  }
>  
>  static void dequeue_dl_entity(struct sched_dl_entity *dl_se, int flags)
> @@ -2534,7 +2540,7 @@ static void enqueue_task_dl(struct rq *r
>  	if (task_on_rq_migrating(p))
>  		flags |= ENQUEUE_MIGRATING;
>  
> -	enqueue_dl_entity(dl_se, flags);
> +	enqueue_dl_entity(rq, dl_se, flags);
>  
>  	if (dl_server(dl_se))
>  		return;

Let's say a fair task named TA is working in CPU1, a DL task named DA is
waking up. We enter the normal wakeup path, and call enqueue_task_dl()
and from there enqueue_dl_entity(). enqueue_dl_entity() sets next_class
to dl_sched_class. Later enqueue, when we call wakeup_preempt TA does
not gets preempted because:

void wakeup_preempt(struct rq *rq, struct task_struct *p, int flags)
{
	struct task_struct *donor = rq->donor;

	if (p->sched_class == rq->next_class) {
		rq->next_class->wakeup_preempt(rq, p, flags);

We already set next_class to DL, so we call wakeup_preempt_dl().

And it just early returns.

So, we have a fair task working while a new woken DL task just sits
around...

I also tested this:

On hackery, the latency of DA was 12 us, with hackery + above diff it
was 200 ms. On idle, it was 12 us to 3 seconds.

I'm not sure what is the cleanest fix tho. So this time there is no
diff.

Hope I'm not missing something :-).



Cheers,

Kayra