Re: [PATCH v3 7/7] sched/eevdf: Move to a single runqueue

[Vincent Guittot <vincent.guittot@linaro.org>]

Le dimanche 18 oct. 2026 à 08:49:29 (+0200), Mike Galbraith a écrit :
> Greetings!
> 
> On Fri, 2026-10-16 at 15:18 +0200, Jakub Okoński wrote:
> > Hi Peter,
> > 
> > On 6/5/26 14:40, Peter Zijlstra wrote:
> > 
> > > Change fair/cgroup to a single runqueue.
> > > 
> > > [...]
> > > 
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -9916,7 +9837,6 @@ static void wakeup_preempt_fair(struct r
> > >  	if (!sched_feat(WAKEUP_PREEMPTION))
> > >  		return;
> > >  
> > > -	find_matching_se(&se, &pse);
> > >  	WARN_ON_ONCE(!pse);
> > >  
> > >  	cse_is_idle = se_is_idle(se);
> > 
> > I think this changes how cpu.idle affects wakeup preemption.
> 
> My box claims this to be unrelated to the jitter you're seeing, as it
> is present in 7.2.11, with and without the sched-flat set applied.

Yes, we don't take into account whether the task group is sched_idle

Jakub, Mike,
Could you try the patch below ? It solves the problem on my setup

---
 kernel/sched/fair.c | 80 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 68 insertions(+), 12 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 0f2364479f9d..d17e88ad12a5 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -433,6 +433,38 @@ static inline struct sched_entity *parent_entity(const struct sched_entity *se)
 	return se->parent;
 }
 
+static void
+find_matching_se(struct sched_entity **se, struct sched_entity **pse)
+{
+	int se_depth, pse_depth;
+
+	/*
+	 * preemption test can be made between sibling entities who are in the
+	 * same cfs_rq i.e who have a common parent. Walk up the hierarchy of
+	 * both tasks until we find their ancestors who are siblings of common
+	 * parent.
+	 */
+
+	/* First walk up until both entities are at same depth */
+	se_depth = (*se)->depth;
+	pse_depth = (*pse)->depth;
+
+	while (se_depth > pse_depth) {
+		se_depth--;
+		*se = parent_entity(*se);
+	}
+
+	while (pse_depth > se_depth) {
+		pse_depth--;
+		*pse = parent_entity(*pse);
+	}
+
+	while (!is_same_group(*se, *pse)) {
+		*se = parent_entity(*se);
+		*pse = parent_entity(*pse);
+	}
+}
+
 static int tg_is_idle(struct task_group *tg)
 {
 	return tg->idle > 0;
@@ -476,6 +508,11 @@ static inline struct sched_entity *parent_entity(struct sched_entity *se)
 	return NULL;
 }
 
