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

[Vincent Guittot]

On Wed, 21 Oct 2026 at 15:13, Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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

>