Messages in this thread | | | From | Vincent Guittot <> | Date | Wed, 27 May 2020 12:23:23 +0200 | 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@infradead.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@lca.pw> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Vincent Guittot <vincent.guittot@linaro.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; > >
| |