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
>
> Sukadev
> _______________________________________________
> Containers mailing list
> Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
> https://lists.linux-foundation.org/mailman/listinfo/containers
--
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
