Messages in this thread | ![/](/images/icornerl.gif) | | From | Rusty Russell <> | Subject | Re: [PATCH 1/2] kthread_create | Date | Wed, 31 Dec 2003 16:18:10 +1100 |
| |
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| ![\](/images/icornerr.gif) |