Re: [PATCH 1/2] sched/fair: consider hk_mask early in triggering ilb

[Shrikanth Hegde]

Hi Mukesh.

On 3/19/26 1:45 PM, Mukesh Kumar Chaurasiya wrote:
> On Thu, Mar 19, 2026 at 12:23:13PM +0530, Shrikanth Hegde wrote:
> > Current code around nohz_balancer_kick and kick_ilb:
> > 1. Checks for nohz.idle_cpus_mask to see if idle load balance(ilb) is
> >     needed.
> > 2. Does a few checks to see if any conditions meet the criteria.
> > 3. Tries to find the idle CPU. But the idle CPU found should be part of
> >     housekeeping CPUs.
> >
> > If there is no housekeeping idle CPU, then step 2 is done
> > un-necessarily, since 3 bails out without doing the ilb.
> >
> > Fix that by making the decision early and pass it on to find_new_ilb.
> > Use a percpu cpumask instead of allocating it everytime since this is in
> > fastpath.
> >
> > If flags is set to NOHZ_STATS_KICK since the time is after nohz.next_blocked
> > but before nohz.next_balance and there are idle CPUs which are part of
> > housekeeping, need to copy the same logic there too.
> >
> > While there, fix the stale comments around nohz.nr_cpus
> >
> > Signed-off-by: Shrikanth Hegde <sshegde@xxxxxxxxxxxxx>
> > ---
> >
> > Didn't add the fixes tag since it addresses more than stale comments.
> >
> >   kernel/sched/fair.c | 45 +++++++++++++++++++++++++++++++--------------
> >   1 file changed, 31 insertions(+), 14 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index b19aeaa51ebc..02cca2c7a98d 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -7392,6 +7392,7 @@ static inline unsigned int cfs_h_nr_delayed(struct rq *rq)
> >   static DEFINE_PER_CPU(cpumask_var_t, load_balance_mask);
> >   static DEFINE_PER_CPU(cpumask_var_t, select_rq_mask);
> >   static DEFINE_PER_CPU(cpumask_var_t, should_we_balance_tmpmask);
> > +static DEFINE_PER_CPU(cpumask_var_t, kick_ilb_tmpmask);
> >   
> >   #ifdef CONFIG_NO_HZ_COMMON
> >   
> > @@ -12629,15 +12630,14 @@ static inline int on_null_domain(struct rq *rq)
> >    * - When one of the busy CPUs notices that there may be an idle rebalancing
> >    *   needed, they will kick the idle load balancer, which then does idle
> >    *   load balancing for all the idle CPUs.
> > + *
> > + *   @cpus idle CPUs in HK_TYPE_KERNEL_NOISE housekeeping
> >    */
> > -static inline int find_new_ilb(void)
> > +static inline int find_new_ilb(struct cpumask *cpus)
> >   {
> > -	const struct cpumask *hk_mask;
> >   	int ilb_cpu;
> >   
> > -	hk_mask = housekeeping_cpumask(HK_TYPE_KERNEL_NOISE);
> > -
> > -	for_each_cpu_and(ilb_cpu, nohz.idle_cpus_mask, hk_mask) {
> > +	for_each_cpu(ilb_cpu, cpus) {
> >   
> >   		if (ilb_cpu == smp_processor_id())
> >   			continue;
> > @@ -12656,7 +12656,7 @@ static inline int find_new_ilb(void)
> >    * We pick the first idle CPU in the HK_TYPE_KERNEL_NOISE housekeeping set
> >    * (if there is one).
> >    */
> > -static void kick_ilb(unsigned int flags)
> > +static void kick_ilb(unsigned int flags, struct cpumask *cpus)
> >   {
> >   	int ilb_cpu;
> >   
> > @@ -12667,7 +12667,7 @@ static void kick_ilb(unsigned int flags)
> >   	if (flags & NOHZ_BALANCE_KICK)
> >   		nohz.next_balance = jiffies+1;
> >   
> > -	ilb_cpu = find_new_ilb();
> > +	ilb_cpu = find_new_ilb(cpus);
> >   	if (ilb_cpu < 0)
> >   		return;
> >   
> > @@ -12700,6 +12700,7 @@ static void kick_ilb(unsigned int flags)
> >    */
> >   static void nohz_balancer_kick(struct rq *rq)
> >   {
> > +	struct cpumask *ilb_cpus = this_cpu_cpumask_var_ptr(kick_ilb_tmpmask);
> >   	unsigned long now = jiffies;
> >   	struct sched_domain_shared *sds;
> >   	struct sched_domain *sd;
> > @@ -12715,27 +12716,41 @@ static void nohz_balancer_kick(struct rq *rq)
> >   	 */
> >   	nohz_balance_exit_idle(rq);
> >   
> > +	/* ILB considers only HK_TYPE_KERNEL_NOISE housekeeping CPUs */
> > +
> >   	if (READ_ONCE(nohz.has_blocked_load) &&
> > -	    time_after(now, READ_ONCE(nohz.next_blocked)))
> > +	    time_after(now, READ_ONCE(nohz.next_blocked))) {
> >   		flags = NOHZ_STATS_KICK;
> > +		cpumask_and(ilb_cpus, nohz.idle_cpus_mask,
> > +			    housekeeping_cpumask(HK_TYPE_KERNEL_NOISE));
> > +	}
> >   
> >   	/*
> > -	 * Most of the time system is not 100% busy. i.e nohz.nr_cpus > 0
> > -	 * Skip the read if time is not due.
> > +	 * Most of the time system is not 100% busy. i.e there are idle
> > +	 * housekeeping CPUs.
> > +	 *
> > +	 * So, Skip the reading idle_cpus_mask if time is not due.
> >   	 *
> >   	 * If none are in tickless mode, there maybe a narrow window
> >   	 * (28 jiffies, HZ=1000) where flags maybe set and kick_ilb called.
> >   	 * But idle load balancing is not done as find_new_ilb fails.
> > -	 * That's very rare. So read nohz.nr_cpus only if time is due.
> > +	 * That's very rare. So check (idle_cpus_mask & HK_TYPE_KERNEL_NOISE)
> > +	 * only if time is due.
> > +	 *
> >   	 */
> >   	if (time_before(now, nohz.next_balance))
> >   		goto out;
> >   
> > +	/* Avoid the double computation */
> > +	if (flags != NOHZ_STATS_KICK)
> > +		cpumask_and(ilb_cpus, nohz.idle_cpus_mask,
> > +			    housekeeping_cpumask(HK_TYPE_KERNEL_NOISE));
> > +
> There is no usage of ilb_cpus till this point. We can avoid this if
> condition and get the ilb_cpus here itself instead of earlier.

No there is. Why?

struct cpumask *ilb_cpus = this_cpu_cpumask_var_ptr(kick_ilb_tmpmask) << this is just a variable.

if (READ_ONCE(nohz.has_blocked_load) && time_after(now, READ_ONCE(nohz.next_blocked)))
	flags = NOHZ_STATS_KICK

if (time_before(now, nohz.next_balance))
  	goto out;

If there are idle cpus, nohz.has_blocked_load=1 on idle entry which
could be after previous nohz idle balance. After 32 jiffies time now points
after next_blocked. But nohz.next_balance is typically set to 60 jiffies.
So, it goes to out with flags set and that passes ilb_cpus which is not set yet.
Hence both places setting the ilb_cpu is necessary.

I kept it at both places and added flags check since it is difficult to
predict movement of nohz.next_balance and nohz.next_blocked since there
multiple CPUs involved which maybe doing idle entry/exit. On first tick
after idle exit, nohz_balancer_kick would be called.

> >   	/*
> >   	 * None are in tickless mode and hence no need for NOHZ idle load
> >   	 * balancing
> >   	 */
> > -	if (unlikely(cpumask_empty(nohz.idle_cpus_mask)))
> > +	if (unlikely(cpumask_empty(ilb_cpus)))
> >   		return;
> >   
> >   	if (rq->nr_running >= 2) {
> > @@ -12767,7 +12782,7 @@ static void nohz_balancer_kick(struct rq *rq)
> >   		 * When balancing between cores, all the SMT siblings of the
> >   		 * preferred CPU must be idle.
> >   		 */
> > -		for_each_cpu_and(i, sched_domain_span(sd), nohz.idle_cpus_mask) {
> > +		for_each_cpu_and(i, sched_domain_span(sd), ilb_cpus) {
> >   			if (sched_asym(sd, i, cpu)) {
> >   				flags = NOHZ_STATS_KICK | NOHZ_BALANCE_KICK;
> >   				goto unlock;
> > @@ -12820,7 +12835,7 @@ static void nohz_balancer_kick(struct rq *rq)
> >   		flags |= NOHZ_NEXT_KICK;
> >   
> >   	if (flags)
> > -		kick_ilb(flags);
> > +		kick_ilb(flags, ilb_cpus);
> >   }
> >   
> >   static void set_cpu_sd_state_busy(int cpu)
> > @@ -14253,6 +14268,8 @@ __init void init_sched_fair_class(void)
> >   		zalloc_cpumask_var_node(&per_cpu(select_rq_mask,    i), GFP_KERNEL, cpu_to_node(i));
> >   		zalloc_cpumask_var_node(&per_cpu(should_we_balance_tmpmask, i),
> >   					GFP_KERNEL, cpu_to_node(i));
> > +		zalloc_cpumask_var_node(&per_cpu(kick_ilb_tmpmask, i),
> > +					GFP_KERNEL, cpu_to_node(i));
> >   
> >   #ifdef CONFIG_CFS_BANDWIDTH
> >   		INIT_CSD(&cpu_rq(i)->cfsb_csd, __cfsb_csd_unthrottle, cpu_rq(i));
> > --
> > 2.43.0
> Rest LGTM

Thank you for going through the patch.