Messages in this thread | | | From | Vincent Guittot <> | Date | Tue, 5 May 2020 17:43:04 +0200 | Subject | Re: [PATCH] sched/fair: Fix nohz.next_balance update |
| |
On Tue, 5 May 2020 at 17:16, Peng Liu <iwtbavbm@gmail.com> wrote: > > On Tue, May 05, 2020 at 04:27:11PM +0200, Vincent Guittot wrote: > > Le mardi 05 mai 2020 à 21:40:56 (+0800), Peng Liu a écrit : > > > On Mon, May 04, 2020 at 05:17:11PM +0200, Vincent Guittot wrote: > > > > On Sun, 3 May 2020 at 10:34, Peng Liu <iwtbavbm@gmail.com> wrote: > > > > > > > [...] > > > > Yes, you're right. When need_resched() returns true, things become out > > > of expectation. We haven't really got the earliest next_balance, abort > > > the update immediately and let the successor to help. Doubtless this > > > will incur some overhead due to the repeating work. > > > > There should not be some repeating works because CPUs and sched_domain, which > > have already been balanced, will not be rebalanced until the next load balance > > interval. > > > > Futhermore, there is in fact still work to do bcause not all the idle CPUs got > > a chance to pull work > > > > > > > > > > > About the "tick is not stopped when entering idle" case, defer the > > > update to nohz_balance_enter_idle() would be a choice too. > > > > > > > > > Of course, only update nohz.next_balance in rebalance_domains() is the > > > simpliest way, but as @Valentin put, too many write to it may incur > > > unnecessary overhead. If we can gather the earliest next_balance in > > > > This is not really possible because we have to move it to the next interval. > > > > > advance, then a single write is considered to be better. > > > > > > By the way, remove the redundant check in nohz_idle_balance(). > > > > > > FWIW, how about the below? > > > > Your proposal below looks quite complex. IMO, one solution would be to move the > > update of nohz.next_balance before calling rebalance_domains(this_rq, CPU_IDLE) > > so you are back to the previous behavior. > > > > The only difference is that in case of an break because of need_resched, it > > doesn't update nohz.next_balance. But on the other hand, we haven't yet > > finished run rebalance_domains for all CPUs and some load_balance are still > > pending. In fact, this will be done during next tick by an idle CPU. > > > > So I would be in favor of something as simple as : > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > index 04098d678f3b..e028bc1c4744 100644 > > --- a/kernel/sched/fair.c > > +++ b/kernel/sched/fair.c > > @@ -10457,6 +10457,14 @@ static bool _nohz_idle_balance(struct rq *this_rq, unsigned int flags, > > } > > } > > > > + /* > > + * next_balance will be updated only when there is a need. > > + * When the CPU is attached to null domain for ex, it will not be > > + * updated. > > + */ > > + if (likely(update_next_balance)) > > + nohz.next_balance = next_balance; > > + > > /* Newly idle CPU doesn't need an update */ > > if (idle != CPU_NEWLY_IDLE) { > > update_blocked_averages(this_cpu); > > @@ -10477,14 +10485,6 @@ static bool _nohz_idle_balance(struct rq *this_rq, unsigned int flags, > > if (has_blocked_load) > > WRITE_ONCE(nohz.has_blocked, 1); > > > > - /* > > - * next_balance will be updated only when there is a need. > > - * When the CPU is attached to null domain for ex, it will not be > > - * updated. > > - */ > > - if (likely(update_next_balance)) > > - nohz.next_balance = next_balance; > > - > > return ret; > > } > > > > Indeed, simple and straightforward, it's better. > > > > *********************************************** > > > * Below code is !!!ENTIRELY UNTESTED!!!, just * > > [...] > > > > @@ -10354,9 +10350,7 @@ static bool nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle) > > > { > > > int this_cpu = this_rq->cpu; > > > unsigned int flags; > > > - > > > - if (!(atomic_read(nohz_flags(this_cpu)) & NOHZ_KICK_MASK)) > > > - return false; > > > > why did you remove this ? > > > > It seems that below 'if' do the same thing, isn't?
The test above is an optimization for the most common case
> > /* could be _relaxed() */ > flags = atomic_fetch_andnot(NOHZ_KICK_MASK, nohz_flags(this_cpu)); > if (!(flags & NOHZ_KICK_MASK)) > return false; > > > > + bool done; > > > > > > if (idle != CPU_IDLE) { > > > atomic_andnot(NOHZ_KICK_MASK, nohz_flags(this_cpu)); > > > @@ -10368,9 +10362,16 @@ static bool nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle) > > > if (!(flags & NOHZ_KICK_MASK)) > > > return false; > > > > > [...] > > > > static void nohz_newidle_balance(struct rq *this_rq)
| |