Messages in this thread | | | Date | Mon, 17 Jan 2022 12:18:19 +0000 | From | Vincent Donnefort <> | Subject | Re: [PATCH 3/3] sched/fair: Do not raise overutilized for idle CPUs |
| |
On Mon, Jan 17, 2022 at 11:45:33AM +0100, Vincent Guittot wrote: > On Mon, 10 Jan 2022 at 17:40, Vincent Donnefort > <vincent.donnefort@arm.com> 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@arm.com> 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.
But what would then be the point of running the load balancer and waste time there?
We could alternatively keep OU (for the sack of signal continuity) and bail-out earlier if we know nothing can be done (i.e. OU but idle)?
But still that doesn't solve that EAS can stay disabled for a moment (until the util_avg is properly decayed) and we would waste energy for that duration (which might not represent a lot of energy, I agree).
> > 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
Not related. Even without (sched/fair: Fix newidle_balance() for overutilized systems) the load balancer would run while it is not necessary.
Anyway, it was an attempt to maximize the time where EAS is enabled to save energy. If you think it is too risky to bring potential OU discontinuities, I'll drop that idea.
> > > > > > > > > Being idle is not a good reason for not being overutilized (ie ~80% of > > > average utilisation) > > > > > > > > > > > Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com> > > > > > > > > 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
My bad, I understood the policy was to make cosmetic changes only alongside "useful" content.
> > > > > [...]
| |