From: Peter Zijlstra <peterz@infradead.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@alien8.de>,
"Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>,
Dave Hansen <dave@sr71.net>, LKML <linux-kernel@vger.kernel.org>,
Dave Jones <davej@redhat.com>,
dhillf@gmail.com, Ingo Molnar <mingo@kernel.org>
Subject: Re: [PATCH] kthread: Prevent unpark race which puts threads on the wrong cpu
Date: Fri, 12 Apr 2013 12:41:24 +0200 [thread overview]
Message-ID: <1365763284.17140.50.camel@laptop> (raw)
In-Reply-To: <alpine.LFD.2.02.1304091635430.21884@ionos>
On Tue, 2013-04-09 at 16:38 +0200, Thomas Gleixner wrote:
> The smpboot threads rely on the park/unpark mechanism which binds per
> cpu threads on a particular core. Though the functionality is racy:
>
> CPU0 CPU1 CPU2
> unpark(T) wake_up_process(T)
> clear(SHOULD_PARK) T runs
> leave parkme() due to !SHOULD_PARK
> bind_to(CPU2) BUG_ON(wrong CPU)
>
> We cannot let the tasks move themself to the target CPU as one of
> those tasks is actually the migration thread itself, which requires
> that it starts running on the target cpu right away.
>
> The only rock solid solution to this problem is to prevent wakeups in
> park state which are not from unpark(). That way we can guarantee that
> the association of the task to the target cpu is working correctly.
>
> Add a new task state (TASK_PARKED) which prevents other wakeups and
> use this state explicitely for the unpark wakeup.
explicitly
Also, since the task state is visible to userspace and all the parked
tasks are still in the PID space, its a good hint in ps and friends
that these tasks aren't really there for the moment.
>
> Reported-by: Dave Jones <davej@redhat.com>
> Reported-by: Dave Hansen <dave@sr71.net>
> Reported-by: Borislav Petkov <bp@alien8.de>
> Cc: stable@vger.kernel.org
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Assuming you're going to merge in the missing trace hunk you posted
further down the thread...
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> ---
> fs/proc/array.c | 1 +
> include/linux/sched.h | 5 +++--
> kernel/kthread.c | 38 +++++++++++++++++++++-----------------
> 3 files changed, 25 insertions(+), 19 deletions(-)
>
> Index: linux-2.6/fs/proc/array.c
> ===================================================================
> --- linux-2.6.orig/fs/proc/array.c
> +++ linux-2.6/fs/proc/array.c
> @@ -143,6 +143,7 @@ static const char * const task_state_arr
> "x (dead)", /* 64 */
> "K (wakekill)", /* 128 */
> "W (waking)", /* 256 */
> + "P (parked)", /* 512 */
> };
>
> static inline const char *get_task_state(struct task_struct *tsk)
> Index: linux-2.6/include/linux/sched.h
> ===================================================================
> --- linux-2.6.orig/include/linux/sched.h
> +++ linux-2.6/include/linux/sched.h
> @@ -163,9 +163,10 @@ print_cfs_rq(struct seq_file *m, int cpu
> #define TASK_DEAD 64
> #define TASK_WAKEKILL 128
> #define TASK_WAKING 256
> -#define TASK_STATE_MAX 512
> +#define TASK_PARKED 512
> +#define TASK_STATE_MAX 1024
>
> -#define TASK_STATE_TO_CHAR_STR "RSDTtZXxKW"
> +#define TASK_STATE_TO_CHAR_STR "RSDTtZXxKWP"
>
> extern char ___assert_task_state[1 - 2*!!(
> sizeof(TASK_STATE_TO_CHAR_STR)-1 != ilog2(TASK_STATE_MAX)+1)];
> Index: linux-2.6/kernel/kthread.c
> ===================================================================
> --- linux-2.6.orig/kernel/kthread.c
> +++ linux-2.6/kernel/kthread.c
> @@ -124,12 +124,12 @@ void *kthread_data(struct task_struct *t
>
> static void __kthread_parkme(struct kthread *self)
> {
> - __set_current_state(TASK_INTERRUPTIBLE);
> + __set_current_state(TASK_PARKED);
> while (test_bit(KTHREAD_SHOULD_PARK, &self->flags)) {
> if (!test_and_set_bit(KTHREAD_IS_PARKED, &self->flags))
> complete(&self->parked);
> schedule();
> - __set_current_state(TASK_INTERRUPTIBLE);
> + __set_current_state(TASK_PARKED);
> }
> clear_bit(KTHREAD_IS_PARKED, &self->flags);
> __set_current_state(TASK_RUNNING);
> @@ -324,6 +324,22 @@ static struct kthread *task_get_live_kth
> return NULL;
> }
>
> +static void __kthread_unpark(struct task_struct *k, struct kthread *kthread)
> +{
> + clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
> + /*
> + * We clear the IS_PARKED bit here as we don't wait
> + * until the task has left the park code. So if we'd
> + * park before that happens we'd see the IS_PARKED bit
> + * which might be about to be cleared.
> + */
> + if (test_and_clear_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
> + if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
> + __kthread_bind(k, kthread->cpu);
> + wake_up_state(k, TASK_PARKED);
> + }
> +}
> +
> /**
> * kthread_unpark - unpark a thread created by kthread_create().
> * @k: thread created by kthread_create().
> @@ -336,20 +352,8 @@ void kthread_unpark(struct task_struct *
> {
> struct kthread *kthread = task_get_live_kthread(k);
>
> - if (kthread) {
> - clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
> - /*
> - * We clear the IS_PARKED bit here as we don't wait
> - * until the task has left the park code. So if we'd
> - * park before that happens we'd see the IS_PARKED bit
> - * which might be about to be cleared.
> - */
> - if (test_and_clear_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
> - if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
> - __kthread_bind(k, kthread->cpu);
> - wake_up_process(k);
> - }
> - }
> + if (kthread)
> + __kthread_unpark(k, kthread);
> put_task_struct(k);
> }
>
> @@ -407,7 +411,7 @@ int kthread_stop(struct task_struct *k)
> trace_sched_kthread_stop(k);
> if (kthread) {
> set_bit(KTHREAD_SHOULD_STOP, &kthread->flags);
> - clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
> + __kthread_unpark(k, kthread);
> wake_up_process(k);
> wait_for_completion(&kthread->exited);
> }
next prev parent reply other threads:[~2013-04-12 10:41 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-05 21:43 kernel BUG at kernel/smpboot.c:134! Dave Hansen
2013-04-06 7:12 ` Srivatsa S. Bhat
2013-04-06 8:31 ` Thomas Gleixner
2013-04-07 9:20 ` Thomas Gleixner
2013-04-07 9:50 ` Borislav Petkov
2013-04-08 9:24 ` Thomas Gleixner
2013-04-08 11:55 ` Borislav Petkov
2013-04-08 12:17 ` Thomas Gleixner
2013-04-09 14:38 ` [PATCH] kthread: Prevent unpark race which puts threads on the wrong cpu Thomas Gleixner
2013-04-09 15:55 ` Dave Hansen
2013-04-09 18:43 ` Thomas Gleixner
2013-04-09 19:30 ` Thomas Gleixner
2013-04-09 20:38 ` Dave Hansen
2013-04-09 20:54 ` Dave Hansen
2013-04-10 8:29 ` Thomas Gleixner
2013-04-10 10:51 ` Thomas Gleixner
2013-04-10 19:41 ` Dave Hansen
2013-04-11 10:19 ` Thomas Gleixner
2013-04-11 10:48 ` Srivatsa S. Bhat
2013-04-11 11:43 ` Srivatsa S. Bhat
2013-04-11 11:59 ` Srivatsa S. Bhat
2013-04-11 12:51 ` Thomas Gleixner
2013-04-11 12:54 ` Thomas Gleixner
2013-04-11 13:46 ` Thomas Gleixner
2013-04-11 18:07 ` Dave Hansen
2013-04-11 19:48 ` Thomas Gleixner
2013-04-10 14:03 ` [PATCH] CPU hotplug, smpboot: Fix crash in smpboot_thread_fn() Srivatsa S. Bhat
2013-04-11 8:10 ` Thomas Gleixner
2013-04-11 10:19 ` Srivatsa S. Bhat
2013-04-11 19:16 ` [PATCH] kthread: Prevent unpark race which puts threads on the wrong cpu Srivatsa S. Bhat
2013-04-11 20:47 ` Thomas Gleixner
2013-04-11 21:19 ` Srivatsa S. Bhat
2013-04-12 10:59 ` Thomas Gleixner
2013-04-12 11:26 ` Srivatsa S. Bhat
2013-04-15 19:49 ` Dave Hansen
2013-04-12 10:41 ` Peter Zijlstra [this message]
2013-04-12 12:32 ` [tip:core/urgent] " tip-bot for Thomas Gleixner
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=1365763284.17140.50.camel@laptop \
--to=peterz@infradead.org \
--cc=bp@alien8.de \
--cc=dave@sr71.net \
--cc=davej@redhat.com \
--cc=dhillf@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=srivatsa.bhat@linux.vnet.ibm.com \
--cc=tglx@linutronix.de \
/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).