Re: [PATCH v2 2/2] sched/fair: avoid recalculating curr status in place_entity() and requeue_delayed_entity()

[Vincent Guittot]

On Wed, 26 Aug 2026 at 12:18, Kayra Cizmeci <kayracizmeci@xxxxxxxxx> wrote:
>
> In enqueue_task_fair() a bool is calculated by cfs_rq->curr == se.
> But this information gets recalculated on requeue_delayed_entity() and
> requeue_delayed_entity() only gets called in enqueue_task_fair().
> And on place_entity() if se == curr we call the avg_vruntime_weight()
> twice with the same input. place_entity() only gets called in
> enqueue_task_fair() and requeue_delayed_entity().
>
> Use the information on enqueue_task_fair() in requeue_delayed_entity().
> And place_entity() to avoid calling avg_vruntime_weight() twice.
>
> Signed-off-by: Kayra Cizmeci <kayracizmeci@xxxxxxxxx>
> ---
> The patch needs curr to be invariant so I added the same
> (cfs_rq->curr == se) to places in requeue_delayed_entity() and
> and enqueue_task_fair(). Like after the place_entity() call in
> requeue_delayed_entity(), or like before if (curr) the old place
> of the calculation. And added WARN_ON_ONCE(curr != new_calc_curr)
> or something like that I don't know how to say it normally.
>
> Then I booted these changes on x86 (Zen 3)
> called perf bench sched messaging with 200 groups and 5000 loops.
> And then I make sure if the requeue_delayed_entity() was really working
> with ftrace. The results were good but I left the computer on 2 more
> hours and then checked the results again. It was still good, but
> considering that I booted the kernel with busybox I don't think
> much happened on that 2 hour window.
>
>  kernel/sched/fair.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index b411384125ec..f1b46fe26d12 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6175,7 +6175,7 @@ void __setparam_fair(struct task_struct *p, const struct sched_attr *attr)
>  }
>
>  static void
> -place_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
> +place_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags, bool is_curr)
>  {
>         u64 vslice, vruntime = avg_vruntime(cfs_rq);
>         unsigned int nr_queued = cfs_rq->h_nr_queued;
> @@ -6199,7 +6199,7 @@ place_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
>          */
>         if (sched_feat(PLACE_LAG) && nr_queued && se->vlag) {
>                 struct sched_entity *curr = cfs_rq->curr;
> -               long load, weight;
> +               long load, weight, curr_weight;
>
>                 lag = se->vlag;
>
> @@ -6256,10 +6256,17 @@ place_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
>                  *   vl_i = (W + w_i)*vl'_i / W
>                  */
>                 load = cfs_rq->sum_weight;
> -               if (curr && curr->on_rq)
> -                       load += avg_vruntime_weight(cfs_rq, curr->h_load.weight);
> +               if (curr && (curr->on_rq || is_curr)) {
> +                       curr_weight = avg_vruntime_weight(cfs_rq, curr->h_load.weight);
> +                       if (curr->on_rq)
> +                               load += curr_weight;
> +               }
> +
> +               if (is_curr)
> +                       weight = curr_weight;
> +               else
> +                       weight = avg_vruntime_weight(cfs_rq, se->h_load.weight);

This makes the code less readable and I don't think it gives any
measurable benefit.

>
> -               weight = avg_vruntime_weight(cfs_rq, se->h_load.weight);
>                 lag *= load + weight;
>                 if (WARN_ON_ONCE(!load))
>                         load = 1;
> @@ -7900,7 +7907,7 @@ static int choose_idle_cpu(int cpu, struct task_struct *p)
>  }
>
>  static void
> -requeue_delayed_entity(struct cfs_rq *cfs_rq, struct sched_entity *se)
> +requeue_delayed_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, bool curr)
>  {
>         /*
>          * se->sched_delayed should imply: se->on_rq == 1.
> @@ -7912,10 +7919,10 @@ requeue_delayed_entity(struct cfs_rq *cfs_rq, struct sched_entity *se)
>
>         if (update_entity_lag(cfs_rq, se)) {
>                 cfs_rq->h_nr_queued--;
> -               if (se != cfs_rq->curr)
> +               if (!curr)
>                         __dequeue_entity(cfs_rq, se);
> -               place_entity(cfs_rq, se, 0);
> -               if (se != cfs_rq->curr)
> +               place_entity(cfs_rq, se, 0, curr);
> +               if (!curr)
>                         __enqueue_entity(cfs_rq, se);
>                 cfs_rq->h_nr_queued++;
>         }
> @@ -8000,9 +8007,10 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>                 util_est_enqueue(cfs_rq, p);
>
>         update_curr_eevdf(cfs_rq);
> +       curr = (cfs_rq->curr == se);
>
>         if (delayed) {
> -               requeue_delayed_entity(cfs_rq, se);
> +               requeue_delayed_entity(cfs_rq, se, curr);
>                 return;
>         }
>
> @@ -8017,18 +8025,18 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>         /*
>          * XXX comment on the curr thing
>          */
> -       curr = (cfs_rq->curr == se);
> +
>         if (curr)
> -               place_entity(cfs_rq, se, flags);
> +               place_entity(cfs_rq, se, flags, curr);
>
>         if (se->on_rq && se->sched_delayed)
> -               requeue_delayed_entity(cfs_rq, se);
> +               requeue_delayed_entity(cfs_rq, se, curr);
>
>         weight = enqueue_hierarchy(p, flags);
>
>         if (!curr) {
>                 reweight_eevdf(cfs_rq, se, weight, false);
> -               place_entity(cfs_rq, se, flags | ENQUEUE_QUEUED);
> +               place_entity(cfs_rq, se, flags | ENQUEUE_QUEUED, curr);
>                 __enqueue_entity(cfs_rq, se);
>         }
>
> --
> 2.53.0
>