Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200503083407.GA27766@iZj6chx1xj0e0buvshuecpZ>
In-Reply-To: <20200503083407.GA27766@iZj6chx1xj0e0buvshuecpZ>
From:   Vincent Guittot <vincent.guittot@linaro.org>
Date:   Mon, 4 May 2020 17:17:11 +0200
Message-ID: <CAKfTPtCNG9Y4xNA-iLd+JRRsUCA1+SkkFFRbbzk5n7q6v401tw@mail.gmail.com>
Subject: Re: [PATCH] sched/fair: Fix nohz.next_balance update
To:     Peng Liu <iwtbavbm@gmail.com>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        Valentin Schneider <valentin.schneider@arm.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, 3 May 2020 at 10:34, Peng Liu <iwtbavbm@gmail.com> wrote:
>
> commit c5afb6a87f23 ("sched/fair: Fix nohz.next_balance update")
> 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.
>
> 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().

I'm not sure that's always true. Nothing prevents nohz_idle_balance()
to return false . Then run_rebalance_domains() calls
rebalance_domains(this_rq ,SCHED_IDLE) outside _nohz_idle_balance().
In this case we must keep the code in rebalance_domains().

For example when the tick is not stopped when entering idle. Or when
need_resched() returns true.

So instead of removing the code from rebalance_domains, you should
move the one in _nohz_idle_balance() to make sure that the "if
(likely(update_next_balance)) ..." is called before calling
rebalance_domains for the local cpu


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