Re: [RFC PATCH v3 13/13] sched/fair: Switch nohz.idle_cpus to use sbm

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] sched/fair: _nohz_idle_balance() aborts during CPU hotplug
- [Medium] sched/fair: NOHZ blocked load decay broken for NO_HZ_FULL CPUs
- [Medium] sched/fair: silent deletion of ASYM_PACKING idle balance kick
- [Low] sched/fair: missing atomic operation before smp_mb__after_atomic()
--

commit 0b0c43c6e4d9b31b812d8407b146c5dc13783dc8
Author: Peter Zijlstra <peterz@infradead.org>

    sched/fair: Switch nohz.idle_cpus to use sbm

With sbm infrastructure in place, convert the global nohz.idle_cpus cpumask
to use sparsebitmap (sbm).

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index a0a659f4c3be6..0b1458c4360e3 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -14186,24 +14186,6 @@ static void nohz_balancer_kick(struct rq *rq)
>  		}
>  	}
>  
> -	sd = rcu_dereference_all(per_cpu(sd_asym_packing, cpu));
> -	if (sd) {
> -		/*
> -		 * When ASYM_PACKING; see if there's a more preferred CPU
> -		 * currently idle; in which case, kick the ILB to move tasks
> -		 * around.
> -		 *
> -		 * 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) {
> -			if (sched_asym(sd, i, cpu)) {
> -				flags |= NOHZ_STATS_KICK | NOHZ_BALANCE_KICK;
> -				goto out;
> -			}
> -		}
> -	}
> -

[Severity: Medium]
Was it intentional to completely remove the ASYM_PACKING handling block here
without a replacement?

This appears to mean tasks will no longer be proactively migrated to favored
cores during NOHZ idle load balancing on asymmetric capacity systems.

>  	sd = rcu_dereference_all(per_cpu(sd_asym_cpucapacity, cpu));
>  	if (sd) {
>  		/*

[ ... ]

> @@ -14324,7 +14307,8 @@ void nohz_balance_enter_idle(int cpu)
>  
>  	rq->nohz_tick_stopped = 1;
>  
> -	cpumask_set_cpu(cpu, nohz.idle_cpus_mask);
> +	if (cpumask_test_cpu(rq->cpu, housekeeping_cpumask(HK_TYPE_KERNEL_NOISE)))
> +		sbm_cpu_set(nohz.sbm, rq->cpu);
>  
>  	/*
>  	 * Ensures that if nohz_idle_balance() fails to observe our
>  	 * @idle_cpus_mask store, it must observe the @has_blocked_load
>  	 * and @needs_update stores.
>  	 */
>  	smp_mb__after_atomic();

[Severity: Low]
Does this conditional atomic operation violate the smp_mb__after_atomic() API
contract when the CPU is not in the housekeeping mask?

If a CPU is not in the housekeeping mask, sbm_cpu_set() is skipped. The
subsequent smp_mb__after_atomic() then executes without a preceding atomic
operation, which may fail to provide hardware memory ordering on architectures
where it is just a compiler barrier.

[ ... ]

> @@ -14351,7 +14335,7 @@ static bool update_nohz_stats(struct rq *rq)
>  	if (!rq->has_blocked_load)
>  		return false;
>  
> -	if (!cpumask_test_cpu(cpu, nohz.idle_cpus_mask))
> +	if (!sbm_cpu_test(nohz.sbm, cpu))
>  		return false;

[Severity: Medium]
Will this prevent NO_HZ_FULL CPUs from ever decaying their blocked load when
idle?

When an isolated CPU enters idle, it is excluded from nohz.sbm due to the
housekeeping_cpumask() check in nohz_balance_enter_idle(). Consequently,
update_nohz_stats() entirely skips it because it checks sbm_cpu_test(), which
could lead to stale load averages and incorrect scheduling decisions for those
CPUs.

>  
>  	if (!time_after(jiffies, READ_ONCE(rq->last_blocked_load_update_tick)))

[ ... ]

> @@ -14405,7 +14390,10 @@ static void _nohz_idle_balance(struct rq *this_rq, unsigned int flags)
>  	 * Start with the next CPU after this_cpu so we will end with this_cpu and let a
>  	 * chance for other idle cpu to pull load.
>  	 */
> -	for_each_cpu_wrap(balance_cpu,  nohz.idle_cpus_mask, this_cpu+1) {
> +	start = sbm_cpu_to_idx(cpumask_next_wrap(this_cpu, cpu_online_mask));
> +	sbm_for_each_set_bit_wrap(nohz.sbm, idx, start) {
> +		balance_cpu = sbm_idx_to_cpu(idx);
> +

[Severity: High]
Can this iteration abort entirely during CPU hotplug?

_nohz_idle_balance() uses cpumask_next_wrap() on cpu_online_mask. During CPU
teardown, sched_cpu_deactivate() calls free_sbm_index(cpu) which sets the CPU's
SBM index to -1, but the CPU remains in cpu_online_mask until later in the
teardown process.

If the ILB samples this offlining CPU, sbm_cpu_to_idx() returns -1, which is
passed as `start` to sbm_for_each_set_bit_wrap().

Since sbm_find_next_bit_wrap() returns -1 when start is -1:

include/linux/sbm.h:sbm_find_next_bit_wrap() {
    ...
	bit = sbm_find_next_bit(sbm, 0);
	return bit < start ? bit : -1;
}

won't this cause the loop to terminate immediately without balancing any CPUs?

>  		if (!idle_cpu(balance_cpu))
>  			continue;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001192849.74788-1-kprateek.nayak@amd.com?part=13