Re: [RFC patch 1/5] kthread: Implement park/unpark facility

[Namhyung Kim <namhyung@kernel.org>]

Hi, Thomas

A nitpick and questions. :)


On Wed, 13 Jun 2012 11:00:54 -0000, Thomas Gleixner wrote:
> To avoid the full teardown/setup of per cpu kthreads in the case of
> cpu hot(un)plug, provide a facility which allows to put the kthread
> into a park position and unpark it when the cpu comes online again.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
[snip]
>  
>  /**
> + * kthread_create_on_cpu - Create a cpu bound kthread
> + * @threadfn: the function to run until signal_pending(current).
> + * @data: data ptr for @threadfn.
> + * @node: memory node number.

Should be @cpu.


> + * @namefmt: printf-style name for the thread.
> + *
> + * Description: This helper function creates and names a kernel thread
> + * and binds it to a given CPU. The thread will be woken and put into
> + * park mode.
> + */
> +struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
> +					  void *data, unsigned int cpu,
> +					  const char *namefmt)
> +{
> +	struct task_struct *p;
> +
> +	p = kthread_create_on_node(threadfn, data, cpu_to_node(cpu), namefmt,
> +				   cpu);
> +	if (IS_ERR(p))
> +		return p;
> +	/* Park the thread, mark it percpu and then bind it */
> +	kthread_park(p);

Why park? AFAIK the p is not running after creation, so binding it
doesn't need parking, right?


> +	set_bit(KTHREAD_IS_PER_CPU, &to_kthread(p)->flags);
> +	to_kthread(p)->cpu = cpu;
> +	__kthread_bind(p, cpu);
> +	return p;
> +}
> +
> +/**
> + * kthread_unpark - unpark a thread created by kthread_create().
> + * @k:		thread created by kthread_create().
> + *
> + * Sets kthread_should_park() for @k to return false, wakes it, and
> + * waits for it to return. If the thread is marked percpu then its
> + * bound to the cpu again.
> + */
> +void kthread_unpark(struct task_struct *k)
> +{
> +	struct kthread *kthread;
> +
> +	get_task_struct(k);
> +
> +	kthread = to_kthread(k);
> +	barrier(); /* it might have exited */
> +	if (k->vfork_done != NULL) {

Does is guarantee it's not exited? If so, as it is used a couple of
times, wouldn't it be better making it up a (static?) helper (with
comments, hopefully) something like a kthread_is_alive() ?


> +		clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
> +		if (test_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
> +			if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
> +				__kthread_bind(k, kthread->cpu);
> +			wake_up_process(k);
> +		}
> +	}
> +	put_task_struct(k);
> +}
> +
> +/**
> + * kthread_park - park a thread created by kthread_create().
> + * @k: thread created by kthread_create().
> + *
> + * Sets kthread_should_park() for @k to return true, wakes it, and
> + * waits for it to return. This can also be called after kthread_create()
> + * instead of calling wake_up_process(): the thread will park without
> + * calling threadfn().
> + *
> + * Returns 0 if the thread is parked, -ENOSYS if the thread exited.
> + * If called by the kthread itself just the park bit is set.
> + */
> +int kthread_park(struct task_struct *k)
> +{
> +	struct kthread *kthread;
> +	int ret = -ENOSYS;
> +
> +	get_task_struct(k);
> +
> +	kthread = to_kthread(k);
> +	barrier(); /* it might have exited */
> +	if (k->vfork_done != NULL) {
> +		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);
> +			}
> +		}
> +		ret = 0;
> +	}
> +	put_task_struct(k);
> +	return ret;
> +}
> +
> +/**
>   * kthread_stop - stop a thread created by kthread_create().
>   * @k: thread created by kthread_create().
>   *
> @@ -259,7 +398,7 @@ int kthread_stop(struct task_struct *k)
>  	kthread = to_kthread(k);
>  	barrier(); /* it might have exited */
>  	if (k->vfork_done != NULL) {
> -		kthread->should_stop = 1;
> +		set_bit(KTHREAD_SHOULD_STOP, &kthread->flags);

I wonder whether it also needs to unpark @k. Isn't it possible to stop a
parked thread?

Thanks,
Namhyung


>  		wake_up_process(k);
>  		wait_for_completion(&kthread->exited);
>  	}