Messages in this thread Patch in this message | | | Date | Tue, 5 May 2020 23:16:41 +0800 | From | Peng Liu <> | Subject | Re: [PATCH] sched/fair: Fix nohz.next_balance update |
| |
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?
/* 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)
| |