[PATCH] sched/fair: Fix flat hierarchy

[PATCH] sched/fair: Fix flat hierarchy

[Vincent Guittot]

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.

Fixes: 85570f10a4c6 ("sched/eevdf: Move to a single runqueue")
Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
---

I overlooked the cgroup part when I reviewed the update_entity_lag() in
flat hierarchy.

 kernel/sched/fair.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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));
+
 	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

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

[Peter Zijlstra]

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.

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() ?


>  	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
> 

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

[Vincent Guittot]

On Wed, 12 Aug 2026 at 16:03, Peter Zijlstra <peterz@infradead.org> 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
> >