Messages in this thread Patch in this message | | | Date | Wed, 26 Oct 2016 16:14:00 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH v3 1/1] kthread: allocate kthread structure using kmalloc |
| |
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); }
| |