From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759134AbcJYQIy (ORCPT ); Tue, 25 Oct 2016 12:08:54 -0400 Received: from mail-oi0-f46.google.com ([209.85.218.46]:34050 "EHLO mail-oi0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754958AbcJYQIw (ORCPT ); Tue, 25 Oct 2016 12:08:52 -0400 MIME-Version: 1.0 In-Reply-To: <20161025154301.GA12015@redhat.com> References: <20161025110508.9052-1-roman.penyaev@profitbricks.com> <20161025140333.GB4326@redhat.com> <20161025154301.GA12015@redhat.com> From: Roman Penyaev Date: Tue, 25 Oct 2016 18:08:31 +0200 Message-ID: Subject: Re: [PATCH v3 1/1] kthread: allocate kthread structure using kmalloc To: Oleg Nesterov Cc: Andy Lutomirski , Peter Zijlstra , Thomas Gleixner , Ingo Molnar , Tejun Heo , linux-kernel@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 25, 2016 at 5:43 PM, Oleg Nesterov 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