Re: [PATCH 7/8] sched/fair: Do not compute overloaded status unnecessarily during lb

[K Prateek Nayak]

Hello Vincent,

On 12/12/2024 4:48 PM, Vincent Guittot wrote:
> On Thu, 12 Dec 2024 at 12:01, K Prateek Nayak <kprateek.nayak@xxxxxxx> wrote:
> > Hello Vincent,
> >
> > Thank you for reviewing the patch.
> >
> > On 12/12/2024 3:26 PM, Vincent Guittot wrote:
> > > On Wed, 11 Dec 2024 at 19:58, K Prateek Nayak <kprateek.nayak@xxxxxxx> wrote:
> > > > Only set sg_overloaded when computing sg_lb_stats() at the highest sched
> > > > domain since rd->overloaded status is updated only when load balancing
> > > > at the highest domain. While at it, move setting of sg_overloaded below
> > > > idle_cpu() check since an idle CPU can never be overloaded.
> > > >
> > > > Signed-off-by: K Prateek Nayak <kprateek.nayak@xxxxxxx>
> > > > ---
> > > >    kernel/sched/fair.c | 7 ++++---
> > > >    1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > > index ec2a79c8d0e7..3f36805ecdca 100644
> > > > --- a/kernel/sched/fair.c
> > > > +++ b/kernel/sched/fair.c
> > > > @@ -10358,9 +10358,6 @@ static inline void update_sg_lb_stats(struct lb_env *env,
> > > >                   nr_running = rq->nr_running;
> > > >                   sgs->sum_nr_running += nr_running;
> > > >
> > > > -               if (nr_running > 1)
> > > > -                       *sg_overloaded = 1;
> > > > -
> > > >                   if (cpu_overutilized(i))
> > > >                           *sg_overutilized = 1;
> > > >
> > > > @@ -10373,6 +10370,10 @@ static inline void update_sg_lb_stats(struct lb_env *env,
> > > >                           continue;
> > > >                   }
> > > >
> > > > +               /* Overload indicator is only updated at root domain */
> > > > +               if (!env->sd->parent && nr_running > 1)
> > >
> > > nit: may be worth checking local variable 1st which should be cheaper
> > > than env->sd->parent
> >
> > What are your thoughts on passing NULL for "sg_overloaded" when calling
> > update_sg_lb_stats() at non-root domains? Would it be equally cheap to
> > do:
> >
> >          if (sg_overloaded && nr_running > 1)
> >                  *sg_overloaded = 1;
>
> you will have to test it twice as it is also set for misfit task

Ah! True that. Local variable approach is indeed simpler.

--
Thanks and Regards,
Prateek

> > > Reviewed-by: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> >
> > Thank you.
> >
> > > > +                       *sg_overloaded = 1;
> > > > +
> > > >    #ifdef CONFIG_NUMA_BALANCING
> > > >                   /* Only fbq_classify_group() uses this to classify NUMA groups */
> > > >                   if (sd_flags & SD_NUMA) {
> > > > --
> > > > 2.34.1
> >
> > --
> > Thanks and Regards,
> > Prateek