+static inline void
+find_matching_se(struct sched_entity **se, struct sched_entity **pse)
+{
+}
+
 static inline int tg_is_idle(struct task_group *tg)
 {
 	return 0;
@@ -10129,7 +10166,34 @@ preempt_sync(struct rq *rq, int wake_flags,
 }
 
 /*
- * Preempt the current task with a newly woken task if needed:
+ * Preempt an idle entity in favor of a non-idle entity (and don't preempt
+ * in the inverse case).
+ */
+static bool wakeup_preempt_sched_idle(struct sched_entity *cse, struct sched_entity *pse)
+{
+	int cse_is_idle, pse_is_idle;
+
+	find_matching_se(&cse, &pse);
+	WARN_ON_ONCE(!pse);
+
+	cse_is_idle = se_is_idle(cse);
+	pse_is_idle = se_is_idle(pse);
+
+	/*
+	 * Preempt an idle entity in favor of a non-idle entity (and don't preempt
+	 * in the inverse case).
+	 */
+	if (cse_is_idle && !pse_is_idle)
+		return PREEMPT_WAKEUP_RESCHED;
+
+	if (cse_is_idle != pse_is_idle)
+		return PREEMPT_WAKEUP_NONE;
+
+	return PREEMPT_WAKEUP_PICK;
+}
+
+/*
+ * Preempt the current task with a newly woken task if needed.
  */
 static void wakeup_preempt_fair(struct rq *rq, struct task_struct *p, int wake_flags)
 {
@@ -10174,22 +10238,14 @@ static void wakeup_preempt_fair(struct rq *rq, struct task_struct *p, int wake_f
 	if (!sched_feat(WAKEUP_PREEMPTION))
 		return;
 
-	WARN_ON_ONCE(!pse);
-
-	cse_is_idle = se_is_idle(se);
-	pse_is_idle = se_is_idle(pse);
+	preempt_action = wakeup_preempt_sched_idle(se, pse);
 
-	nse = se;
-	/*
-	 * Preempt an idle entity in favor of a non-idle entity (and don't preempt
-	 * in the inverse case).
-	 */
-	if (cse_is_idle && !pse_is_idle)
+	if (preempt_action == PREEMPT_WAKEUP_RESCHED)
 		goto preempt;
 
 	update_curr_fair(rq);
 
-	if (cse_is_idle != pse_is_idle)
+	if (preempt_action == PREEMPT_WAKEUP_NONE)
 		goto update;
 
 	/*
-- 
2.53.0


> 
> > With the single runqueue, find_matching_se() is gone and se_is_idle()
> > reduces to task_has_idle_policy(). Wouldn't this mean that SCHED_NORMAL
> > tasks in a cpu.idle cgroup are no longer treated as idle for
> > wakeup_preempt_fair()?
> >
> > 
> > I tested my existing 7.2.9 desktop against 7.3-rc6 with the setup below.
> > cyclictest doesn't reproduce it because it just sleeps and wakes, no CPU
> > burn and therefore its accrued EEVDF lag keeps the latency low?
> > 
> > cc -O2 -o wakelat wakelat.c
> > systemd-run --user --collect --unit=spin -p AllowedCPUs=7 -p CPUWeight=idle sh -c 'while :; do :; done'
> > systemd-run --user --collect --wait --pipe --same-dir -p AllowedCPUs=7 ./wakelat 1000 15 200 |
> >   mlr --inidx --ifs space --opprint --barred --right label lat \
> >     then stats1 -a count,p50,p90,p99,max -f lat then rename -r '^lat_,' \
> >     then put -q 'emit mapsum({"kernel": system("uname -r")}, $*)'
> > systemctl --user stop spin
> > 
> > wakelat wakes on a fixed 1 ms schedule, records how late each wakeup
> > was, then busy-loops for 200 micros (wall clock).
> > 
> > +-----------+-------+-----+-----+------+------+
> > >    kernel | count | p50 | p90 |  p99 |  max |
> > +-----------+-------+-----+-----+------+------+
> > >     7.2.9 | 14998 |  11 |  11 |   12 | 1869 |
> > > 7.3.0-rc6 | 13853 |  11 |  12 | 2029 | 2116 |
> > +-----------+-------+-----+-----+------+------+
> > 
> > count is out of 15000 periods. rc6 had 1147 wakeups delayed by over 1 ms.
> 
> Thanks a bunch for the proggy. I've been looking for a clear signal
> with various loads, and stuffing wakelat+awk into one of my scripts
> provided a perfectly clear signal, which I bisected and confirmed.
> 
> Note: below is box pointing it's little silicon finger at the source of
> aforementioned jitter, not necessarily at breakage.
> 
> Reverting the fingered commit removed the jitter from 7.2.11, and force
> reverting 68e37487810a + 333238da9a19 removed it from master as well.
> 
> 333238da9a193ffc58792995f3e951e4cb87bfd2 is the first 'bad' commit
> commit 333238da9a193ffc58792995f3e951e4cb87bfd2
> Author: Vincent Guittot <vincent.guittot@linaro.org>
> Date:   Fri Aug 14 15:52:41 2026 +0200
> 
>     sched: Update time before requeueing delayed entities
>     
>     In order to compute the right lag, it is required to update time to 'now'.
>     Without this, the delayed entity might appear younger than it really is and
>     receive less compensation for having waited.
>     
>     Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
>     Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
>  kernel/sched/fair.c | 2 ++
>  1 file changed, 2 insertions(+)
>