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

[Vincent Guittot]

On Thu, 13 Aug 2026 at 12:49, Vincent Guittot
<vincent.guittot@xxxxxxxxxx> wrote:
>
> 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()

For the enqueue, the 1st requeue_delayed_entity() which is the main
path for delayed task, requires the update. Then another one before
reweight_eevdf(cfs_rq, se, weight, false); in the !curr case.

But I'm not sure we want to spread this in different places.

For the dequeue, update_entity_lag() requires to call the update 1st

I also wanted to use the helper below:

+/* Update curr's vruntime before placing entity or updating lag */
+static inline update_curr_eevdf(struct cfs_rq *cfs_rq)
+{
+       if (!cfs_rq->curr)
+               return;
+
+       update_curr(cfs_rq_of(cfs_rq->curr));
+}

 >
> >
> > Hmm?