From: Oleg Nesterov <oleg@redhat.com> To: Kay Sievers <kay.sievers@vrfy.org> Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, lennart@poettering.net, 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: Wed, 17 Aug 2011 20:57:09 +0200 [thread overview] Message-ID: <20110817185709.GA27663@redhat.com> (raw) In-Reply-To: <CAPXgP13Dn2c-OnYg-Cty5r4JbqeH_zYPtXDj5GAfK1btoKYmDg@mail.gmail.com> On 08/17, Kay Sievers wrote: > > On Wed, Aug 17, 2011 at 18:20, Oleg Nesterov <oleg@redhat.com> wrote: > > On 08/17, Kay Sievers wrote: > > >> No, we want to be the parent of the process, > >> ... > >> The sub-init is the babysitter of all the things it has > >> started, and that should be reflected in the parent child relation. > > > > OK. But could you explain why do we want this? This is not clear from > > the changelog/discussion. > > As said, PID1 has the privilege of reaping all processes that > double-fork. Any sub-init wants to do the same for the stuff it > watches. The process that reaps has all the information about the > process as long as wants, it can look up stuff in /proc if needed or > has all the return values of wait(). OK. > Async notifications like > taskstats just can not provide what SIGCHLD, /proc and wait() offer. Why not? Async notifications can't delay the reaping, yes. But I am not sure /proc/ is that useful when the task exits. OK, I won't argue. > >> > You should check ->child_reaper only. But see above, it can be multithreaded. > >> > >> The main PID 1 from the system has no ->child_reaper set as far as I > >> see, hence we check for init_task. > > > > No, you don't. > > Don't? I don't check, or we don't have it set? Argh, sorry. "No, this is not needed", this is what I tried to say. > > Once again, if pid_ns->child_reaper exits, you should > > not even try to find the sub-reaper in its parents chain. > > That would mean we can never run a sub-init in a pid namespace? Why not? > > Or do you mean that we *are* already the pid_ns->child_reaper, not > that one *exists*? I already got lost a bit, not sure I understand. I meant we *are* (the caller) the exiting pid_ns->child_reaper thread. > >> >> > Also. You shouldn't do this if the sub-namespace init exits, this is > >> >> > wrong. > >> >> > >> >> It we find a sub-init, before the namespace PID1, why wouldn't we return it? > >> > > >> > Ah, I meant pid_ns->child_reaper, not task->child_reaper. > >> > > >> > If pid_ns->child_reaper exits we should never try to "reparent" its > >> > children, see zap_pid_ns_processes() in particular. IOW, this should > >> > go into the "else" branch of "if (pid_ns->child_reaper == father)" > >> > >> I don't understand this. If we find a marked task->child_reaper > >> _before_ we find a pid_ns->child_reaper in the chain of parents, > > > > This is fine. > > > > OK. I guess I wasn't clear, and I do not know how to explaine better. > > Please look at your code ;) Suppose that a sub-namespace init exits. > > Not the global /sbin/init. Not the caller of prctl(REAPER). > > > > In this case we should kill the children, not reparent them. Or panic > > if it is the global init (see above). > > > > See? > > Not sure. You mean that the lookup for a possible task->cild_reaper > should be _before_ the check for pid_ns->child_reaper == father which > is currently below? Hmm. I am even more confused. So. I actually applied your patch. The code is for (reaper = father->parent; reaper != &init_task && reaper != pid_ns->child_reaper; reaper = reaper->parent) if (reaper->child_reaper) return reaper; if (unlikely(pid_ns->child_reaper == father)) { ... The lookup is already _before_ the check for pid_ns->child_reaper == father, what do you mean? And, ignoring the mt problems I mentioned, I mean we should do if (pid_ns->child_reaper == father) { panic_or_kill_this_namespace; // the current code } else { for (reaper = father->parent; reaper != pid_ns->child_reaper; reaper = reaper->parent) if (reaper->child_reaper) return reaper; } 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: akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, lennart-mdGvqq1h2p+GdvJs77BJ7Q@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: Wed, 17 Aug 2011 20:57:09 +0200 [thread overview] Message-ID: <20110817185709.GA27663@redhat.com> (raw) In-Reply-To: <CAPXgP13Dn2c-OnYg-Cty5r4JbqeH_zYPtXDj5GAfK1btoKYmDg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> On 08/17, Kay Sievers wrote: > > On Wed, Aug 17, 2011 at 18:20, Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > > On 08/17, Kay Sievers wrote: > > >> No, we want to be the parent of the process, > >> ... > >> The sub-init is the babysitter of all the things it has > >> started, and that should be reflected in the parent child relation. > > > > OK. But could you explain why do we want this? This is not clear from > > the changelog/discussion. > > As said, PID1 has the privilege of reaping all processes that > double-fork. Any sub-init wants to do the same for the stuff it > watches. The process that reaps has all the information about the > process as long as wants, it can look up stuff in /proc if needed or > has all the return values of wait(). OK. > Async notifications like > taskstats just can not provide what SIGCHLD, /proc and wait() offer. Why not? Async notifications can't delay the reaping, yes. But I am not sure /proc/ is that useful when the task exits. OK, I won't argue. > >> > You should check ->child_reaper only. But see above, it can be multithreaded. > >> > >> The main PID 1 from the system has no ->child_reaper set as far as I > >> see, hence we check for init_task. > > > > No, you don't. > > Don't? I don't check, or we don't have it set? Argh, sorry. "No, this is not needed", this is what I tried to say. > > Once again, if pid_ns->child_reaper exits, you should > > not even try to find the sub-reaper in its parents chain. > > That would mean we can never run a sub-init in a pid namespace? Why not? > > Or do you mean that we *are* already the pid_ns->child_reaper, not > that one *exists*? I already got lost a bit, not sure I understand. I meant we *are* (the caller) the exiting pid_ns->child_reaper thread. > >> >> > Also. You shouldn't do this if the sub-namespace init exits, this is > >> >> > wrong. > >> >> > >> >> It we find a sub-init, before the namespace PID1, why wouldn't we return it? > >> > > >> > Ah, I meant pid_ns->child_reaper, not task->child_reaper. > >> > > >> > If pid_ns->child_reaper exits we should never try to "reparent" its > >> > children, see zap_pid_ns_processes() in particular. IOW, this should > >> > go into the "else" branch of "if (pid_ns->child_reaper == father)" > >> > >> I don't understand this. If we find a marked task->child_reaper > >> _before_ we find a pid_ns->child_reaper in the chain of parents, > > > > This is fine. > > > > OK. I guess I wasn't clear, and I do not know how to explaine better. > > Please look at your code ;) Suppose that a sub-namespace init exits. > > Not the global /sbin/init. Not the caller of prctl(REAPER). > > > > In this case we should kill the children, not reparent them. Or panic > > if it is the global init (see above). > > > > See? > > Not sure. You mean that the lookup for a possible task->cild_reaper > should be _before_ the check for pid_ns->child_reaper == father which > is currently below? Hmm. I am even more confused. So. I actually applied your patch. The code is for (reaper = father->parent; reaper != &init_task && reaper != pid_ns->child_reaper; reaper = reaper->parent) if (reaper->child_reaper) return reaper; if (unlikely(pid_ns->child_reaper == father)) { ... The lookup is already _before_ the check for pid_ns->child_reaper == father, what do you mean? And, ignoring the mt problems I mentioned, I mean we should do if (pid_ns->child_reaper == father) { panic_or_kill_this_namespace; // the current code } else { for (reaper = father->parent; reaper != pid_ns->child_reaper; reaper = reaper->parent) if (reaper->child_reaper) return reaper; } 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-17 19:00 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 [this message] 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 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=20110817185709.GA27663@redhat.com \ --to=oleg@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=kay.sievers@vrfy.org \ --cc=lennart@poettering.net \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-man@vger.kernel.org \ --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.