Messages in this thread Patch in this message | | | Subject | Re: [PATCH] sched/fair: Rate limit calls to update_blocked_averages() for NOHZ | From | Tim Chen <> | Date | Tue, 11 May 2021 10:25:37 -0700 |
| |
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? 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
| |