Re: [PATCH] sched/fair: use rq_clock_task() in update_tg_load_avg() rate-limit

[Vincent Guittot]

On Wed, 27 May 2026 at 09:37, Vincent Guittot
<vincent.guittot@xxxxxxxxxx> wrote:
>
> On Wed, 27 May 2026 at 03:32, Rik van Riel <riel@xxxxxxxxxxx> wrote:
> >
> > update_tg_load_avg() is called once per leaf cfs_rq from the
> > __update_blocked_fair() walk that runs inside the NOHZ idle-balance
> > softirq, and again from update_load_avg() with UPDATE_TG.  Its first
> > operation after the trivial early-outs is unconditionally:
> >
> >         now = sched_clock_cpu(cpu_of(rq_of(cfs_rq)));
> >         if (now - cfs_rq->last_update_tg_load_avg < NSEC_PER_MSEC)
> >                 return;
> >
> > Jakub ran into a system where nohz_idle_balance() was taking 75%
> > of a CPU (which is handling network traffic and doing many irq_exit_cpu
> > calls), with 35% of that CPU spent in update_load_avg, and 17% of the
> > CPU in sched_clock_cpu(), reading the TSC.
>
> Do you have figures showing the improvement after applying this patch
> compared to the figures above?
>
> >
> > There are some optimizations upstream already to reduce that overhead,
> > but it also looks like those rdtsc calls may not me necessary at all,
>
> s/me/be/
>
> > giving another easy win.
> >
> > Switch the rate-limit to read rq_clock_task(rq_of(cfs_rq)) instead.
> > This does two things:
> >
> > 1. Eliminates the rdtsc.  rq->clock_task is already updated by the
> >    enclosing update_rq_clock(rq), sits in a hot cacheline, and reads
> >    as a single load.
>
> AFAICT this is the primary target of the patch because other clock
> sources don't have such overhead
>
> >
> > 2. Aligns the rate-limit clock with the clock the rate-limited data
> >    is computed on.  cfs_rq->avg.load_avg (the value being published
> >    to tg->load_avg) is computed by
> >      update_cfs_rq_load_avg(cfs_rq_clock_pelt(cfs_rq), cfs_rq)
> >    where cfs_rq_clock_pelt() is derived from rq->clock_pelt, which is
> >    derived from rq->clock_task.  PELT intentionally excludes IRQ-handling
> >    time and steal time from its decay (commit 23127296889f
> >    "sched/fair: Update scale invariance of PELT"), so
> >    cfs_rq->avg.load_avg already evolves in clock_task time.  The
> >    rate-limit was the only outlier in the propagation chain using a
> >    different clock (wall time, via sched_clock_cpu).
>
> Migrating a task is another way to update cfs_rq->avg.load_avg and
> this is not related to how fast rq->clock_task moves forward
>
> >
> >    Under normal load and on idle CPUs (where clock_task advances at
> >    wall-clock rate) behaviour is unchanged.  Under heavy IRQ load
> >    clock_task advances slower than wall time, so the rate-limit fires
> >    less often -- consistent with the fact that the underlying
> >    cfs_rq->avg.load_avg is also changing slower under the same
> >    conditions.  The publish cadence tracks the signal.
>
> As mentioned above, cfs_rq->avg.load_avg can change outside the normal
> PELT tracking.
> It would be good to check that we do not end up in the opposite
> situation where the tg->load_avg becomes stalled because
> rq->clock_task doesn't advance, even though load_avg has changed
> significantly.
>
> So before moving to this new clock, we must ensure it will not trigger
> other issues.
>
> >
> > Note: update_tg_load_avg() propagates the *already-decayed*
> > cfs_rq->avg.load_avg to tg->load_avg; it does not drive decay.  Decay
> > happens in update_cfs_rq_load_avg() on the PELT clock regardless of
> > what clock the rate-limit uses, so this change cannot lose decay
> > information.  The rate-limit governs how often we publish, not how
> > fast load decays.
> >
> > All callers of update_tg_load_avg() and clear_tg_load_avg() hold
> > rq->lock and have called update_rq_clock(rq) within microseconds:
> >
> >   caller                                   pre-state
> >   __update_blocked_fair                    encloser did update_rq_clock(rq)
> >   update_load_avg's three UPDATE_TG sites  under rq->lock after enqueue/dequeue/update_curr
> >   attach_/detach_entity_cfs_rq             preceded by update_load_avg(...)
> >   clear_tg_load_avg via offline path       rq_clock_start_loop_update(rq) upfront
> >
> > so rq->clock_task is fresh at every call.  Since cfs_rqs are per-CPU
> > per-task_group, cfs_rq->last_update_tg_load_avg is always compared
> > against the same rq's clock; no cross-rq drift.
> >
> > The same hoisting pattern was recently applied to find_new_ilb() in
> > commit 76504bce4ee6 ("sched/fair: Get this cpu once in find_new_ilb()").
>
> I'm not sure how I should link this patch to the one mentioned above
>
> >
> > Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
> > Assisted-by: Claude (Anthropic)
> > ---
> >  kernel/sched/fair.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 863de57a8a2c..096bcb00fa62 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4429,8 +4429,15 @@ static inline void update_tg_load_avg(struct cfs_rq *cfs_rq)
> >         /*
> >          * For migration heavy workloads, access to tg->load_avg can be
> >          * unbound. Limit the update rate to at most once per ms.
> > -        */
> > -       now = sched_clock_cpu(cpu_of(rq_of(cfs_rq)));
> > +        *
> > +        * The enclosing PELT update paths always hold rq->lock and have
> > +        * called update_rq_clock(rq) within microseconds, so rq->clock_task
> > +        * is fresh.  Use it instead of sched_clock_cpu() to avoid an rdtsc
> > +        * (plus pipeline serialisation) per call -- this function is invoked
> > +        * once per leaf cfs_rq in __update_blocked_fair(), so on hosts with
> > +        * many cgroups the rdtsc cost dominates the rate-limit check itself.
> > +        */
> > +       now = rq_clock_task(rq_of(cfs_rq));

Why not using rq_clock()  ? This removes rdtsc call and still move
forward whatever the irq pressure

> >         if (now - cfs_rq->last_update_tg_load_avg < NSEC_PER_MSEC)
> >                 return;
> >
> > @@ -4453,7 +4460,8 @@ static inline void clear_tg_load_avg(struct cfs_rq *cfs_rq)
> >         if (cfs_rq->tg == &root_task_group)
> >                 return;
> >
> > -       now = sched_clock_cpu(cpu_of(rq_of(cfs_rq)));
> > +       /* See update_tg_load_avg() for the rq_clock_task() rationale. */
> > +       now = rq_clock_task(rq_of(cfs_rq));
> >         delta = 0 - cfs_rq->tg_load_avg_contrib;
> >         atomic_long_add(delta, &cfs_rq->tg->load_avg);
> >         cfs_rq->tg_load_avg_contrib = 0;
> > --
> > 2.53.0-Meta
> >