Re: [PATCH][cr]: Fix ghost task bug

[Louis Rilling <Louis.Rilling-aw0BnHfMbSpBDgjK7y7TUQ@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4182 bytes --]

On Fri, Feb 25, 2011 at 11:01:32AM -0800, Sukadev Bhattiprolu wrote:
> Louis Rilling [Louis.Rilling-aw0BnHfMbSpBDgjK7y7TUQ@public.gmane.org] 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-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> 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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 206 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers