LKML Archive on lore.kernel.org
 help / color / Atom feed
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
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);
>         }



  parent reply index

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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git