On Thu, Jun 26, 2014 at 02:43:56AM +0200, Thomas Gleixner wrote:
>
> Subject: kthread: Plug park/ unplug race
> From: Thomas Gleixner <t...@linutronix.de>
> Date: Thu, 26 Jun 2014 01:24:36 +0200
>
> The kthread park/unpark logic has the following issue:
>
> Task CPU 0 CPU 1
>
> T1 unplug cpu1
> kthread_park(T2)
> set_bit(KTHREAD_SHOULD_PARK);
> wait_for_completion()
> T2 parkme(X)
But with your patch, isn't it possible for T1 to call thread_unpark here?
Then looking at the code I see this turn of events:
if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
__kthread_bind(k, kthread->cpu, TASK_PARKED);
Which in __kthread_bind() (state == TASK_PARKED)
if (!wait_task_inactive(p, state)) {
WARN_ON(1);
return;
}
Where wait_task_inactive() does:
while (task_running(rq, p)) {
if (match_state && unlikely(p->state != match_state))
return 0;
As match_state is non zero and p->state != match_state because it hasn't been
set yet. The wait_task_inactive() returns zero, and then we hit the WARN_ON()
in __kthread_bind().
-- Steve
> __set_current_state(TASK_PARKED);
> while (test_bit(KTHREAD_SHOULD_PARK))
> {
> if
> (!test_and_set_bit(KTHREAD_IS_PARKED))
> complete();
> schedule();
> T1 plug cpu1
>
> --> premature wakeup of T2, i.e. before unpark, so T2 gets scheduled on
> CPU 0
>
> T2 __set_current_state(TASK_PARKED);
>
> --> Preemption by the plug thread
>
> T1 thread_unpark(T2)
> clear_bit(KTHREAD_SHOULD_PARK);
>
> --> Preemption by the softirq thread which breaks out of the
> while(test_bit(KTHREAD_SHOULD_PARK)) loop because
> KTHREAD_SHOULD_PARK is not longer set.
>
> T2 }
> clear_bit(KTHREAD_IS_PARKED);
>
> --> Now T2 happily continues to run on CPU0 which rightfully causes
> the BUG_ON(T2->cpu != smp_processor_id()) to trigger.
>
> T1
> __kthread_bind(T2)
>
> --> Too late ....
>
> Reorder the logic so that the unplug code binds the thread to the
> target cpu before clearing the KTHREAD_SHOULD_PARK bit.
>
> Reported-by: Subbaraman Narayanamurthy <subba...@codeaurora.org>
> Signed-off-by: Thomas Gleixner <t...@linutronix.de>
> Cc: sta...@vger.kernel.org
>
> ---
> kernel/kthread.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> Index: linux/kernel/kthread.c
> ===================================================================
> --- linux.orig/kernel/kthread.c
> +++ linux/kernel/kthread.c
> @@ -382,6 +382,15 @@ struct task_struct *kthread_create_on_cp
>
> static void __kthread_unpark(struct task_struct *k, struct kthread *kthread)
> {
> + /*
> + * Rebind the thread to the target cpu first if it is a per
> + * cpu thread unconditionally because it must be bound to the
> + * target cpu before it can observe the KTHREAD_SHOULD_PARK
> + * bit cleared.
> + */
> + if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
> + __kthread_bind(k, kthread->cpu, TASK_PARKED);
> +
> clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
> /*
> * We clear the IS_PARKED bit here as we don't wait
> @@ -389,11 +398,8 @@ static void __kthread_unpark(struct task
> * 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, TASK_PARKED);
> + if (test_and_clear_bit(KTHREAD_IS_PARKED, &kthread->flags))
> wake_up_state(k, TASK_PARKED);
> - }
> }
>
> /**
>
>
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
