Re: [PATCH 1/2] kthread_create

[Andrew Morton]

Rusty Russell <rusty@xxxxxxxxxxxxxxx> 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.


-
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/