Re: [PATCH 1/2] kthread_create

[Rusty Russell <rusty@rustcorp.com.au>]

In message <20031230204910.0e767b50.akpm@osdl.org> you write:
> 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 ;)

Um, I've had this on kernel.org for a few years now.  It's even at the
top of the page:

	http:://www.kernel.org/pub/linux/kernel/people/rusty

> > +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?

Possibly, but there's not much we can do.  We never wait long, and
we're keventd or a child here, so we're only talking about SIGCHLD.

> > +	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?

No. (1) By design, we're always a purebred kernel thread descended
directly from the init thread and have never had an mm anywhere.  (2)
daemonize is an abhorrent abortion: it's dangerous and presumptive to
try to "clean up" a random thread into a kernel thread.

> > +		/* 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?

Yep, that's by design.  (1) signal_pending() is not possible, we've
blocked all signals above.  (2) the corefn() MUST set the task state,
as per normal semantics:

       1) set current->state to TASK_INTERRUPTIBLE
       2) check condition
       3) return (so core can schedule)

> > +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.

OK, I've changed it to kthread_creation.  It's private to kthread.c
anyway.

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

Sure, if you want.  Does anyone actually read that?  I prefer the
comments on how to use functions belong in the headers, not above the
definitions as seems to be the kerneldoc way.

> > +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.

Hmm, if it's an RT task, it'll screw up, yes, because yield() won't
yield().

Fixing this well would require a way of notifying someone who is not
the parent when a task dies, OR taking over the parenthood of the
task.  Both of these required non-trivial changes to exit.c and I
shied away.

All things are possible, however...

Thanks!
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.