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

[Vincent Guittot]

On Thu, 13 Aug 2026 at 12:31, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Wed, Aug 12, 2026 at 04:21:14PM +0200, Vincent Guittot wrote:
> > 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.
>
> Well, it is the same place the update was previously, no? Also, that XXX
> comment thing is about enqueueing self, with the thinking that you
> should not shift time since dequeue, allowing 'atomic' dequeue+enqueue.

I was looking at which use case could trigger  such situation.

But doesn't the below need the update
ttwu_runnable
  update_rq_clock
  p->is_blocked is true and p->se.sched_delayed is true -->
enqueue_task(rq, p, ENQUEUE_NOCLOCK | ENQUEUE_DELAYED);

>
> However, looking at things now, I can't actually see how this could
> happen in the new code. Putting a trace_printk() in also doesn't seem to
> help much.

I'm also running some tests w/ and w/o this patch and checking if
rq->cfs->curr->exec_start != rq_clock_task(rq) when we place an entity
or compute a lag.

>
> So perhaps there is some dead code there to clean up as well. But for
> consistencies sake I think placing it near {en,de}queue_hierarchy()
> where the other update_curr() lived/lives makes most sense.

I will run some test with the check above and the update_curr at the
beg of {en,de}queue_hierarchy()

>
> Hmm?