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

On 1/6/26 8:33 AM, K Prateek Nayak wrote:
> Hello Shrikanth,
> 
> On 1/5/2026 4:09 PM, Shrikanth Hegde wrote:
>>> 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
>> +        */

cpumask_empty check.

>> +       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);
>> +
> 
> I'm slightly skeptical - find_new_ilb() will also fail to find any CPU
> if idle_cpu() returns false momentarily.
> 

Yes. I even thought to put cpumask_empty check here and then do these updates.
But since there might be a window where a remote CPU may be going idle,
might race with it and possibly lose a chance for idle balance.

Since it is extreme case of 100% busy, all these checks may not be necessary.
Will put a return instead :)

Thanks for pointing that out.

> Those CPUs can again go back to idle without updating
> "nohz.has_blocked_load" - tick_nohz_idle_stop_tick() will skip
> nohz_balance_enter_idle() if the CPU already had TS_FLAG_STOPPED set.
> 
> For those cases, we'll need to retain the "nohz" state as is until a
> suitable ILB CPU can be found - this is the reason why we delay
> clearing the "nohz" state until _nohz_idle_balance() and reconstruct it
> once the CPU is done with idle balancing.
> 
> There are also nuances like the smp_mb__after_atomic() in

That reminds me, need to upgrade this to smp_mb now, given atomic is gone.

> nohz_balance_enter_idle() which requires us to check the
> "nohz.idle_cpus_mask" after we are done clearing "nohz.needs_update"
> and "nohz.has_blocked_load".
> 
>>          return -1;
>>   }
>>
>>
>