Re: [PATCH v1] kthread/smpboot: Serialize kthread parking against wakeup

["Kohli, Gaurav" <gkohli@codeaurora.org>]

On 5/1/2018 6:49 PM, Peter Zijlstra wrote:
>> On 5/1/2018 5:01 PM, Peter Zijlstra wrote:
> 
>>> Let me ponder what the best solution is, it's a bit of a mess.
> 
> So there's:
> 
>   - TASK_PARKED, which we can make a special state; this solves the
>     problem because then wait_task_inactive() is guaranteed to see
>     TASK_PARKED and DTRT.

Yes this should work, as it takes pi-lock and blocking kthread_parkme.
> 
>   - complete(&kthread->parked), which we can do inside schedule(); this
>     solves the problem because then kthread_park() will not return early
>     and the task really is blocked.

I think complete will not help, as problem is like below :

Control Thread                                CPUHP thread
					
					      cpuhp_thread_fun
					      Wake control thread
					      complete(&st->done);

takedown_cpu
kthread_park
set_bit(KTHREAD_SHOULD_PARK

					     Here cpuhp is looping,
					//success case
					     Generally when issue is not
					     coming
					     it schedule out by below :
                                            ht->thread_should_run(td->cpu
					      scheduler
					//failure case
					before schedule
					loop check
					(kthread_should_park()
				         enter here as PARKED set

wake_up_process(k)
					__kthread_parkme
					 complete(&self->parked);
SETS RUNNING
		                         schedule			
wait_for_completion(&kthread->parked);


So even we protect complete, it may see TASK_RUNNING as final state.
That's why we took pi-lock , So wake_up_process either see TASK_RUNNING
so return or it will exit early before parkme call.

Please correct me , if i misunderstood it. With pi-lock approach we are 
not seeing issue. SO you first solution should fix this(put parked in 
special state).

Regards
Gaurav
					

> 
> and I'm fairly sure I thought of a 3rd way to cure things, but now that
> I'm writing things down I cannot seem to remember :/ -- could be we muck
> with wait_task_inactive().
> 
> In any case, I hate all of them, but I think the completion one is the
> least horrible because it gives the strongest guarantees and cleans up
> most. But promoting TASK_PARKED to special certainly is the earier
> patch.
> 
> The below boots, but that's about all I did with it. Opinions?
> 
> ---
> 
> --- a/include/linux/kthread.h
> +++ b/include/linux/kthread.h
> @@ -62,6 +62,7 @@ void *kthread_probe_data(struct task_str
>   int kthread_park(struct task_struct *k);
>   void kthread_unpark(struct task_struct *k);
>   void kthread_parkme(void);
> +void kthread_park_complete(struct task_struct *k);
>   
>   int kthreadd(void *unused);
>   extern struct task_struct *kthreadd_task;
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -55,7 +55,6 @@ enum KTHREAD_BITS {
>   	KTHREAD_IS_PER_CPU = 0,
>   	KTHREAD_SHOULD_STOP,
>   	KTHREAD_SHOULD_PARK,
> -	KTHREAD_IS_PARKED,
>   };
>   
>   static inline void set_kthread_struct(void *kthread)
> @@ -177,14 +176,12 @@ void *kthread_probe_data(struct task_str
>   
>   static void __kthread_parkme(struct kthread *self)
>   {
> -	__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);
> +	for (;;) {
> +		set_current_state(TASK_PARKED);
> +		if (!test_bit(KTHREAD_SHOULD_PARK, &self->flags))
> +			break;
>   		schedule();
> -		__set_current_state(TASK_PARKED);
>   	}
> -	clear_bit(KTHREAD_IS_PARKED, &self->flags);
>   	__set_current_state(TASK_RUNNING);
>   }
>   
> @@ -194,6 +191,11 @@ void kthread_parkme(void)
>   }
>   EXPORT_SYMBOL_GPL(kthread_parkme);
>   
> +void kthread_park_complete(struct task_struct *k)
> +{
> +	complete(&to_kthread(k)->parked);
> +}
> +
>   static int kthread(void *_create)
>   {
>   	/* Copy data: it's on kthread's stack */
> @@ -450,22 +452,15 @@ void kthread_unpark(struct task_struct *
>   {
>   	struct kthread *kthread = to_kthread(k);
>   
> -	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.
> +	 * Newly created kthread was parked when the CPU was offline.
> +	 * The binding was lost and we need to set it again.
>   	 */
> -	if (test_and_clear_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
> -		/*
> -		 * Newly created kthread was parked when the CPU was offline.
> -		 * The binding was lost and we need to set it again.
> -		 */
> -		if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
> -			__kthread_bind(k, kthread->cpu, TASK_PARKED);
> -		wake_up_state(k, TASK_PARKED);
> -	}
> +	if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
> +		__kthread_bind(k, kthread->cpu, TASK_PARKED);
> +
> +	clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
> +	wake_up_state(k, TASK_PARKED);
>   }
>   EXPORT_SYMBOL_GPL(kthread_unpark);
>   
> @@ -488,12 +483,13 @@ int kthread_park(struct task_struct *k)
>   	if (WARN_ON(k->flags & PF_EXITING))
>   		return -ENOSYS;
>   
> -	if (!test_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
> -		set_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
> -		if (k != current) {
> -			wake_up_process(k);
> -			wait_for_completion(&kthread->parked);
> -		}
> +	if (WARN_ON_ONCE(test_bit(KTHREAD_SHOULD_PARK, &kthread->flags)))
> +		return -EBUSY;
> +
> +	set_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
> +	if (k != current) {
> +		wake_up_process(k);
> +		wait_for_completion(&kthread->parked);
>   	}
>   
>   	return 0;
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7,6 +7,8 @@
>    */
>   #include "sched.h"
>   
> +#include <linux/kthread.h>
> +
>   #include <asm/switch_to.h>
>   #include <asm/tlb.h>
>   
> @@ -2718,20 +2720,28 @@ static struct rq *finish_task_switch(str
>   		membarrier_mm_sync_core_before_usermode(mm);
>   		mmdrop(mm);
>   	}
> -	if (unlikely(prev_state == TASK_DEAD)) {
> -		if (prev->sched_class->task_dead)
> -			prev->sched_class->task_dead(prev);
> +	if (unlikely(prev_state & (TASK_DEAD|TASK_PARKED))) {
> +		switch (prev_state) {
> +		case TASK_DEAD:
> +			if (prev->sched_class->task_dead)
> +				prev->sched_class->task_dead(prev);
> +
> +			/*
> +			 * Remove function-return probe instances associated with this
> +			 * task and put them back on the free list.
> +			 */
> +			kprobe_flush_task(prev);
>   
> -		/*
> -		 * Remove function-return probe instances associated with this
> -		 * task and put them back on the free list.
> -		 */
> -		kprobe_flush_task(prev);
> +			/* Task is done with its stack. */
> +			put_task_stack(prev);
>   
> -		/* Task is done with its stack. */
> -		put_task_stack(prev);
> +			put_task_struct(prev);
> +			break;
>   
> -		put_task_struct(prev);
> +		case TASK_PARKED:
> +			kthread_park_complete(prev);
> +			break;
> +		}
>   	}
>   
>   	tick_nohz_task_switch();
> 

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, 
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.