From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753583AbbJOQfp (ORCPT ); Thu, 15 Oct 2015 12:35:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58210 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751374AbbJOQfo (ORCPT ); Thu, 15 Oct 2015 12:35:44 -0400 Date: Thu, 15 Oct 2015 18:32:17 +0200 From: Oleg Nesterov To: Frederic Weisbecker Cc: Andrew Morton , Rik van Riel , Christoph Lameter , Tejun Heo , Rusty Russell , linux-kernel@vger.kernel.org Subject: Re: [PATCH 0/1] kmod: don't run async usermode helper as a child of kworker thread Message-ID: <20151015163217.GA25118@redhat.com> References: <20151014185153.GA8117@redhat.com> <20151015143739.GA20060@redhat.com> <20151015143757.GB20060@redhat.com> <20151015155255.GE12822@lerouge> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151015155255.GE12822@lerouge> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/15, Frederic Weisbecker wrote: > > On Thu, Oct 15, 2015 at 04:37:57PM +0200, Oleg Nesterov wrote: > > call_usermodehelper_exec_sync() does fork() + wait() with "unignored" > > SIGCHLD. What we have missed is that this worker thread can have other > > children previously forked by call_usermodehelper_exec_work() without > > UMH_WAIT_PROC. If such a child exits in between it becomes a zombie and > > nobody can reap it (unless/until this worker thread exits too). > > I think we should elaborate a tiny bit the last sentence here: OK, I'll try to update the changelog and send v2... > "When the parent masks SIGCHLD, a child autoreaps itself, this is > what we expect from !UMH_WAIT_PROC children. ^^^^^^^^^^^^^^^^^^^^^^^ Not really. This is what we _usually_ expect from kernel_thread(). > > @@ -327,9 +327,13 @@ static void call_usermodehelper_exec_work(struct work_struct *work) > > call_usermodehelper_exec_sync(sub_info); > > } else { > > pid_t pid; > > - > > + /* > > + * Use CLONE_PARENT to reparent it to kthreadd; we do not > > + * want to pollute current->children, in particular because > > + * call_usermodehelper_exec_sync() assumes it is empty. > > + */ > > IMHO, that too should get some more details. Maybe: > > + /* > + * Use CLONE_PARENT to reparent it to kthreadd. We need a parent > + * that always ignore SIGCHLD such that the child always autoreaps > + * as expected. > + */ Well, OK... But I would like to keep "we do not want to pollute current->children" because this the goal of the next cleanups. Plus I don't really like "parent that always ignore SIGCHLD". To remind, we can also remove kernel_sigaction() and sys_wait4() from call_usermodehelper_exec_sync(). Plus I have other changes in mind, kernel_thread() should not rely on SIGCHLD at all. The auto-reapable kernel threads should run with ->exit_signal == 0. Finally, this comment should go into kernel_thread() eventually. Oleg.