Re: [PATCH 4/5] sched/fair: Add SIS_UTIL support to select_idle_capacity()

[Andrea Righi <arighi@nvidia.com>]

Hi Vincent and Prateek,

On Mon, Apr 27, 2026 at 10:35:59AM +0200, Vincent Guittot wrote:
> On Mon, 27 Apr 2026 at 07:13, K Prateek Nayak <kprateek.nayak@amd.com> wrote:
> >
> > Hello Vincent,
> >
> > On 4/24/2026 6:02 PM, Vincent Guittot wrote:
> > >> +       if (sched_feat(SIS_UTIL) && sd->shared) {
> > >> +               /*
> > >> +                * Increment because !--nr is the condition to stop scan.
> > >> +                *
> > >> +                * Since "sd" is "sd_llc" for target CPU dereferenced in the
> > >> +                * caller, it is safe to directly dereference "sd->shared".
> > >> +                * Topology bits always ensure it assigned for "sd_llc" and it
> > >> +                * cannot disappear as long as we have a RCU protected
> > >> +                * reference to one the associated "sd" here.
> > >> +                */
> > >> +               nr = READ_ONCE(sd->shared->nr_idle_scan) + 1;
> > >> +               /* overloaded LLC is unlikely to have idle cpu/core */
> > >> +               if (nr == 1)
> > >> +                       return -1;
> > >
> > > The comment below applies to select_idle_cpu but we want same behavior
> > > for both function
> > > If test_idle_cores is true we will not look for it whereas we don't
> > > care about nr value when test_idle_core is true in the
> > > for_each_cpu_wrap loop
> >
> > Ack but the initial "nr" based bailout in select_idle_cpu() applies even
> > when test_idle_cores() is true too right?
> 
> Yes there is an early bail out for nr == 1 even with idle core.

I did some tests changing the early bailout condition as
!prefers_idle_core && nr == 1.

On Vera I see small performance regressions with this (4-5% slowdown),
especially when approaching system saturation, which makes sense I think.

At this point I'd personally stick with the early bailout condition, considering
that it's still consistent with select_idle_cpu() and it seems to provide
slightly better results. What do you think?

> 
> >
> > >
> > >
> > >> +       }
> > >> +
> > >>         for_each_cpu_wrap(cpu, cpus, target) {
> > >>                 bool preferred_core = !prefers_idle_core || is_core_idle(cpu);
> > >>                 unsigned long cpu_cap = capacity_of(cpu);
> > >>
> > >> +               /* We have found a good enough target. Just use it. */
> > >> +               if (--nr <= 0 && best_fits == -4)
> > >> +                       return best_cpu;
> > >
> > > In select_idle_cpu(), we return immediatly when nr == 0 and
> > > test_idle_cores is false but we loop on all cpus if test_idle_cores is
> > > true until we found an idle core. In the case of
> > > select_idle_capacity(), I agree that util_fits_cpu() add another level
> > > but shouldn't we continue to loop even if we found a best_fits == -4
> >
> > I see what you mean! We can additionally guard the bailout on
> > "!prefers_idle_core".
> >
> > For the case where test_idle_cores() returns false, is stopping for a
> > UCLAMP_MIN restricted CPU alright if SIS_UTIL bailout suggests it might
> > not be fruitful to search further?
> >
> > Do you think it might trigger too many misfit balancing?
> 
> For SMT, I don't think this will make a real diff but for !SMT this
> would indeed trigger more misfits.

You mean the SIS_UTIL mechanism in general (not the extra !prefers_idle_core
guard), right? Because in the !SMT case, prefers_idle_core is always false.

Thanks,
-Andrea