Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1247077 > unrolled thread

[PATCH 0/1] Revert "kmod: handle UMH_WAIT_PROC from system unbound workqueue"

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/1] Revert "kmod: handle UMH_WAIT_PROC from system unbound  workqueue" Oleg Nesterov <oleg@redhat.com> - 2015-10-14 21:00 +0200
    Re: [PATCH 0/1] kmod: don't run async usermode helper as a child  of kworker thread Oleg Nesterov <oleg@redhat.com> - 2015-10-15 16:50 +0200
      Re: [PATCH 0/1] kmod: don't run async usermode helper as a child of  kworker thread Frederic Weisbecker <fweisbec@gmail.com> - 2015-10-15 18:00 +0200
        Re: [PATCH 0/1] kmod: don't run async usermode helper as a child  of kworker thread Oleg Nesterov <oleg@redhat.com> - 2015-10-15 18:40 +0200
          Re: [PATCH 0/1] kmod: don't run async usermode helper as a child of  kworker thread Frederic Weisbecker <fweisbec@gmail.com> - 2015-10-15 19:00 +0200
            Re: [PATCH 0/1] kmod: don't run async usermode helper as a child  of kworker thread Oleg Nesterov <oleg@redhat.com> - 2015-10-15 20:00 +0200
    [PATCH 0/1] kmod: don't run async usermode helper as a child of  kworker thread Oleg Nesterov <oleg@redhat.com> - 2015-10-15 16:50 +0200
      [PATCH v2 1/1] kmod: don't run async usermode helper as a child of  kworker thread Oleg Nesterov <oleg@redhat.com> - 2015-10-15 20:00 +0200

#1247077 — [PATCH 0/1] Revert "kmod: handle UMH_WAIT_PROC from system unbound workqueue"

Hello,

I noticed by accident the kworker zombies on my testing machine.
Can't reproduce (although I think it won't be hard to make a
test-case), but I think the reason is clear, see the changelog.

We could fix this by using kthread_create() if !UMH_WAIT_PROC,
but imo it would be better to revert this change at least for
now. If we really want to avoid the extra kernel_thread(), we
can make another patch which also avoids sys_wait4() and the
games with SIGCHLD; we can rely on wait_chldexit.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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

Change the !UMH_WAIT_PROC case to use CLONE_PARENT.

Note: this is only first step. All PF_KTHREAD tasks, even created by
kernel_thread() should have ->parent == kthreadd by default.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/kmod.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index da98d05..e7185a2 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -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.
+		 */
 		pid = kernel_thread(call_usermodehelper_exec_async, sub_info,
-				    SIGCHLD);
+				    CLONE_PARENT | SIGCHLD);
 		if (pid < 0) {
 			sub_info->retval = pid;
 			umh_complete(sub_info);
-- 
2.4.3


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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:

"When the parent masks SIGCHLD, a child autoreaps itself, this is
what we expect from !UMH_WAIT_PROC children. Now if such a child exits during
this unlucky window where the parent worker enabled SIGCHLD to wait for a sibling,
the autoreap will fail and the child then becomes a zombie because  nobody can reap it
(unless/until this worker thread exits too)."

> 
> Change the !UMH_WAIT_PROC case to use CLONE_PARENT.
> 
> Note: this is only first step. All PF_KTHREAD tasks, even created by
> kernel_thread() should have ->parent == kthreadd by default.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/kmod.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index da98d05..e7185a2 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -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.
 +		 */

Thanks!

>  		pid = kernel_thread(call_usermodehelper_exec_async, sub_info,
> -				    SIGCHLD);
> +				    CLONE_PARENT | SIGCHLD);
>  		if (pid < 0) {
>  			sub_info->retval = pid;
>  			umh_complete(sub_info);
> -- 
> 2.4.3
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1247995 — 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248050 — 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 06:32:17PM +0200, Oleg Nesterov wrote:
> >
> > 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.

Well, this connects to the "Note:" part of the changelog, I don't
this the comment can really explain this.

But note that this fix is not enough in the long term _unless_ we
ensure that another workueue callback can not call kernel_thread()
without CLONE_PARENT. And this is what I am going to do.

Nevermind, this is (almost) off-topic right now. Please review V2
I'll send in a minute, I tried to update the comment/changelog.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

Andrew, please drop

	revert-kmod-handle-umh_wait_proc-from-system-unbound-workqueue.patch

I sent yesterday. On a second thought we have a better solution.

On 10/14, Oleg Nesterov wrote:
>
> Hello,
>
> I noticed by accident the kworker zombies on my testing machine.
> Can't reproduce (although I think it won't be hard to make a
> test-case), but I think the reason is clear, see the changelog.
>
> We could fix this by using kthread_create() if !UMH_WAIT_PROC,
> but imo it would be better to revert this change at least for
> now.

I changed my mind. I was worried about other workqueue callbacks
which could abuse kernel_thread() and populate kworker->children
even if we change call_usermodehelper_exec_work() to not do this.

But according to git-grep nobody does this. And this is good!
Because we can do more cleanups (will try to send "soon") to
ensure that all kthreads have parent == kthreadd.

And since the worker thread is already its child, we do not need
kthread_create(), we can just use CLONE_PARENT (which should be
later used by kernel_thread() by default).

> If we really want to avoid the extra kernel_thread(), we
> can make another patch which also avoids sys_wait4() and the
> games with SIGCHLD; we can rely on wait_chldexit.

Yes, this probably makes sense too, but we can do this regardless.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1248049 — [PATCH v2 1/1] kmod: don't run async usermode helper as a child of kworker thread

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
because auto-reaping only works if SIGCHLD is ignored, and nobody can
reap it (unless/until this worker thread exits too).

Change the !UMH_WAIT_PROC case to use CLONE_PARENT.

Note: this is only first step. All PF_KTHREAD tasks, even created by
kernel_thread() should have ->parent == kthreadd by default.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/kmod.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index da98d05..0277d12 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -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, and we need a parent
+		 * that always ignores SIGCHLD to ensure auto-reaping.
+		 */
 		pid = kernel_thread(call_usermodehelper_exec_async, sub_info,
-				    SIGCHLD);
+				    CLONE_PARENT | SIGCHLD);
 		if (pid < 0) {
 			sub_info->retval = pid;
 			umh_complete(sub_info);
-- 
2.4.3


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web