On 28/02/11 9:43 -0500, Oren Laadan wrote:
>
>
> On 02/26/2011 08:54 AM, Louis Rilling wrote:
> > On Fri, Feb 25, 2011 at 11:01:32AM -0800, Sukadev Bhattiprolu wrote:
> >> Louis Rilling [Louis.Rilling@xxxxxxxxxxx] wrote:
> >> | On 24/02/11 23:55 -0800, Sukadev Bhattiprolu wrote:
> >> | >
> >> | > diff --git a/kernel/checkpoint/restart.c b/kernel/checkpoint/restart.c
> >> | > index b0ea8ec..8ecc052 100644
> >> | > --- a/kernel/checkpoint/restart.c
> >> | > +++ b/kernel/checkpoint/restart.c
> >> | > @@ -972,6 +972,7 @@ static int do_ghost_task(void)
> >> | > if (ret < 0)
> >> | > ckpt_err(ctx, ret, "ghost restart failed\n");
> >> | >
> >> | > + current->exit_signal = -1;
> >> |
> >> | Setting ->exit_signal outside of tasklist_lock makes me nervous. All other
> >> | places that change ->exit_signal hold write_lock_irq(&tasklist_lock), and
> >> | eligibile_child() (for an instance of a reader being another task) holds
> >> | read_lock(&tasklist_lock). But maybe this does not matter for ghost tasks.
> >> |
> >>
> >> Yes, an earlier version had the write_lock(&tasklist_lock). Will add it
> >> back.
> >>
> >> | > restore_debug_exit(ctx);
> >> | > ckpt_ctx_put(ctx);
> >> | > do_exit(0);
> >> | > @@ -1465,7 +1466,22 @@ void exit_checkpoint(struct task_struct *tsk)
> >> | > /* restarting zombies will activate next task in restart */
> >> | > if (tsk->flags & PF_RESTARTING) {
> >> | > BUG_ON(ctx->active_pid == -1);
> >> | > +
> >> | > + /*
> >> | > + * if we are a "ghost" task, that was terminated by the
> >> | > + * container-init (from zap_pid_ns_processes()), we should
> >> | > + * wake up the parent since we are now a detached process.
> >> | > + */
> >> | > + read_lock_irq(&tasklist_lock);
> >> |
> >> | read_lock() is enough. tasklist_lock is never taken for write from IRQs or
> >> | softIRQs.
> >> |
> >> | > + if (tsk->exit_state == EXIT_DEAD && !tsk->parent->exit_state) {
> >> | > + ckpt_debug("[%d, %s]: exit_checkpoint(): notifying "
> >> | > + "parent\n", tsk->pid, tsk->comm);
> >> | > + __wake_up_parent(tsk, tsk->parent);
> >> | > + }
> >> | > + read_unlock_irq(&tasklist_lock);
> >> |
> >> | Looking at this closer, I wonder if this wakeup logic should be called from
> >> | do_ghost_task(), right after setting ->exit_signal. This way there would be no
> >> | need for a tricky condition to recognize ghost tasks, and (I think) this is
> >> | closer to the other cases changing ->exit_signal (reparent_leader() and
> >> | exit_notify()).
> >>
> >> Yes, we tried the following in the earlier version.
> >>
> >> void ghost_auto_reapable()
> >> {
> >> write_lock(&tasklist_lock);
> >> current->exit_signal = -1;
> >> __wake_up_parent(current, current->parent);
> >> write_unlock(&tasklist_lock);
> >> }
> >>
> >> And called this from do_ghost_task(). But with this, the parent could
> >> wake up, find that it still has an eligible child (the ghost) to wait
> >> for, and go back to waiting before the ghost enters the EXIT_DEAD state.
> >> And so we would lose the wake up.
> >>
> >> (zap_pid_ns_processes() passes the __WALL so the ghost would be considered
> >> an eligible child).
> >
> > I think I see now. The point is that ->exit_signal = -1 is only meant to work
> > correctly for sub-threads, which the parent does not need to wait for. IOW, the
> > notion of detached task is only implemented for sub-threads.
> >
> > IIUC, setting ->exit_signal to -1 is only used here to let exit_notify() set
> > ->exit_state to EXIT_DEAD, right? Otherwise, setting ->exit_signal to 0 and
> > letting do_notify_parent() proceed for ghost tasks would have be sufficient I
> > guess (provided that the confusion between ghost tasks and zombies could be
> > easily avoided in do_notify_parent()).
> >
> > Then I agree that the proposed patch looks like a reasonably simple approach.
> >
> > Thanks for the explanation,
> >
>
> Louis, Suka:
>
> One subtlety with the method is that if a process get reparented
> (for whatever reason) then the ->exit_signal field is reset to
> SIGCHLD. Fortunately, that should not affect us because our ghost
> tasks never become orphaned.
AFAICS, this is not true for detached tasks: neither in reparent_leader(), nor
in exit_notify(). Unless I missed another place?
>
> However, I can't avoid thinking that maybe there is a better way
> to do this altogether ?
>
> The requirement is simple: ghost tasks are temporary tasks whose
> role is to keep certain pid's alive for the duration of the restart
> and then exit without a trace before the restarted tasks resume
> execution.
>
> The reason I opted for the ->exit_signal = -1 is because it makes
> sure that the parent need not explicitly collect the child ghost.
>
> If I had set it to 0, then it would not send a signal, but still
> would require a wait() to collect it (right ?).
Right.
>
> Can you think of a way to achieve this functionality without the
> subtleties that we have observed so far ? even at the cost of a
> minor change to, say, wait() logic or what not ?
I wonder if things would be easier by providing a mean to distinguish ghost
tasks from truely restarted tasks in do_notify_parent().
Assuming this, there might be a kernel-patch-free way, although I can't say if
this fits userspace restart constraints:
Setting the signal handler of ghost tasks' parent to SIG_IGN, and ->exit_signal
of ghost tasks to SIGCHLD, will make them autoreap. This requires that the
parent does not need to synchronize with other children until all ghost tasks
have exited, and that the parent remains alive too.
So, why not adding some flag PF_RESTART_GHOST or TIF_RESTART_GHOST?
What do you think?
Thanks,
Louis
--
Dr Louis Rilling Kerlabs
Skype: louis.rilling Batiment Germanium
Phone: (+33|0) 6 80 89 08 23 80 avenue des Buttes de Coesmes
http://www.kerlabs.com/ 35700 Rennes
Attachment:
signature.asc
Description: Digital signature
_______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers
