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

[Shrikanth Hegde]

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.