Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [PATCH] sched/fair: Fix nohz.next_balance update | Date | Mon, 04 May 2020 01:10:46 +0100 |
| |
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?
> 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@arm.com>
> Signed-off-by: Peng Liu <iwtbavbm@gmail.com> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Juri Lelli <juri.lelli@redhat.com> > Cc: Vincent Guittot <vincent.guittot@linaro.org> > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Ben Segall <bsegall@google.com> > Cc: Mel Gorman <mgorman@suse.de> > Cc: Valentin Schneider <valentin.schneider@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.
> + > WRITE_ONCE(nohz.next_blocked, > now + msecs_to_jiffies(LOAD_AVG_PERIOD));
| |