Re: [RFC][PATCH 1/7] sched: Fix smp_call_function_single_async() usage for ILB

From: Vincent Guittot
Date: Wed May 27 2020 - 06:23:38 EST


On Tue, 26 May 2020 at 18:19, Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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@xxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>

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

> ---
> 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;
>
>