Re: [PATCH] sched/fair: Fix flat hierarchy

[Vincent Guittot]

On Wed, 12 Aug 2026 at 16:03, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Wed, Aug 12, 2026 at 02:50:39PM +0200, Vincent Guittot wrote:
> > When a fair task is enqueued, we must update curr and more precisely
> > its vruntime before placing the enqueued task so avg vruntime will take
> > into account the last exec phase.
> >
> > Example:
> > TA is an always running task in cgroup G0.
> > TB is a short running task (cyclictest) in cgroup G1.
> > The lag of TB always increases up the clamp limit because TB is placed
> > before TA(curr) is updated (since the last tick). When curr(TA) is
> > finally updated, its last exec phase provide positive lag to TB
> >
> > Because TA and TB don't belong to the same group, enqueue_hierarchy()
> > will not update TA's entity when updating curr but only G0's entity at
> > root level.
> >
> > The same applies when dequeuing.
>
> This doesn't quite make sense to me; on the one hand you talk about
> vruntime (which is only relevant for rq->cfs) on the other hand you talk
> about non overlapping cgroup hierarchies.
>
> Hmm, update_curr() looks at ->h_curr, which is the intermediate crud. So
> even though it updates all the cgroup nonsense, it will not in fact
> update the root group, because it never actually sees rq->cfs.curr.

Exactly

>
> Bah.
>
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index dcf860c59a14..649b4f7505a1 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -7983,6 +7983,9 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> >       if (!p->se.sched_delayed || (flags & ENQUEUE_DELAYED))
> >               util_est_enqueue(cfs_rq, p);
> >
> > +     if (cfs_rq->curr)
> > +             update_curr(cfs_rq_of(cfs_rq->curr));
> > +
>
> Still, I think this wants to be in a different spot. It needs to be
> below the whole initial if(curr) place_entity() thing. Perhaps stick
> these into {en,de}queue_hierarchy() ?

But are we sure that cfs_rq->curr has been updated ? Otherwise it
means that we place cfs_rq->curr before having updated its vruntime so
avg_vruntime will not account the last running phase.

The same applies when we requeue a delayed entity.

>
>
> >       if (flags & ENQUEUE_DELAYED) {
> >               requeue_delayed_entity(cfs_rq, se);
> >               return;
> > @@ -8103,7 +8106,8 @@ static bool __dequeue_task(struct rq *rq, struct task_struct *p, int flags)
> >
> >       clear_buddies(cfs_rq, se);
> >
> > -     update_curr(cfs_rq_of(se));
> > +     if (cfs_rq->curr)
> > +             update_curr(cfs_rq_of(cfs_rq->curr));
> >       update_entity_lag(cfs_rq, se);
> >
> >       if (flags & DEQUEUE_DELAYED) {
> > --
> > 2.43.0
> >