On Mon, May 04, 2020 at 01:10:46AM +0100, Valentin Schneider wrote:
>
> Hi,
>
> On 03/05/20 09:34, Peng Liu wrote:
> > commit c5afb6a87f23 ("sched/fair: Fix nohz.next_balance update")
>
> I got confused because this has the same topic as your patch, but that's a
> genuine commit from 2015. Is this meant to be a "Fixes:" reference?
>
Ah, it was careless of me, that's a sane patch from Vincent, which I
referred when tracking the issue, but finally forgot to remove it from
changelog, not related to this patch.
> > During idle load balance, this_cpu(ilb) do load balance for the other
> > idle CPUs, also gather the earliest (nohz.)next_balance.
> >
> > Since commit:
> > 'b7031a02ec75 ("sched/fair: Add NOHZ_STATS_KICK")'
> >
> > We update nohz.next_balance like this:
> >
> > _nohz_idle_balance() {
> > for_each_cpu(nohz.idle_cpus_mask) {
> > rebalance_domains() {
> > update nohz.next_balance <-- compare and update
> > }
> > }
> > rebalance_domains(this_cpu) {
> > update nohz.next_balance <-- compare and update
> > }
> > update nohz.next_balance <-- unconditionally update
> > }
> >
> > For instance, nohz.idle_cpus_mask spans {cpu2,3,5,8}, and this_cpu is
> > cpu5. After the above loop we could gather the earliest *next_balance*
> > among {cpu2,3,8}, then rebalance_domains(this_cpu) update
> > nohz.next_balance with this_rq->next_balance, but finally overwrite
> > nohz.next_balance with the earliest *next_balance* among {cpu2,3,8},
> > we may end up with not getting the earliest next_balance.
> >
>
> That does look like it, nice catch!
>
> > Since we can gather all the updated rq->next_balance, including this_cpu,
> > in _nohz_idle_balance(), it's safe to remove the extra lines in
> > rebalance_domains() which are originally intended for this_cpu. And
> > finally the updating only happen in _nohz_idle_balance().
> >
>
> One added benefit of this is that we get rid of extra writes to
> nohz.next_balance, since that special case in rebalance_domains() could be
> hit by all NOHZ CPUs, not just the ILB.
>
> With the below comment taken into account:
>
> Reviewed-by: Valentin Schneider <valentin.schnei...@arm.com>
>
> > Signed-off-by: Peng Liu <iwtba...@gmail.com>
> > Cc: Ingo Molnar <mi...@redhat.com>
> > Cc: Peter Zijlstra <pet...@infradead.org>
> > Cc: Juri Lelli <juri.le...@redhat.com>
> > Cc: Vincent Guittot <vincent.guit...@linaro.org>
> > Cc: Dietmar Eggemann <dietmar.eggem...@arm.com>
> > Cc: Steven Rostedt <rost...@goodmis.org>
> > Cc: Ben Segall <bseg...@google.com>
> > Cc: Mel Gorman <mgor...@suse.de>
> > Cc: Valentin Schneider <valentin.schnei...@arm.com>
> > ---
> > kernel/sched/fair.c | 24 ++++++++----------------
> > 1 file changed, 8 insertions(+), 16 deletions(-)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 02f323b85b6d..1d0cf33fefad 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -9943,22 +9943,8 @@ static void rebalance_domains(struct rq *rq, enum
> > cpu_idle_type idle)
> > * When the cpu is attached to null domain for ex, it will not be
> > * updated.
> > */
> > - if (likely(update_next_balance)) {
> > + if (likely(update_next_balance))
> > rq->next_balance = next_balance;
> > -
> > -#ifdef CONFIG_NO_HZ_COMMON
> > - /*
> > - * If this CPU has been elected to perform the nohz idle
> > - * balance. Other idle CPUs have already rebalanced with
> > - * nohz_idle_balance() and nohz.next_balance has been
> > - * updated accordingly. This CPU is now running the idle load
> > - * balance for itself and we need to update the
> > - * nohz.next_balance accordingly.
> > - */
> > - if ((idle == CPU_IDLE) && time_after(nohz.next_balance,
> > rq->next_balance))
> > - nohz.next_balance = rq->next_balance;
> > -#endif
> > - }
> > }
> >
> > static inline int on_null_domain(struct rq *rq)
> > @@ -10321,9 +10307,15 @@ static bool _nohz_idle_balance(struct rq *this_rq,
> > unsigned int flags,
> > has_blocked_load |= this_rq->has_blocked_load;
> > }
> >
> > - if (flags & NOHZ_BALANCE_KICK)
> > + if (flags & NOHZ_BALANCE_KICK) {
> > rebalance_domains(this_rq, CPU_IDLE);
> >
> > + if (time_after(next_balance, this_rq->next_balance)) {
> > + next_balance = this_rq->next_balance;
> > + update_next_balance = 1;
> > + }
> > + }
>
> To align with what we do for the other NOHZ CPUs, shouldn't this update
> be outside of the NOHZ_BALANCE_KICK condition? That way we can update
> nohz.next_balance with just NOHZ_STATS_KICK, which IMO is the expected
> course of action.
>
I think I got your meaning, in _nohz_idle_balance():
for_each_cpu(balance_cpu, nohz.idle_cpus_mask) {
if (time_after_eq(jiffies, rq->next_balance)) {
if (flags & NOHZ_BALANCE_KICK)
rebalance_domains(rq, CPU_IDLE);
}
if (time_after(next_balance, rq->next_balance)) {
next_balance = rq->next_balance;
update_next_balance = 1;
}
}
nohz.next_balance is the earliest next_balance, so some rqs'
next_balance may be not due, some are due and updated, so we need
take all of them into consideration.
In NOHZ_STAT_KICK case, all rq don't go through rebalance_domains(),
their next_balance are supposed to be unchanged, including this_rq,
so we can safely left nohz.next_balance unchanged.
Thanks for your time!
> > +
> > WRITE_ONCE(nohz.next_blocked,
> > now + msecs_to_jiffies(LOAD_AVG_PERIOD));
