Re: [PATCH] kthread: Fix the race condition when kthread is parked

[Subbaraman Narayanamurthy]

On 06/25/14 17:43, Thomas Gleixner wrote:
> 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)
> 				   	  __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
I understood the explanation above. But still I don't understand how 
this premature wakeup of T2 is happening/possible? Also, what will 
happen if the task state is not in TASK_PARKED when __kthread_unpark is 
called? __kthread_bind will fail silently causing the same problem.
> 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<subbaram@xxxxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner<tglx@xxxxxxxxxxxxx>
> Cc:stable@xxxxxxxxxxxxxxx
>
> ---
>   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);
> -	}
>   }
>   
>   /**
Thanks for the patch. I've tested (running hotplug tests) it for 
sometime and looks good so far. Can you please submit it?

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/