From: Oleg Nesterov <oleg@redhat.com> To: Kay Sievers <kay.sievers@vrfy.org> Cc: Lennart Poettering <mzxreary@0pointer.de>, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, linux-man@vger.kernel.org, roland@hack.frob.com, torvalds@linux-foundation.org Subject: Re: + prctl-add-pr_setget_child_reaper-to-allow-simple-process-supervision .patch added to -mm tree Date: Fri, 19 Aug 2011 14:25:03 +0200 [thread overview] Message-ID: <20110819122503.GA8411@redhat.com> (raw) In-Reply-To: <1313717521.991.4.camel@mop> On 08/19, Kay Sievers wrote: > > On Thu, 2011-08-18 at 20:48 +0200, Oleg Nesterov wrote: > > On 08/18, Kay Sievers wrote: > > > No, this doesn't look right. > > > > This code should do something like > > > > for (reaper = father->real_parent; > > !same_thread_group(reaper, pid_ns->child_reaper); > > Without that check, bootup immediately hangs. The problem is, I expect, > that we need to exit the loop for re-parenting kernel threads, Argh. Indeed, I forgot about kthreads. See below. > - optimization: let processes inherit a flag to indicate that there is > a subreaper to lookup, in case they need to be re-parented. I'll write another email about this... > static struct task_struct *find_new_reaper(struct task_struct *father) > __releases(&tasklist_lock) > @@ -724,6 +725,23 @@ static struct task_struct *find_new_reap > * forget_original_parent() must move them somewhere. > */ > pid_ns->child_reaper = init_pid_ns.child_reaper; > + } else if (father->signal->has_child_subreaper) { > + struct task_struct *reaper; > + > + /* find the first ancestor marked as child_subreaper */ > + for (reaper = father->real_parent; > + reaper != reaper->real_parent; This looks mysterious. This relies on the fact that INIT_TASK(tsk) sets .real_parent = tsk. "reaper != &init_task" looks much more clean. And we can't use PF_KTHREAD because of usermodehelper. But. Now that you check ->has_child_subreaper before the lookup, this problem should go away? I mean, if ->has_child_subreaper == T then some of our parents is the userspace task. Even if it was spawned by kthread and then exited, we can't miss ->child_reaper in the parents chain. Or I missed something? > + if (!reaper->signal->is_child_subreaper) > + continue; > + thread = reaper; > + do { > + if (!(thread->flags & PF_EXITING)) > + return reaper; > + } while_each_thread(reaper, thread); Yes, this looks correct. > + case PR_SET_CHILD_SUBREAPER: > + me->signal->is_child_subreaper = !!arg2; > + me->signal->has_child_subreaper = true; Hmm. This looks wrong... why do we set ->has_child_subreaper? Oleg.
WARNING: multiple messages have this Message-ID (diff)
From: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> To: Kay Sievers <kay.sievers-tD+1rO4QERM@public.gmane.org> Cc: Lennart Poettering <mzxreary-uLTowLwuiw4b1SvskN2V4Q@public.gmane.org>, akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-man-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, roland-/Z5OmTQCD9xF6kxbq+BtvQ@public.gmane.org, torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org Subject: Re: + prctl-add-pr_setget_child_reaper-to-allow-simple-process-supervision .patch added to -mm tree Date: Fri, 19 Aug 2011 14:25:03 +0200 [thread overview] Message-ID: <20110819122503.GA8411@redhat.com> (raw) In-Reply-To: <1313717521.991.4.camel@mop> On 08/19, Kay Sievers wrote: > > On Thu, 2011-08-18 at 20:48 +0200, Oleg Nesterov wrote: > > On 08/18, Kay Sievers wrote: > > > No, this doesn't look right. > > > > This code should do something like > > > > for (reaper = father->real_parent; > > !same_thread_group(reaper, pid_ns->child_reaper); > > Without that check, bootup immediately hangs. The problem is, I expect, > that we need to exit the loop for re-parenting kernel threads, Argh. Indeed, I forgot about kthreads. See below. > - optimization: let processes inherit a flag to indicate that there is > a subreaper to lookup, in case they need to be re-parented. I'll write another email about this... > static struct task_struct *find_new_reaper(struct task_struct *father) > __releases(&tasklist_lock) > @@ -724,6 +725,23 @@ static struct task_struct *find_new_reap > * forget_original_parent() must move them somewhere. > */ > pid_ns->child_reaper = init_pid_ns.child_reaper; > + } else if (father->signal->has_child_subreaper) { > + struct task_struct *reaper; > + > + /* find the first ancestor marked as child_subreaper */ > + for (reaper = father->real_parent; > + reaper != reaper->real_parent; This looks mysterious. This relies on the fact that INIT_TASK(tsk) sets .real_parent = tsk. "reaper != &init_task" looks much more clean. And we can't use PF_KTHREAD because of usermodehelper. But. Now that you check ->has_child_subreaper before the lookup, this problem should go away? I mean, if ->has_child_subreaper == T then some of our parents is the userspace task. Even if it was spawned by kthread and then exited, we can't miss ->child_reaper in the parents chain. Or I missed something? > + if (!reaper->signal->is_child_subreaper) > + continue; > + thread = reaper; > + do { > + if (!(thread->flags & PF_EXITING)) > + return reaper; > + } while_each_thread(reaper, thread); Yes, this looks correct. > + case PR_SET_CHILD_SUBREAPER: > + me->signal->is_child_subreaper = !!arg2; > + me->signal->has_child_subreaper = true; Hmm. This looks wrong... why do we set ->has_child_subreaper? Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2011-08-19 12:28 UTC|newest] Thread overview: 63+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-08-16 20:11 + prctl-add-pr_setget_child_reaper-to-allow-simple-process-supervision.patch added to -mm tree akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b 2011-08-17 11:55 ` + prctl-add-pr_setget_child_reaper-to-allow-simple-process-supervision .patch " Oleg Nesterov 2011-08-17 11:55 ` Oleg Nesterov 2011-08-17 13:05 ` Oleg Nesterov 2011-08-17 13:05 ` Oleg Nesterov 2011-08-17 13:21 ` Kay Sievers 2011-08-17 13:21 ` Kay Sievers 2011-08-17 13:37 ` Alan Cox 2011-08-17 13:37 ` Alan Cox 2011-08-23 0:30 ` Colin Walters 2011-08-23 0:30 ` Colin Walters 2011-08-17 14:16 ` Oleg Nesterov 2011-08-17 14:16 ` Oleg Nesterov 2011-08-17 16:03 ` Denys Vlasenko 2011-08-17 16:03 ` Denys Vlasenko 2011-08-17 13:13 ` Kay Sievers 2011-08-17 13:45 ` Oleg Nesterov 2011-08-17 13:45 ` Oleg Nesterov 2011-08-17 15:45 ` Kay Sievers 2011-08-17 15:45 ` Kay Sievers 2011-08-17 15:53 ` Alan Cox 2011-08-17 15:53 ` Alan Cox 2011-08-17 16:20 ` Oleg Nesterov 2011-08-17 16:20 ` Oleg Nesterov 2011-08-17 16:47 ` Kay Sievers 2011-08-17 16:47 ` Kay Sievers 2011-08-17 18:57 ` Oleg Nesterov 2011-08-17 18:57 ` Oleg Nesterov 2011-08-17 20:56 ` Kay Sievers 2011-08-17 20:56 ` Kay Sievers 2011-08-18 12:43 ` Lennart Poettering 2011-08-18 12:43 ` Lennart Poettering 2011-08-18 14:25 ` Oleg Nesterov 2011-08-18 14:25 ` Oleg Nesterov 2011-08-18 18:11 ` Kay Sievers 2011-08-18 18:48 ` Oleg Nesterov 2011-08-18 18:48 ` Oleg Nesterov 2011-08-19 1:31 ` Kay Sievers 2011-08-19 1:31 ` Kay Sievers 2011-08-19 12:25 ` Oleg Nesterov [this message] 2011-08-19 12:25 ` Oleg Nesterov 2011-08-19 12:44 ` Kay Sievers 2011-08-19 12:44 ` Kay Sievers 2011-08-19 13:13 ` Oleg Nesterov 2011-08-19 13:13 ` Oleg Nesterov 2011-08-19 14:20 ` Kay Sievers 2011-08-19 14:58 ` Oleg Nesterov 2011-08-19 14:58 ` Oleg Nesterov 2011-08-20 15:33 ` Oleg Nesterov 2011-08-20 15:33 ` Oleg Nesterov 2011-08-21 18:33 ` Kay Sievers 2011-08-22 11:14 ` Oleg Nesterov 2011-08-22 11:14 ` Oleg Nesterov 2011-08-22 23:48 ` Kay Sievers 2011-08-22 23:48 ` Kay Sievers 2011-08-18 21:23 ` Linus Torvalds 2011-08-18 21:23 ` Linus Torvalds 2011-08-18 21:55 ` Kay Sievers 2011-08-18 21:55 ` Kay Sievers 2011-08-18 22:22 ` Linus Torvalds 2011-08-18 22:22 ` Linus Torvalds 2011-08-19 0:48 ` Kay Sievers 2011-08-19 0:48 ` Kay Sievers
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20110819122503.GA8411@redhat.com \ --to=oleg@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=kay.sievers@vrfy.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-man@vger.kernel.org \ --cc=mzxreary@0pointer.de \ --cc=roland@hack.frob.com \ --cc=torvalds@linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.