Re: [PATCH v2 2/3] sched/fair: Change likelyhood of nohz.nr_cpus and do stats update if its due

[Shrikanth Hegde]

Hi Prateek,

> On 1/5/2026 10:37 AM, Shrikanth Hegde wrote:
> > > > --- a/kernel/sched/fair.c
> > > > +++ b/kernel/sched/fair.c
> > > > @@ -12456,10 +12456,10 @@ static void nohz_balancer_kick(struct rq *rq)
> > > >          /*
> > > >         * None are in tickless mode and hence no need for NOHZ idle load
> > > > -     * balancing:
> > > > +     * balancing, do stats update if its due
> > > >         */
> > > > -    if (likely(!atomic_read(&nohz.nr_cpus)))
> > > > -        return;
> > > > +    if (unlikely(!atomic_read(&nohz.nr_cpus)))
> > > > +        goto out;
> >
> > Did something got edited here?
>
> Welp! Stray edit. My bad. Reverted back to original diff.
>
> > > Since we are sure that "nohz.nr_cpus" is 0, there is a good chance
> > > find_new_ilb() in kick_ilb() will not find any CPU to run balance on, so
> > > why not just retain that return?
> > >
> > > The "flags" can only be set to (NOHZ_NEXT_KICK | NOHZ_STATS_KICK) on
> > > this path and kick_ilb() will simply return early without updating
> > > "nohz.next_balance" when it doesn't see NOHZ_BALANCE_KICK and fails to
> > > find any CPU. Might as well keep the early return.
> >
> > The only reason why flags would be set is, if nohz.has_blocked_load
> > is set and time is after next_blocked. In that case, doing a stats
> > based balance will make nohz.has_blocked_load=0 and subsequent invocations
> > flags =0 and no load balance will happen if nr_cpus stays 0.
> >
> > However, if we just, has_blocked_load might remains stale value.
> >
> > Isn't that the case?
>
> So cumulatively, including Patch 3, we do:
>
>      flags = 0;
>
>      if (READ_ONCE(nohz.has_blocked_load) && ...)
>          flags = NOHZ_STATS_KICK;
>
>      if (time_before(now, nohz.next_balance))
>          goto out; /* Checks nohz.idle_cpus_mask in find_new_ilb() ... (1) */
>
>      if (unlikely(cpumask_empty(nohz.idle_cpus_mask)))
>          goto out; /* Still goes to kick_ilb()                     ... (2) */
>
>      ...
>
> out:
>      if (READ_ONCE(nohz.needs_update))
>          flags |= NOHZ_NEXT_KICK;
>
>      /* assume either NOHZ_STATS_KICK or NOHZ_NEXT_KICK is set */
>      kick_ilb()
>      {
>           if (flags & NOHZ_BALANCE_KICK) /* Not possible */
>               ...
>
>           ilb_cpu = find_new_ilb(); /* Find CPU in nohz.idle_cpus_mask */
>
>
> If we arrive here from (2), we know "nohz.idle_cpus_mask" was empty a
> while back and we've not updated any global "nohz" state. If we don't
> find an ilb_cpu, we just do:
>
>          if (ilb_cpu < 0)
>              return;
>
> So why not simply return from (2)?

I see, kick_ilb though called will not do a balance since ilb_cpu was not found.

I don't want to have that return in between the two out's.

How about we do below? When there are no idle CPUs left, both has_blocked_load
and needs_update should be reset. no?

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 805b53d9709e..fa0e6065bc9c 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -12377,6 +12377,15 @@ static inline int find_new_ilb(void)
                        return ilb_cpu;
        }
 
+       /* There is no idle CPU left.
+        * reset has_blocked_load and needs_update, such that unless
+        * some CPU enters idle state, it will not trigger kick_ilb
+        */
+       if (READ_ONCE(nohz.has_blocked_load))
+               WRITE_ONCE(nohz.has_blocked_load, 0);
+       if (READ_ONCE(nohz.needs_update))
+               WRITE_ONCE(nohz.needs_update, 0);
+
        return -1;
 }