[PATCH v2] sched/eevdf: Fix sched idle task group preemption

[PATCH v2] sched/eevdf: Fix sched idle task group preemption

[Vincent Guittot]

Even with flat hierarchy which puts all runnable tasks in the same cfs rq,
we need to parse the hierarchy to check for sched_idle task group.

Reported-by: Jakub Okoński <jakub@okonski.org>
Link: https://lore.kernel.org/all/58dc8e8a-7260-43ce-aebb-8d9c29a6afc9@okonski.org/
Fixes: 85570f10a4c6 ("sched/eevdf: Move to a single runqueue")
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---

Changes since v1:
- fix prototype and cleanup unsued variable

 kernel/sched/fair.c | 82 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 69 insertions(+), 13 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 0f2364479f9d..27ce3eb467a0 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,35 @@ 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 inline enum preempt_wakeup_action
+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)
 {
@@ -10137,7 +10202,6 @@ static void wakeup_preempt_fair(struct rq *rq, struct task_struct *p, int wake_f
 	struct task_struct *donor = rq->donor;
 	struct sched_entity *nse, *se = &donor->se, *pse = &p->se;
 	struct cfs_rq *cfs_rq = &rq->cfs;
-	int cse_is_idle, pse_is_idle;
 
 	/*
 	 * XXX Getting preempted by higher class, try and find idle CPU?
@@ -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

Re: [PATCH v2] sched/eevdf: Fix sched idle task group preemption

[Kayra Cizmeci]

On Mon, Oct 19, 2026 at 08:26:22PM +0200, Vincent Guittot wrote:
> Even with flat hierarchy which puts all runnable tasks in the same cfs rq,
> we need to parse the hierarchy to check for sched_idle task group.
> 
> Reported-by: Jakub Okoński <jakub@okonski.org>
> Link: https://lore.kernel.org/all/58dc8e8a-7260-43ce-aebb-8d9c29a6afc9@okonski.org/
> Fixes: 85570f10a4c6 ("sched/eevdf: Move to a single runqueue")
> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
> ---
> 
> Changes since v1:
> - fix prototype and cleanup unsued variable
> 
>  kernel/sched/fair.c | 82 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 69 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 0f2364479f9d..27ce3eb467a0 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);
> +	}
> +}
> +

So in a hierarchy like:

    ROOT
     /\
    /  \
   /    \
  GA    GB
   |    /\
   V   /  \
  TA  GC   GD
      |    |
      V    V
      TB   TC

Where GA is idle, GB is not, GC is idle and GD is not.

1. TA == se & TB == pse:
pse_depth > se_depth is true, so we goto GC. Now pse = GC.
Then we enter !is_same_group() block where we go up one more
time, so now se is GA, and pse is GB.


>  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,35 @@ 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 inline enum preempt_wakeup_action
> +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;
> +}
> +
> +/*


2. In here, cse_is_idle is true, while pse_is_idle is false.
So we return PREEMPT_WAKEUP_RESCHED. If these are inversed,
we return PREEMPT_WAKEUP_NONE. If they are equal, we 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)
>  {
> @@ -10137,7 +10202,6 @@ static void wakeup_preempt_fair(struct rq *rq, struct task_struct *p, int wake_f
>  	struct task_struct *donor = rq->donor;
>  	struct sched_entity *nse, *se = &donor->se, *pse = &p->se;
>  	struct cfs_rq *cfs_rq = &rq->cfs;
> -	int cse_is_idle, pse_is_idle;
>  
>  	/*
>  	 * XXX Getting preempted by higher class, try and find idle CPU?
> @@ -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
> 

This only changes where we look, not what we do
with the results. AFAICT. I added my review notes too. They are not
essential but I still did wanted to add them IDK why tho.

Reviewed-by: Kayra Cizmeci <kayracizmeci@gmail.com>

Thanks,

Kayra

Re: [PATCH v2] sched/eevdf: Fix sched idle task group preemption

[Peter Zijlstra]

On Mon, Oct 19, 2026 at 08:26:22PM +0200, Vincent Guittot wrote:
> Even with flat hierarchy which puts all runnable tasks in the same cfs rq,
> we need to parse the hierarchy to check for sched_idle task group.

I F'ing hate cgroups :-(


> +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,35 @@ 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 inline enum preempt_wakeup_action
> +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)
>  {
> @@ -10137,7 +10202,6 @@ static void wakeup_preempt_fair(struct rq *rq, struct task_struct *p, int wake_f
>  	struct task_struct *donor = rq->donor;
>  	struct sched_entity *nse, *se = &donor->se, *pse = &p->se;
>  	struct cfs_rq *cfs_rq = &rq->cfs;
> -	int cse_is_idle, pse_is_idle;
>  
>  	/*
>  	 * XXX Getting preempted by higher class, try and find idle CPU?
> @@ -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;
>  
>  	/*

What is the actual semantics of cpu.idle nonsense? Why do we care about
this common ancestor?

Can't we do any of:

 - propagate cfs_rq->idle to all child groups in sched_group_set_idle(); or

 - have se_is_idle() walk up and report true if any of the ancestors is
   idle?

I despise this find_matching_se() thing and was so very glad to have
deleted it.

Re: [PATCH v2] sched/eevdf: Fix sched idle task group preemption

[Vincent Guittot]

On Wed, 21 Oct 2026 at 15:13, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Oct 19, 2026 at 08:26:22PM +0200, Vincent Guittot wrote:
> > Even with flat hierarchy which puts all runnable tasks in the same cfs rq,
> > we need to parse the hierarchy to check for sched_idle task group.
>
> I F'ing hate cgroups :-(
>
>
> > +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,35 @@ 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 inline enum preempt_wakeup_action
> > +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)
> >  {
> > @@ -10137,7 +10202,6 @@ static void wakeup_preempt_fair(struct rq *rq, struct task_struct *p, int wake_f
> >       struct task_struct *donor = rq->donor;
> >       struct sched_entity *nse, *se = &donor->se, *pse = &p->se;
> >       struct cfs_rq *cfs_rq = &rq->cfs;
> > -     int cse_is_idle, pse_is_idle;
> >
> >       /*
> >        * XXX Getting preempted by higher class, try and find idle CPU?
> > @@ -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;
> >
> >       /*
>
> What is the actual semantics of cpu.idle nonsense? Why do we care about
> this common ancestor?

AFAICT, cpu.idle applied to one level and against entities at the same level

>
> Can't we do any of:
>
>  - propagate cfs_rq->idle to all child groups in sched_group_set_idle(); or
>
>  - have se_is_idle() walk up and report true if any of the ancestors is
>    idle?

I'm fine with either way but this will change the behavior.

>
> I despise this find_matching_se() thing and was so very glad to have
> deleted it.

+1

>

Re: [PATCH v2] sched/eevdf: Fix sched idle task group preemption

[Kayra Cizmeci]

On Wed, Oct 21, 2026 at 03:32:59PM +0200, Vincent Guittot wrote:
> On Wed, 21 Oct 2026 at 15:13, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Mon, Oct 19, 2026 at 08:26:22PM +0200, Vincent Guittot wrote:
> > > Even with flat hierarchy which puts all runnable tasks in the same cfs rq,
> > > we need to parse the hierarchy to check for sched_idle task group.
> >
> > I F'ing hate cgroups :-(
> >
> >
> > > +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,35 @@ 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 inline enum preempt_wakeup_action
> > > +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)
> > >  {
> > > @@ -10137,7 +10202,6 @@ static void wakeup_preempt_fair(struct rq *rq, struct task_struct *p, int wake_f
> > >       struct task_struct *donor = rq->donor;
> > >       struct sched_entity *nse, *se = &donor->se, *pse = &p->se;
> > >       struct cfs_rq *cfs_rq = &rq->cfs;
> > > -     int cse_is_idle, pse_is_idle;
> > >
> > >       /*
> > >        * XXX Getting preempted by higher class, try and find idle CPU?
> > > @@ -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;
> > >
> > >       /*
> >
> > What is the actual semantics of cpu.idle nonsense? Why do we care about
> > this common ancestor?
> 
> AFAICT, cpu.idle applied to one level and against entities at the same level
>

Yeah. Like on here:

    ROOT
     /\
    /  \
   /    \
  GA    GB
   |    /\
   V   /  \
  TA  GC   GD
      |    |
      V    V
      TB   TC

If GA is idle and GB is not, while GC is idle and were looking to TA and
TB we gotta choose TB. But if we just look to GC and GA we would choose
neither. .idle just applies to one level as Vincent says.

> >
> > Can't we do any of:
> >
> >  - propagate cfs_rq->idle to all child groups in sched_group_set_idle(); or
> >
> >  - have se_is_idle() walk up and report true if any of the ancestors is
> >    idle?
> 
> I'm fine with either way but this will change the behavior.
> 

So is it good or bad to change the behavior in this case? If we don't
want to change it are there any cleaner way? Well I can't find any.

> >
> > I despise this find_matching_se() thing and was so very glad to have
> > deleted it.
> 
> +1
> 
> >

Re: [PATCH v2] sched/eevdf: Fix sched idle task group preemption

[Peter Zijlstra]

On Wed, Oct 21, 2026 at 04:41:52PM +0300, Kayra Cizmeci wrote:
> On Wed, Oct 21, 2026 at 03:32:59PM +0200, Vincent Guittot wrote:
> > On Wed, 21 Oct 2026 at 15:13, Peter Zijlstra <peterz@infradead.org> wrote:

> > > What is the actual semantics of cpu.idle nonsense? Why do we care about
> > > this common ancestor?
> > 
> > AFAICT, cpu.idle applied to one level and against entities at the same level
> >
> 
> Yeah. Like on here:
> 
>     ROOT
>      /\
>     /  \
>    /    \
>   GA*   GB
>    |    /\
>    V   /  \
>   TA  GC*  GD
>       |    |
>       V    V
>       TB   TC

(marked GA and GC with * to denote their 'idleness')

> If GA is idle and GB is not, while GC is idle and were looking to TA and
> TB we gotta choose TB. But if we just look to GC and GA we would choose
> neither. .idle just applies to one level as Vincent says.

Yeah, that's what we used to do, but the more I think about this, the
less sense it makes! 

Josh added this (added to Cc now), but we have a number of Google folks
on Cc already. It would be good to hear how this is actually used.


So in your example TA and TB are both inside an idle group, and
therefore one can argue their behaviour should be like !idle. But their
common ancestor is root, and so the fact that they're contained in idle
groups gets lost (which seems crazeh).

But worse I think is T{AB} vs TC. In that case none of the common
ancestors (root,GB) resp. is idle. But T{AB} being inside idle should,
IMO. definitely matter vs TC, which has no idle anywhere.

So I think I would very much like to change the semantics here. The
current stuff makes no sense.

Anybody, please chime in with how this idle crap is used and expected to
work.

Re: [PATCH v2] sched/eevdf: Fix sched idle task group preemption

[Kayra Cizmeci]

On Wed, Oct 21, 2026 at 04:50:42PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 21, 2026 at 04:41:52PM +0300, Kayra Cizmeci wrote:
> > On Wed, Oct 21, 2026 at 03:32:59PM +0200, Vincent Guittot wrote:
> > > On Wed, 21 Oct 2026 at 15:13, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > > What is the actual semantics of cpu.idle nonsense? Why do we care about
> > > > this common ancestor?
> > > 
> > > AFAICT, cpu.idle applied to one level and against entities at the same level
> > >
> > 
> > Yeah. Like on here:
> > 
> >     ROOT
> >      /\
> >     /  \
> >    /    \
> >   GA*   GB
> >    |    /\
> >    V   /  \
> >   TA  GC*  GD
> >       |    |
> >       V    V
> >       TB   TC
> 
> (marked GA and GC with * to denote their 'idleness')
> 
> > If GA is idle and GB is not, while GC is idle and were looking to TA and
> > TB we gotta choose TB. But if we just look to GC and GA we would choose
> > neither. .idle just applies to one level as Vincent says.
> 
> Yeah, that's what we used to do, but the more I think about this, the
> less sense it makes! 
> 
> Josh added this (added to Cc now), but we have a number of Google folks
> on Cc already. It would be good to hear how this is actually used.

Yeah.

> 
> 
> So in your example TA and TB are both inside an idle group, and
> therefore one can argue their behaviour should be like !idle. But their
> common ancestor is root, and so the fact that they're contained in idle
> groups gets lost (which seems crazeh).
> 

Actually in that case TB wins over TA, since GA is idle while GB is
not. GC being idle only makes TB lose against TC.

Right now being idle only changes a groups status to its siblings.
Like right now GD being not idle is only a advantage for TC when
its TB vs TC.

> But worse I think is T{AB} vs TC. In that case none of the common
> ancestors (root,GB) resp. is idle. But T{AB} being inside idle should,
> IMO. definitely matter vs TC, which has no idle anywhere.

It does. In the case of TA vs TC, we look to GA and GB, and
TC wins. In TB vs TC we look to GC and GD and again TC wins.


> 
> So I think I would very much like to change the semantics here. The
> current stuff makes no sense.
> 
> Anybody, please chime in with how this idle crap is used and expected to
> work.

Re: [PATCH v2] sched/eevdf: Fix sched idle task group preemption

[Josh Don]

On Wed, Oct 21, 2026 at 7:50 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Oct 21, 2026 at 04:41:52PM +0300, Kayra Cizmeci wrote:
> > On Wed, Oct 21, 2026 at 03:32:59PM +0200, Vincent Guittot wrote:
> > > On Wed, 21 Oct 2026 at 15:13, Peter Zijlstra <peterz@infradead.org> wrote:
>
> > > > What is the actual semantics of cpu.idle nonsense? Why do we care about
> > > > this common ancestor?
> > >
> > > AFAICT, cpu.idle applied to one level and against entities at the same level
> > >
> >
> > Yeah. Like on here:
> >
> >     ROOT
> >      /\
> >     /  \
> >    /    \
> >   GA*   GB
> >    |    /\
> >    V   /  \
> >   TA  GC*  GD
> >       |    |
> >       V    V
> >       TB   TC
>
> (marked GA and GC with * to denote their 'idleness')
>
> > If GA is idle and GB is not, while GC is idle and were looking to TA and
> > TB we gotta choose TB. But if we just look to GC and GA we would choose
> > neither. .idle just applies to one level as Vincent says.
>
> Yeah, that's what we used to do, but the more I think about this, the
> less sense it makes!
>
> Josh added this (added to Cc now), but we have a number of Google folks
> on Cc already. It would be good to hear how this is actually used.
>
>
> So in your example TA and TB are both inside an idle group, and
> therefore one can argue their behaviour should be like !idle. But their
> common ancestor is root, and so the fact that they're contained in idle
> groups gets lost (which seems crazeh).

TA and TB have a matching se of GA and GB, since those entities are
siblings under the common ancestor. Since GB is not idle but GA is, I
would argue that TB should preempt TA.

Consider GA and GB as groups representing different users, and user GB
has internally decided to create an idle cgroup GC to adjust
competition between its own tasks (TB and TC). This should not affect
GB's overall preemption ability against other users.

However in practice doing the exact computation to determine
preemption for relative idle properties (especially on wakeup) is
expensive and perhaps not strictly required. This is also why e.g. the
whole sched_idle_rq stuff is imprecise. We keep an overall tally of
the number of tasks that have a cpu.idle ancestor cgroup and use that
for wakeup placement, but if we were trying to be perfect we would
need to consider the matching_se stuff for that as well. What I'd say
there though is that imprecise placement is better than imprecise
preemption.

So overall, I would say the find_matching_se() stuff gives the more
ideal preemption decision; my first preference would be to keep it. It
avoids a regression with users who delegate cpu.idle internally within
their own cgroup hierarchy. FWIW wakeup preemption already does a
cgroup ancestor walk via update_curr; find_matching_se is similar in
spirit.

But to be clear I also don't think it would be terrible if we did
something like you suggest, cache whether a cgroup has _any_ cpu.idle
ancestor and then just make a single decision on wakeup comparing this
for the two tasks. The result of which is TC preempts T{AB}, but TA
and TB don't preempt each other. In which case I would suggest we
cache that as a part of updating cpu.idle so that we can avoid a
pointer chasing loop in se_is_idle().

Either of these two will at least also restore some semblance of
respecting cgroup cpu.idle in wakeup, as the flattening made it only
consider task level SCHED_IDLE.

>
> But worse I think is T{AB} vs TC. In that case none of the common
> ancestors (root,GB) resp. is idle. But T{AB} being inside idle should,
> IMO. definitely matter vs TC, which has no idle anywhere.
>
> So I think I would very much like to change the semantics here. The
> current stuff makes no sense.
>
> Anybody, please chime in with how this idle crap is used and expected to
> work.

Hopefully the above makes the use a bit more clear; cpu.idle is both a
weight thing and a preemption thing. The weight part is respected
naturally by the hierarchy, and it would be ideal to try to respect
the preemption part as much as feasible.

Best,
Josh