Re: [PATCH 3/3] sched/fair: Do not raise overutilized for idle CPUs

[Vincent Guittot]

On Mon, 10 Jan 2022 at 17:40, Vincent Donnefort
<vincent.donnefort@xxxxxxx> wrote:
>
> On Wed, Dec 22, 2021 at 09:20:17AM +0100, Vincent Guittot wrote:
> > On Mon, 20 Dec 2021 at 12:43, Vincent Donnefort
> > <vincent.donnefort@xxxxxxx> wrote:
> > >
> > > During a migration, the lock for the previous runqueue is not taken and
> > > hence, the task contribution isn't directly removed from that runqueue
> > > utilization but instead temporarily saved, until the next PELT signals
> > > update where it would be accounted. There is then a window in which a
> > > CPU can ben idle be nonetheless overutilized.
> > >
> > > The load balancer wouldn't be able to do anything to help a sleeping CPU,
> > > it brings then no gain to raise overutilized there, only the risk of
> > > spuriously doing it.
> >
> > But how do you make the difference between a very short idle time of
> > an overutilized CPU and a idle cpu with outdated utilization
>
> No distinction here, but if the CPU is idle there's nothing to pull, so the load
> balance wouldn't do anything with this information.

The load balance has never done anything with this information. This
information is only used to disable LB for EAS and as mentioned below,
being idle is not a good reason for not being overutilized.

Also this patch seems to be there just to fix a problem created by the
previous one which triggers the costly new idle load balance without
good reason

>
> >
> > Being idle is not a good reason for not being overutilized (ie ~80% of
> > average utilisation)
> >
> > >
> > > Signed-off-by: Vincent Donnefort <vincent.donnefort@xxxxxxx>
> > >
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 51f6f55abb37..37f737c5f0b8 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -8641,26 +8641,28 @@ 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_status |= SG_OVERLOAD;
> > > -
> > > -               if (cpu_overutilized(i))
> > > -                       *sg_status |= SG_OVERUTILIZED;
> > > -
> > >  #ifdef CONFIG_NUMA_BALANCING
> > >                 sgs->nr_numa_running += rq->nr_numa_running;
> > >                 sgs->nr_preferred_running += rq->nr_preferred_running;
> > >  #endif
> > > +               if (nr_running > 1)
> > > +                       *sg_status |= SG_OVERLOAD;
> >
> > Why do you move this code related to overload ?
>
> This was a cosmetic change to put the NUMA related stats next to the other ones.

Please don't add unrelated cosmetic changes in a patch

>
> [...]