Messages in this thread | | | From | Vincent Guittot <> | Date | Tue, 11 May 2021 19:56:23 +0200 | Subject | Re: [PATCH] sched/fair: Rate limit calls to update_blocked_averages() for NOHZ |
| |
On Tue, 11 May 2021 at 19:25, Tim Chen <tim.c.chen@linux.intel.com> wrote: > > > > On 5/11/21 8:25 AM, Vincent Guittot wrote: > > Hi Tim, > > > > Sometimes, we want to set this_rq->next_balance backward compared to > > its current value. When a CPU is busy, the balance interval is > > multiplied by busy_factor which is set to 16 by default. On SMT2 sched > > domain level, it means that the interval will be 32ms when busy > > instead of 2ms. But if a busy load balance happens just before > > becoming idle, the this_rq->next_balance will be set 32ms later > > whereas it should go down to 2ms as the CPU is now idle. And this > > becomes even worse when you have 128 CPUs at die sched_domain level > > because the idle CPU will have to wait 2048ms instead of the correct > > 128ms interval. > > > >> > >> out: > >> /* Move the next balance forward */ > >> - if (time_after(this_rq->next_balance, next_balance)) > >> + if (time_after(next_balance, this_rq->next_balance)) > > > > The current comparison is correct but next_balance should not be in the past. > > I understand then the intention is after the update, > this_rq->next_balance should have a minimum value of jiffies+1. So > we will need > > out: > /* Move the next balance forward */ > + this_rq->next_balance = max(jiffies+1, this_rq->next_balance); > if (time_after(this_rq->next_balance, next_balance)) > this_rq->next_balance = next_balance; > > as next_balance computed will be at least jiffies+1 after your fix to > update_next_balance(). We still need to take care of the case when > this_rq->next_balance <= jiffies. > > So combining with your suggestion on the fix to update_next_balance(), > the fix will be > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > index 1d75af1ecfb4..2dc471c1511c 100644 > --- a/kernel/sched/fair.c > +++ b/kernel/sched/fair.c > @@ -9901,7 +9901,7 @@ update_next_balance(struct sched_domain *sd, unsigned long *next_balance) > > /* used by idle balance, so cpu_busy = 0 */ > interval = get_sd_balance_interval(sd, 0); > - next = sd->last_balance + interval; > + next = max(jiffies+1, sd->last_balance + interval); > > if (time_after(*next_balance, next)) > *next_balance = next; > @@ -10681,6 +10681,7 @@ static int newidle_balance(struct rq *this_rq, struct rq_flags *rf) > > out: > /* Move the next balance forward */ > + this_rq->next_balance = max(jiffies+1, this_rq->next_balance); > if (time_after(this_rq->next_balance, next_balance)) > this_rq->next_balance = next_balance; > > > > > > update_next_balance() is only used in newidle_balance() so we could > > modify it to have: > > > > next = max(jiffies+1, next = sd->last_balance + interval) > > Is the extra assignment "next = sd->last_balance + interval" needed?
No it's a typo mistake while copy pasting the line
> This seems more straight forward: > > next = max(jiffies+1, sd->last_balance + interval) > > I will try to get the benchmark folks to do another run with this change. > Hopefully I'll get some bandwidth from them soon. > > Thanks. > > Tim >
| |