Date:   Tue, 5 May 2020 20:36:28 +0800
From:   Peng Liu <iwtbavbm@...il.com>
To:     Valentin Schneider <valentin.schneider@....com>
Cc:     mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
        iwtbavbm@...il.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched/fair: Fix nohz.next_balance update

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.schneider@....com>
> 
> > Signed-off-by: Peng Liu <iwtbavbm@...il.com>
> > Cc: Ingo Molnar <mingo@...hat.com>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Juri Lelli <juri.lelli@...hat.com>
> > Cc: Vincent Guittot <vincent.guittot@...aro.org>
> > Cc: Dietmar Eggemann <dietmar.eggemann@....com>
> > Cc: Steven Rostedt <rostedt@...dmis.org>
> > Cc: Ben Segall <bsegall@...gle.com>
> > Cc: Mel Gorman <mgorman@...e.de>
> > Cc: Valentin Schneider <valentin.schneider@....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));

Powered by blists - more mailing lists