Messages in this thread | | | From | Vincent Guittot <> | Date | Mon, 25 Jan 2021 14:23:51 +0100 | Subject | Re: [PATCH] sched/fair: Rate limit calls to update_blocked_averages() for NOHZ |
| |
On Fri, 22 Jan 2021 at 19:39, Qais Yousef <qais.yousef@arm.com> wrote: > > On 01/22/21 17:56, Vincent Guittot wrote: > > > --- > > > kernel/sched/fair.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > > index 04a3ce20da67..fe2dc0024db5 100644 > > > --- a/kernel/sched/fair.c > > > +++ b/kernel/sched/fair.c > > > @@ -8381,7 +8381,7 @@ static bool update_nohz_stats(struct rq *rq, bool force) > > > if (!cpumask_test_cpu(cpu, nohz.idle_cpus_mask)) > > > return false; > > > > > > - if (!force && !time_after(jiffies, rq->last_blocked_load_update_tick)) > > > + if (!force && !time_after(jiffies, rq->last_blocked_load_update_tick + (HZ/20))) > > > > This condition is there to make sure to update blocked load at most > > once a tick in order to filter newly idle case otherwise the rate > > limit is already done by load balance interval > > This hard coded (HZ/20) looks really like an ugly hack > > This was meant as an RFC patch to discuss the problem really. > > Joel is seeing update_blocked_averages() taking ~100us. Half of it seems in > processing __update_blocked_fair() and the other half in sugov_update_shared(). > So roughly 50us each. Note that each function is calling an iterator in
Can I assume that a freq change happens if sugov_update_shared() takes 50us ? which would mean that the update was useful at the end ?
> return. Correct me if my numbers are wrong Joel. > > Running on a little core on low frequency these numbers don't look too odd. > So I'm not seeing how we can speed these functions up. > > But since update_sg_lb_stats() will end up with multiple calls to > update_blocked_averages() in one go, this latency adds up quickly. > > One noticeable factor in Joel's system is the presence of a lot of cgroups. > Which is essentially what makes __update_blocked_fair() expensive, and it seems > to always return something has decayed so we end up with a call to > sugov_update_shared() in every call. > > I think we should limit the expensive call to update_blocked_averages() but
At the opposite, some will complain that block values stay stall to high value and prevent any useful adjustment.
Also update_blocked average is already rate limited with idle and busy load_balance
Seems like the problem raised by Joel is the number of newly idle load balance
> I honestly don't know what would be the right way to do it :-/ > > Or maybe there's another better alternative too.. > > Thanks > > -- > Qais Yousef > > > > > > return true; > > > > > > update_blocked_averages(cpu); > > > -- > > > 2.30.0.280.ga3ce27912f-goog > > >
| |