Re: [PATCH 0/1] kmod: don't run async usermode helper as a child of kworker thread

[Frederic Weisbecker]

On Thu, Oct 15, 2015 at 06:32:17PM +0200, Oleg Nesterov wrote:
> 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().

Sure. But kmod does special things with signals.

> 
> > > @@ -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.

I don't mind it, it's just that it's not obvious _why_ we don't want to
pollute it. I think that's what is missing in the comment. At least I
couldn't deduce it by myself without you explaining.

> 
> 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().

Yeah that would be great. Something that lets us wait for a task completion
without bothering with signals.

> 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.

Ok.

> 
> Finally, this comment should go into kernel_thread() eventually.

Agreed, as long as reviewers really understand what we are doing in usermodehelper.
It took me several days to understand why we were doing things the way we did before
updating all the comments recently. The flags, the workqueues, the kernel threads...
Not much is intuitive there. Of course usermodehelper isn't doing intuitive things, hence why.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/