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.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.
> +
> WRITE_ONCE(nohz.next_blocked,
> now + msecs_to_jiffies(LOAD_AVG_PERIOD));
