Re: + prctl-add-pr_setget_child_reaper-to-allow-simple-process-supervision.patch added to -mm tree

[Kay Sievers]

On Wed, Aug 17, 2011 at 20:57, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> On 08/17, Kay Sievers wrote:
>> 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.

I guess parents usually prefer to pick up their children themselves
that ran away from home, instead of getting calls from somebody else
telling them that they already took care of everything. :)

The most prominent issue with async messages is the re-use of the same
keys/ids they carry, during the window the event happened and the
async message is received. If we reap PIDs ourselves, the PID is
pinned until we take care of it, it can not be another process be
created in the meantime with the same PID.

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

Right, that's what I meant. And what can be avoided if we move the
parent walk below the current check.

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

Yeah, sorry, it sounds confusing, but I think we arrived now for that
specific part. :)

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

Yeah, it is before, but should be after.

> 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;
> Â Â Â Â}

Yeah, clear now, it looks better to keep it in that order.

Thanks,
Kay
--
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/