Re: [PATCH 1/2] kthread_create

Andrew Morton
2003-12-31T04:48:57Z

Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> Hi all,
>
> Ingo read through this before and liked it: this is the basis
> of the Hotplug CPU patch, and as such has been stressed fairly well.
> Tested stand-alone, and included here for wider review.

It would be nice to be able to see all the hotplug CPU patches in one
place, to get a feel for their shape and size. That way, we can decide
whether we need to look at this patch ;)

> D: kthread_create(), kthread_start() and kthread_destroy(). These

A few things:

> +static struct kt_message ktm_receive(void)
> +{
> + struct kt_message m;
> +
> + for (;;) {
> + spin_lock(&ktm_lock);
> + if (ktm.to == current)
> + break;
> + current->state = TASK_INTERRUPTIBLE;
> + spin_unlock(&ktm_lock);
> + schedule();
> + }

If the calling task has a signal pending, this could become a tight loop?

> +
> +static int kthread(void *data)
> +{
> + /* Copy data: it's on keventd_init's stack */
> + struct kthread k = *(struct kthread *)data;
> + struct kt_message m;
> + int ret = 0;
> + sigset_t blocked;
> +
> + strcpy(current->comm, k.name);
> +
> + /* Block and flush all signals. */
> + sigfillset(&blocked);
> + sigprocmask(SIG_BLOCK, &blocked, NULL);
> + flush_signals(current);
> +

deamonize() was not suitable here?

> + /* Send to spawn_kthread, so it knows who we are. */
> + ktm_send(ktm.info, current);
> +
> + /* Receive from kthread_start or kthread_destroy */
> + m = ktm_receive();
> + if (!m.info)
> + goto stop;
> + if (k.initfn && (ret = k.initfn(k.data)) < 0)
> + goto stop;
> + ktm_send(m.from, current);
> +
> + for (;;) {
> + if (time_to_die(&m))
> + break;
> +
> + /* If it fails, just wait until kthread_destroy. */
> + if (k.corefn && (ret = k.corefn(k.data)) < 0)
> + k.corefn = NULL;
> +
> + if (time_to_die(&m))
> + break;
> +
> + schedule();
> + }

In what state is this schedule() called? If it's TASK_RUNNING (or
TASK_INTERRUPTIBLE with signal_pending()) and this task has rt priority
higher than the thing it is waiting for we could have a problem?

> +
> + current->state = TASK_RUNNING;
> +stop:
> + ktm_send(m.from, ERR_PTR(ret));
> + return ret;
> +}
> +
> +struct kthread_create
> +{
> + struct task_struct *result;
> + struct kthread k;
> + struct completion done;
> +};
> +

`kthread_create' sounds like the name of a function to me, not a structure.

> +struct task_struct *kthread_create(int (*initfn)(void *data),

I was right! ;)

It would be nice to kerneldocify kthread_create(), kthread_start() and
kthread_destroy() sometime.

> +static void wait_for_death(struct task_struct *k)
> +{
> + while (!(k->state & TASK_ZOMBIE) && !(k->state & TASK_DEAD))
> + yield();
> +}
> +

If the calling task has higher rt priority than *k, could this not become a
busy loop? It would be preferable to use a real sleep/wait primitive here.