Messages in this thread | | | From | Roman Penyaev <> | Date | Tue, 25 Oct 2016 18:08:31 +0200 | Subject | Re: [PATCH v3 1/1] kthread: allocate kthread structure using kmalloc |
| |
On Tue, Oct 25, 2016 at 5:43 PM, Oleg Nesterov <oleg@redhat.com> wrote: > On 10/25, Oleg Nesterov wrote: >> >> On 10/25, Roman Pen wrote: >> > >> > This patch avoids allocation of kthread structure on a stack, and simply >> > uses kmalloc. >> >> Oh. I didn't even read this patch, but I have to admit I personally do not >> like it. I can be wrong, but imo this is the step to the wrong direction. > > And after I tried to actually read it I dislike it even more, sorry Roman. > Starting from the fact it moves kthread_create_info into struct kthread.
that can be changed, of course, as I told, I wanted to keep allocations/ deallocations simpler.
>> struct kthread is already bloated, we should not bloat it more. Instead >> we should kill it. And to_kthread() too, at least in its current form. > > Yes, but even if we can't or do not want to do this, even if we want to > kmalloc struct kthread, I really think it should not be refcounted > separately from task_struct.
it is already like that, we have to get/put references on a task stack.
> > something like the patch in http://marc.info/?l=linux-kernel&m=146715459127804
the key function in that patch is:
free_kthread_struct(tsk);
so if we teach the generic free_task() to deal with kthreads, that of course solves these kind of problems. I did not consider that variant.
> > Either way to_live_kthread() must go away. Currently we can't avoid it > because we abuse vfork_done, but as I already said we no longer need this.
There is something which I do not understand. You still need to have a connection (a pointer) between task_struct and private data (kthread AND private data, whatever), which is passed by the user of kthread API. You still need to find a victim in a task_struct and abuse it :)
So in particular I do not understand this comment from the patch above where you abuse 'current->set_child_tid':
* This is the ugly but simple hack we will hopefully remove soon.
how you are going to avoid this abuse of set_child_tid? or vfork_done? because vfork_done is not only for waking up (yes, I totally agree, we can reuse task_work), it is also for getting a private data (like workqueue uses it): task_struct->vfork_done->kthread->data.
-- Roman
| |