Re: [PATCH 2/3] sched: Simplify ifdeffery around cpu_smt_mask

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

On 5/11/26 8:07 PM, Shrikanth Hegde wrote:
> Hi Valentin, Thanks for the reviewing this patchset.
> 
> On 5/11/26 6:23 PM, Valentin Schneider wrote:
>> On 06/05/26 16:30, Shrikanth Hegde wrote:
>>> Now, that cpu_smt_mask is defined as cpumask_of(cpu) for
>>> CONFIG_SCHED_SMT=n, it is possible to get rid of the ifdeffery.
>>>
>>> Effectively,
>>> - This makes sched_smt_present is defined always
>>>
>>> - cpumask_weight(cpumask_of(cpu)) == 1. So sched_smt_present_inc/dec
>>>    will never enable the sched_smt_present. Which is expected.
>>>
>>> - Paths that were compile-time eliminated become runtime guarded
>>>    using static keys.
>>>
>>> - Defines set_idle_cores, test_idle_cores etc which could likely benefit
>>>    the CONFIG_SCHED_SMT=n systems to use the same optimizations 
>>> within the
>>>    LLC at wakeups.
>>>
>>> - This will expose sched_smt_present,stop_core_cpuslocked symbol for
>>>    CONFIG_SCHED_SMT=n. Likely not a concern.
>>>
>>> - There a bloat of code CONFIG_SCHED_SMT=n. (NR_CPUS=2048)
>>>    add/remove: 25/18 grow/shrink: 26/19 up/down: 6696/-3064 (3632)
>>>    Total: Before=30771823, After=30775455, chg +0.01%
>>>
>>> - No code bloat for CONFIG_SCHED_SMT=y, which is expected.
>>>
>>
>> Some nitpicks below, otherwise this LGTM except the sched_ext bits which
>> I'm not familiar enough with.
> 
> sched_ext just added the ifdefs for the masks i think.
> It has sched_smt_active() already.
> 
>>
>>> @@ -8703,9 +8699,7 @@ int sched_cpu_deactivate(unsigned int cpu)
>>>         */
>>>        sched_smt_present_dec(cpu);
>>>
>>> -#ifdef CONFIG_SCHED_SMT
>>>        sched_core_cpu_deactivate(cpu);
>>> -#endif
>>
>> That ends up grabbing @core_lock, arguably this is during hotplug but 
>> still
>> seems a bit wasteful when, with CONFIG_SCHED_SMT=1, we know the mask 
>> weight
>> will never exceed 1. Probably worth adding a sched_smt_active() check
>> within the callee.
>>
> 
> Ok. Fair enough. Even cpu bringup path too could use the same opt.
> Something like below?
> ---
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 084ec3987d7c..add0fcc8ba90 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6494,6 +6494,10 @@ static void sched_core_cpu_starting(unsigned int 
> cpu)
>          struct rq *rq = cpu_rq(cpu), *core_rq = NULL;
>          int t;
> 
> +       /* No point in doing anything further if SMT is not active */
> +       if (!sched_smt_active())
> +               return;
> +
>          guard(core_lock)(&cpu);
> 
>          WARN_ON_ONCE(rq->core != rq);
> @@ -6533,6 +6537,10 @@ static void sched_core_cpu_deactivate(unsigned 
> int cpu)
>          struct rq *rq = cpu_rq(cpu), *core_rq = NULL;
>          int t;
> 
> +       /* No point in doing anything further if SMT is not active */
> +       if (!sched_smt_active())
> +               return;
> +
>          guard(core_lock)(&cpu);
> 
>          /* if we're the last man standing, nothing to do */
> 
> 

This isn't necessary. Both sched_core_cpu_starting/sched_core_cpu_deactivate bail out
quickly when cpumask_weight(cpu_smt_mask) == 1.

Given that it is not fastpath, I will skip the above diff.