Messages in this thread Patch in this message | | | Date | Thu, 15 Oct 2015 18:32:17 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 0/1] kmod: don't run async usermode helper as a child of kworker thread |
| |
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.
| |