Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933759AbcJZOOG (ORCPT ); Wed, 26 Oct 2016 10:14:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59776 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754814AbcJZOOD (ORCPT ); Wed, 26 Oct 2016 10:14:03 -0400 Date: Wed, 26 Oct 2016 16:14:00 +0200 From: Oleg Nesterov To: Andy Lutomirski Cc: Roman Pen , Andy Lutomirski , Peter Zijlstra , Thomas Gleixner , Ingo Molnar , Tejun Heo , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v3 1/1] kthread: allocate kthread structure using kmalloc Message-ID: <20161026141359.GA6893@redhat.com> References: <20161025110508.9052-1-roman.penyaev@profitbricks.com> <20161025140333.GB4326@redhat.com> <20161025154301.GA12015@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Wed, 26 Oct 2016 14:14:03 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4746 Lines: 156 On 10/25, Andy Lutomirski wrote: > > Would it perhaps make sense to do something like Roman's patch for 4.9 > and then consider further changes down the road? OK, lets make it kmalloc'ed. Please see my old patch below, slightly changed. I'll send it after I do some testing and write the changelog. No separate refcounting, no complications. 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. > Roman's patch > appears to fix a real bug, Well, it fixes the additional problems if we already have a bug, but I agree this is a problem anyway. > and I think that, while not really ideal, > the code is an incredible mess right now and Roman's patch (assuming > it's correct) makes it considerably nicer. This is where I disagree with you and Roman. Yes, it needs cleanups and only because of kthread on stack. But _IMO_ Roman's patch makes it much, much worse and adds a lot of unnecessary complications. Could you please look at the patch below? Oleg. --- diff --git a/include/linux/kthread.h b/include/linux/kthread.h index a6e82a6..c1c3e63 100644 --- a/include/linux/kthread.h +++ b/include/linux/kthread.h @@ -48,6 +48,7 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data), __k; \ }) +void free_kthread_struct(struct task_struct *k); void kthread_bind(struct task_struct *k, unsigned int cpu); void kthread_bind_mask(struct task_struct *k, const struct cpumask *mask); int kthread_stop(struct task_struct *k); diff --git a/kernel/fork.c b/kernel/fork.c index 623259f..663c6a7 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -351,6 +351,8 @@ void free_task(struct task_struct *tsk) ftrace_graph_exit_task(tsk); put_seccomp_filter(tsk); arch_release_task_struct(tsk); + if (tsk->flags & PF_KTHREAD) + free_kthread_struct(tsk); free_task_struct(tsk); } EXPORT_SYMBOL(free_task); 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; +} 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 */ +} + +#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); }