Date:   Wed, 27 May 2020 12:23:23 +0200
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>, frederic@...nel.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        x86 <x86@...nel.org>, Qian Cai <cai@....pw>,
        Mel Gorman <mgorman@...hsingularity.net>
Subject: Re: [RFC][PATCH 1/7] sched: Fix smp_call_function_single_async()
 usage for ILB

On Tue, 26 May 2020 at 18:19, Peter Zijlstra <peterz@...radead.org> wrote:
>
> The recent commit: 90b5363acd47 ("sched: Clean up scheduler_ipi()")
> got smp_call_function_single_async() subtly wrong. Even though it will
> return -EBUSY when trying to re-use a csd, that condition is not
> atomic and still requires external serialization.
>
> The change in kick_ilb() got this wrong.
>
> While on first reading kick_ilb() has an atomic test-and-set that
> appears to serialize the use, the matching 'release' is not in the
> right place to actually guarantee this serialization.
>
> Rework the nohz_idle_balance() trigger so that the release is in the
> IPI callback and thus guarantees the required serialization for the
> CSD.
>
> Fixes: 90b5363acd47 ("sched: Clean up scheduler_ipi()")
> Reported-by: Qian Cai <cai@....pw>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>

Reviewed-by: Vincent Guittot <vincent.guittot@...aro.org>

> ---
>  include/linux/sched.h      |    4 +
>  include/linux/sched/idle.h |   55 ++++++++++++++++++
>  kernel/sched/core.c        |  132 +++++++++------------------------------------
>  kernel/sched/fair.c        |   18 ++----
>  kernel/sched/idle.c        |    3 -
>  kernel/sched/sched.h       |    2
>  kernel/smp.c               |    7 ++
>  7 files changed, 102 insertions(+), 119 deletions(-)
>
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -637,41 +568,25 @@ void wake_up_nohz_cpu(int cpu)
>                 wake_up_idle_cpu(cpu);
>  }
>
> -static inline bool got_nohz_idle_kick(void)
> +static void nohz_csd_func(void *info)
>  {
> -       int cpu = smp_processor_id();
> -
> -       if (!(atomic_read(nohz_flags(cpu)) & NOHZ_KICK_MASK))
> -               return false;
> -
> -       if (idle_cpu(cpu) && !need_resched())
> -               return true;
> +       struct rq *rq = info;
> +       int cpu = cpu_of(rq);
> +       unsigned int flags;
>
>         /*
> -        * We can't run Idle Load Balance on this CPU for this time so we
> -        * cancel it and clear NOHZ_BALANCE_KICK
> +        * Release the rq::nohz_csd.
>          */
> -       atomic_andnot(NOHZ_KICK_MASK, nohz_flags(cpu));
> -       return false;
> -}
> -
> -static void nohz_csd_func(void *info)
> -{
> -       struct rq *rq = info;
> +       flags = atomic_fetch_andnot(NOHZ_KICK_MASK, nohz_flags(cpu));

Why can't this be done in nohz_idle_balance() instead ?

you are not using flags in nohz_csd_func() and SCHED_SOFTIRQ which
calls nohz_idle_balance(), happens after nohz_csd_func(), isn't it ?

In this case, you don't have to use the intermediate variable
this_rq->nohz_idle_balance


> +       WARN_ON(!(flags & NOHZ_KICK_MASK));
>
> -       if (got_nohz_idle_kick()) {
> -               rq->idle_balance = 1;
> +       rq->idle_balance = idle_cpu(cpu);
> +       if (rq->idle_balance && !need_resched()) {
> +               rq->nohz_idle_balance = flags;
>                 raise_softirq_irqoff(SCHED_SOFTIRQ);
>         }
>  }
>
> -#else /* CONFIG_NO_HZ_COMMON */
> -
> -static inline bool got_nohz_idle_kick(void)
> -{
> -       return false;
> -}
> -
>  #endif /* CONFIG_NO_HZ_COMMON */
>
>  #ifdef CONFIG_NO_HZ_FULL
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -10024,6 +10024,10 @@ static void kick_ilb(unsigned int flags)
>         if (ilb_cpu >= nr_cpu_ids)
>                 return;
>
> +       /*
> +        * Access to rq::nohz_csd is serialized by NOHZ_KICK_MASK; he who sets
> +        * the first flag owns it; cleared by nohz_csd_func().
> +        */
>         flags = atomic_fetch_or(flags, nohz_flags(ilb_cpu));
>         if (flags & NOHZ_KICK_MASK)
>                 return;
> @@ -10371,20 +10375,14 @@ static bool _nohz_idle_balance(struct rq
>   */
>  static bool nohz_idle_balance(struct rq *this_rq, enum cpu_idle_type idle)
>  {
> -       int this_cpu = this_rq->cpu;
> -       unsigned int flags;
> +       unsigned int flags = this_rq->nohz_idle_balance;
>
> -       if (!(atomic_read(nohz_flags(this_cpu)) & NOHZ_KICK_MASK))
> +       if (!flags)
>                 return false;
>
> -       if (idle != CPU_IDLE) {
> -               atomic_andnot(NOHZ_KICK_MASK, nohz_flags(this_cpu));
> -               return false;
> -       }
> +       this_rq->nohz_idle_balance = 0;
>
> -       /* could be _relaxed() */
> -       flags = atomic_fetch_andnot(NOHZ_KICK_MASK, nohz_flags(this_cpu));
> -       if (!(flags & NOHZ_KICK_MASK))
> +       if (idle != CPU_IDLE)
>                 return false;
>
>         _nohz_idle_balance(this_rq, flags, idle);
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -951,6 +951,7 @@ struct rq {
>
>         struct callback_head    *balance_callback;
>
> +       unsigned char           nohz_idle_balance;
>         unsigned char           idle_balance;
>
>         unsigned long           misfit_task_load;
>
>

Powered by blists - more mailing lists