Messages in this thread | | | Date | Fri, 19 Aug 2011 14:25:03 +0200 | From | Oleg Nesterov <> | Subject | Re: + prctl-add-pr_setget_child_reaper-to-allow-simple-process-supervision .patch added to -mm tree |
| |
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.
| |