From: Vincent Guittot <vincent.guittot@linaro.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
frederic@kernel.org, linux-kernel <linux-kernel@vger.kernel.org>,
x86 <x86@kernel.org>, Qian Cai <cai@lca.pw>,
Mel Gorman <mgorman@techsingularity.net>
Subject: Re: [RFC][PATCH 1/7] sched: Fix smp_call_function_single_async() usage for ILB
Date: Wed, 27 May 2020 12:23:23 +0200 [thread overview]
Message-ID: <CAKfTPtA6t5=Gc6cWR3iS9QL+Vy=jhUkP345V9q2xqyhHx=rGNQ@mail.gmail.com> (raw)
In-Reply-To: <20200526161907.778543557@infradead.org>
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;
>
>
next prev parent reply other threads:[~2020-05-27 10:23 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-26 16:10 [RFC][PATCH 0/7] Fix the scheduler-IPI mess Peter Zijlstra
2020-05-26 16:10 ` [RFC][PATCH 1/7] sched: Fix smp_call_function_single_async() usage for ILB Peter Zijlstra
2020-05-26 23:56 ` Frederic Weisbecker
2020-05-27 10:23 ` Vincent Guittot [this message]
2020-05-27 11:28 ` Frederic Weisbecker
2020-05-27 12:07 ` Vincent Guittot
2020-05-29 15:26 ` Valentin Schneider
2020-06-01 9:52 ` [tip: sched/core] " tip-bot2 for Peter Zijlstra
2020-06-01 11:40 ` Frederic Weisbecker
2020-05-26 16:10 ` [RFC][PATCH 2/7] smp: Optimize flush_smp_call_function_queue() Peter Zijlstra
2020-05-28 12:28 ` Frederic Weisbecker
2020-06-01 9:52 ` [tip: sched/core] " tip-bot2 for Peter Zijlstra
2020-05-26 16:11 ` [RFC][PATCH 3/7] smp: Move irq_work_run() out of flush_smp_call_function_queue() Peter Zijlstra
2020-05-29 13:04 ` Frederic Weisbecker
2020-06-01 9:52 ` [tip: sched/core] " tip-bot2 for Peter Zijlstra
2020-05-26 16:11 ` [RFC][PATCH 4/7] smp: Optimize send_call_function_single_ipi() Peter Zijlstra
2020-05-27 9:56 ` Peter Zijlstra
2020-05-27 10:15 ` Peter Zijlstra
2020-05-27 15:56 ` Paul E. McKenney
2020-05-27 16:35 ` Peter Zijlstra
2020-05-27 17:12 ` Peter Zijlstra
2020-05-27 19:39 ` Paul E. McKenney
2020-05-28 1:35 ` Joel Fernandes
2020-05-28 8:59 ` [tip: core/rcu] rcu: Allow for smp_call_function() running callbacks from idle tip-bot2 for Peter Zijlstra
2021-01-21 16:56 ` [RFC][PATCH 4/7] smp: Optimize send_call_function_single_ipi() Peter Zijlstra
2021-01-22 0:20 ` Paul E. McKenney
2021-01-22 8:31 ` Peter Zijlstra
2021-01-22 15:35 ` Paul E. McKenney
2020-05-29 13:01 ` Frederic Weisbecker
2020-06-01 9:52 ` [tip: sched/core] " tip-bot2 for Peter Zijlstra
2020-05-26 16:11 ` [RFC][PATCH 5/7] irq_work, smp: Allow irq_work on call_single_queue Peter Zijlstra
2020-05-28 23:40 ` Frederic Weisbecker
2020-05-29 13:36 ` Peter Zijlstra
2020-06-05 9:37 ` Peter Zijlstra
2020-06-05 15:02 ` Frederic Weisbecker
2020-06-05 16:17 ` Peter Zijlstra
2020-06-05 15:24 ` Kees Cook
2020-06-10 13:24 ` Frederic Weisbecker
2020-06-01 9:52 ` [tip: sched/core] " tip-bot2 for Peter Zijlstra
2020-05-26 16:11 ` [RFC][PATCH 6/7] sched: Add rq::ttwu_pending Peter Zijlstra
2020-06-01 9:52 ` [tip: sched/core] " tip-bot2 for Peter Zijlstra
2020-05-26 16:11 ` [RFC][PATCH 7/7] sched: Replace rq::wake_list Peter Zijlstra
2020-05-29 15:10 ` Valdis Klētnieks
2020-06-01 9:52 ` [tip: sched/core] " tip-bot2 for Peter Zijlstra
2020-06-02 15:16 ` Frederic Weisbecker
2020-06-04 14:18 ` [RFC][PATCH 7/7] " Guenter Roeck
2020-06-05 0:24 ` Eric Biggers
2020-06-05 7:41 ` Peter Zijlstra
2020-06-05 16:15 ` Eric Biggers
2020-06-06 23:13 ` Guenter Roeck
2020-06-09 20:21 ` Eric Biggers
2020-06-09 21:25 ` Guenter Roeck
2020-06-09 21:38 ` Eric Biggers
2020-06-09 22:06 ` Peter Zijlstra
2020-06-09 23:03 ` Guenter Roeck
2020-06-10 9:09 ` Peter Zijlstra
2020-06-18 17:57 ` Steven Rostedt
2020-06-18 19:06 ` Guenter Roeck
2020-06-09 22:07 ` Peter Zijlstra
2020-06-05 8:10 ` Peter Zijlstra
2020-06-05 13:33 ` Guenter Roeck
2020-06-05 14:09 ` Peter Zijlstra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAKfTPtA6t5=Gc6cWR3iS9QL+Vy=jhUkP345V9q2xqyhHx=rGNQ@mail.gmail.com' \
--to=vincent.guittot@linaro.org \
--cc=cai@lca.pw \
--cc=frederic@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mgorman@techsingularity.net \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).