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

Namhyung Kim
2012-06-14T02:34:59Z

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);
> }