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

[Shrikanth Hegde <sshegde@linux.ibm.com>]

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