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

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

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 */


> 
>> @@ -632,7 +632,6 @@ int stop_machine(cpu_stop_fn_t fn, void *data, const struct cpumask *cpus)
>>   }
>>   EXPORT_SYMBOL_GPL(stop_machine);
>>
>> -#ifdef CONFIG_SCHED_SMT
>>   int stop_core_cpuslocked(unsigned int cpu, cpu_stop_fn_t fn, void *data)
> 
> That seems to be only used by the INTEL_IFS selftest stuff which does some
> wait_for_sibling_cpu() loop; at a quick glance it seems to do the right
> thing for weight := 1 but IMO worth a proper look. That or have the IFS
> code not run that when there is no SMT.
> 

Right. intel_ifs is the only user.
INTEL_IFS depends on SMP. SMP select CONFIG_SMT on x86.

Symbol: INTEL_IFS [=n]
Depends on: X86_PLATFORM_DEVICES [=y] && X86 [=y] && CPU_SUP_INTEL [=y] && 64BIT [=y] && SMP [=y]

So, maybe leave this as is, to avoid the code bloat for CONFIG_SCHED_SMT=n as there is no user?
Maybe i will add a comment about it.

from ./bloat-o-meter. it adds about 260
stop_core_cpuslocked                           -     260    +260

Does that make sense?