Re: [PATCH v5] sched/fair: Prefer fully idle cores for NOHZ balancing

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

Hi Andrea.

On 8/10/26 3:11 PM, Shrikanth Hegde wrote:
> 
> 
> On 8/6/26 7:13 PM, Andrea Righi wrote:
>> find_new_ilb() selects the first idle housekeeping CPU without
>> considering whether another thread is running on the same physical core.
>> On an SMT system, the idle load balancer can therefore activate both
>> siblings even when another housekeeping CPU has an entirely idle core.
>>
>> On most SMT systems, this is not problematic because the idle load
>> balancer is a short-lived activity and the transient wakeup of a sibling
>> has negligible performance impact.
>>
>> However, this can be particularly costly on NVIDIA Olympus cores used in
>> Vera. Briefly activating an otherwise idle sibling can reduce the
>> performance available to the other sibling and this effect does not
>> necessarily end once the activated sibling becomes idle: after the ILB
>> finishes and its CPU enters WFI, full single-thread performance is
>> restored only after the sibling has remained idle for a qualification
>> interval (10 Ki cycles on the tested Vera system). Repeated short
>> sibling wakeups can therefore sustain the interference even with little
>> actual overlap.
>>
>> Prevent this by preferring an idle housekeeping CPU whose entire SMT
>> core is idle. Retain the first idle CPU as a fallback when no fully idle
>> core is available, so NOHZ balancing continues to make forward progress.
>> Once a partially busy core has been examined, skip its remaining SMT
>> siblings to avoid repeating the core-idle check on wide SMT systems.
>>
>> Tests performed using an ad hoc GEMM benchmark running one CPU-intensive
>> task per SMT core within its CPU affinity mask improved from
>> approximately 6.2 TFLOP/s to 9.4 TFLOP/s.
>>
>> Note that this preference may wake a fully idle physical core instead of
>> using an idle sibling of an active core, potentially increasing ILB
>> wakeup latency or energy consumption on some architectures. It may also
>> scan additional CPUs before selecting the one to run the ILB. The
>> selection falls back to the first idle CPU when no fully idle SMT core
>> is available. Non-SMT systems continue to select the first idle
>> housekeeping CPU.
>>
>> Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>
>> Reviewed-by: K Prateek Nayak <kprateek.nayak@amd.com>
>> Reviewed-by: Mete Durlu <meted@linux.ibm.com>
>> Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
>> Reviewed-by: Shrikanth Hegde <sshegde@linux.ibm.com>
>> Signed-off-by: Andrea Righi <arighi@nvidia.com>
>> ---
>> Changes in v5:
>>   - Collect Tested-by and Reviewed-by tags
>>   - Reorder local variable declarations (Prateek Nayak)
>>   - Link to v4: https://lore.kernel.org/all/20260804151324.918020-1- 
>> arighi@nvidia.com/
>>
>> Changes in v4:
>>   - Remove redundant this_cpu check (Prateek Nayak, Vincent Guittot)
>>   - Link to v3: https://lore.kernel.org/all/20260731191957.3199642-1- 
>> arighi@nvidia.com/
>>
>> Changes in v3:
>>   - After finding an idle fallback, skip all siblings when a busy CPU is
>>     encountered, avoiding per-CPU traversal of known-busy cores (Mete 
>> Durlu)
>>   - Link to v2: https://lore.kernel.org/all/20260729163225.1987068-1- 
>> arighi@nvidia.com/
>>
>> Changes in v2:
>>   - Avoid repeated is_core_idle() checks on wide SMT systems by pruning
>>     the remaining siblings of a partially busy core (Prateek Nayak)
>>   - Link to v1: https://lore.kernel.org/r/20260728214442.1648483-1- 
>> arighi@nvidia.com/
>>
>>   kernel/sched/fair.c | 55 ++++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 44 insertions(+), 11 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 37001c63452e5..89bec68622db9 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -13964,29 +13964,62 @@ static inline int on_null_domain(struct rq *rq)
>>    */
>>   static inline int find_new_ilb(void)
>>   {
>> -    int this_cpu = smp_processor_id();
>> -    const struct cpumask *hk_mask;
>> -    int ilb_cpu;
>> +    int ilb_cpu, fallback = -1;
>> +    struct cpumask *ilb_cpus;
>> +
>> +    lockdep_assert_irqs_disabled();
>> +
>> +    /*
>> +     * Reuse the per-CPU select_rq_mask, which is protected from 
>> concurrent
>> +     * use on this CPU by having interrupts disabled.
>> +     */
>> +    ilb_cpus = this_cpu_cpumask_var_ptr(select_rq_mask);
>> +    cpumask_and(ilb_cpus, nohz.idle_cpus_mask,
>> +            housekeeping_cpumask(HK_TYPE_KERNEL_NOISE));
>> +
>> +    for_each_cpu(ilb_cpu, ilb_cpus) {
>> +        if (!idle_cpu(ilb_cpu)) {
>> +            /*
>> +             * Once an idle fallback exists, a busy CPU proves that
>> +             * this core cannot be fully idle. Skip its siblings.
>> +             */
>> +            if (sched_smt_active() && fallback >= 0)
>> +                cpumask_andnot(ilb_cpus, ilb_cpus, 
>> cpu_smt_mask(ilb_cpu));
>> +            continue;
>> +        }
>> -    hk_mask = housekeeping_cpumask(HK_TYPE_KERNEL_NOISE);
>> +        /*
>> +         * Running the idle load balancer on an idle sibling of a busy
>> +         * SMT core can reduce the capacity available to its sibling. 
>> Prefer
>> +         * a CPU whose entire core is idle, but retain the first idle 
>> CPU as
>> +         * a fallback so idle balancing can still make progress when 
>> no fully
>> +         * idle core exists.
>> +         */
>> +        if (sched_smt_active() && !is_core_idle(ilb_cpu)) {
> 
> nit:
> 
> Now, when making the changes for paravirt series,
> i remembered sched_smt_active() is not needed anymore.
> since cpu_smt_mask in that case just points to just that cpu.

I guess it is good to have since it skips the cpumask_and which might save few cycles.
Also, I see peter has pulled in v4. So you can ignore this comment.

Sorry for the noise.

> 
>> +            if (fallback < 0)
>> +                fallback = ilb_cpu;
>> -    for_each_cpu_and(ilb_cpu, nohz.idle_cpus_mask, hk_mask) {
>> -        if (ilb_cpu == this_cpu)
>> +            /*
>> +             * The core is not idle, so there is no need to check
>> +             * any of its other SMT siblings.
>> +             */
>> +            cpumask_andnot(ilb_cpus, ilb_cpus,
>> +                       cpu_smt_mask(ilb_cpu));
>>               continue;
>> +        }
>> -        if (idle_cpu(ilb_cpu))
>> -            return ilb_cpu;
>> +        return ilb_cpu;
>>       }
>> -    return -1;
>> +    return fallback;
>>   }
>>   /*
>>    * Kick a CPU to do the NOHZ balancing, if it is time for it, via a 
>> cross-CPU
>>    * SMP function call (IPI).
>>    *
>> - * We pick the first idle CPU in the HK_TYPE_KERNEL_NOISE 
>> housekeeping set
>> - * (if there is one).
>> + * Prefer a CPU on a fully idle core in the HK_TYPE_KERNEL_NOISE 
>> housekeeping
>> + * set. Fall back to the first idle CPU when no fully idle core exists.
>>    */
>>   static void kick_ilb(unsigned int flags)
>>   {
>