From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754140AbcJZPAd (ORCPT ); Wed, 26 Oct 2016 11:00:33 -0400 Received: from Galois.linutronix.de ([146.0.238.70]:40388 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751894AbcJZPAc (ORCPT ); Wed, 26 Oct 2016 11:00:32 -0400 Date: Wed, 26 Oct 2016 16:57:42 +0200 (CEST) From: Thomas Gleixner To: Oleg Nesterov cc: Andy Lutomirski , Roman Pen , Andy Lutomirski , Peter Zijlstra , Ingo Molnar , Tejun Heo , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v3 1/1] kthread: allocate kthread structure using kmalloc In-Reply-To: <20161026141359.GA6893@redhat.com> Message-ID: References: <20161025110508.9052-1-roman.penyaev@profitbricks.com> <20161025140333.GB4326@redhat.com> <20161025154301.GA12015@redhat.com> <20161026141359.GA6893@redhat.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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