On Wed, 26 Oct 2016, Oleg Nesterov wrote: > Some notes right now. Of course, with this patch we are ready to remove > put_task_stack() from kthread.c right now. The next change should kill > to_live_kthread() altogether. And stop using ->vfork_done. > > And. With this patch we do not need another "workqueue: ignore dead tasks > in a workqueue sleep hook" fix from Roman.
Nice ! > diff --git a/kernel/kthread.c b/kernel/kthread.c > index be2cc1f..c6adbde 100644 > --- a/kernel/kthread.c > +++ b/kernel/kthread.c > @@ -53,14 +53,34 @@ enum KTHREAD_BITS { > KTHREAD_IS_PARKED, > }; > > -#define __to_kthread(vfork) \ > - container_of(vfork, struct kthread, exited) > +static inline void set_kthread_struct(void *kthread) > +{ > + /* > + * We abuse ->set_child_tid to avoid the new member and because it > + * can't be wrongly copied by copy_process(). We also rely on fact > + * that the caller can't exec, so PF_KTHREAD can't be cleared. > + */ > + current->set_child_tid = (__force void __user *)kthread; Can we pretty please avoid this type casting? We only have 5 places using set_child_tid. So we can really make it a proper union and fix up the 5 usage sites as a preparatory patch for this. > +} > > static inline struct kthread *to_kthread(struct task_struct *k) > { > - return __to_kthread(k->vfork_done); > + WARN_ON(!(k->flags & PF_KTHREAD)); > + return (__force void *)k->set_child_tid; > } > > +void free_kthread_struct(struct task_struct *k) > +{ > + kfree(to_kthread(k)); /* can be NULL if kmalloc() failed */ Can you please not use tail comments? They really stop the reading flow. > +#define __to_kthread(vfork) \ > + container_of(vfork, struct kthread, exited) > + > +/* > + * TODO: kill it and use to_kthread(). But we still need the users > + * like kthread_stop() which has to sync with the exiting kthread. > + */ > static struct kthread *to_live_kthread(struct task_struct *k) > { > struct completion *vfork = ACCESS_ONCE(k->vfork_done); > @@ -181,14 +201,11 @@ static int kthread(void *_create) > int (*threadfn)(void *data) = create->threadfn; > void *data = create->data; > struct completion *done; > - struct kthread self; > + struct kthread *self; > int ret; > > - self.flags = 0; > - self.data = data; > - init_completion(&self.exited); > - init_completion(&self.parked); > - current->vfork_done = &self.exited; > + self = kmalloc(sizeof(*self), GFP_KERNEL); > + set_kthread_struct(self); > > /* If user was SIGKILLed, I release the structure. */ > done = xchg(&create->done, NULL); > @@ -196,6 +213,19 @@ static int kthread(void *_create) > kfree(create); > do_exit(-EINTR); > } > + > + if (!self) { > + create->result = ERR_PTR(-ENOMEM); > + complete(done); > + do_exit(-ENOMEM); > + } > + > + self->flags = 0; > + self->data = data; > + init_completion(&self->exited); > + init_completion(&self->parked); > + current->vfork_done = &self->exited; > + > /* OK, tell user we're spawned, wait for stop or wakeup */ > __set_current_state(TASK_UNINTERRUPTIBLE); > create->result = current; > @@ -203,12 +233,10 @@ static int kthread(void *_create) > schedule(); > > ret = -EINTR; > - > - if (!test_bit(KTHREAD_SHOULD_STOP, &self.flags)) { > - __kthread_parkme(&self); > + if (!test_bit(KTHREAD_SHOULD_STOP, &self->flags)) { > + __kthread_parkme(self); > ret = threadfn(data); > } > - /* we can't just return, we must preserve "self" on stack */ > do_exit(ret); > } Other than the above nits, this is the right direction to go. Thanks, tglx