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

[Shrikanth Hegde]

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?