On 03/26/2011 12:06 PM, Louis Rilling wrote: > On 25/03/11 22:14 -0400, Oren Laadan wrote: >> >> >> On 03/04/2011 12:29 PM, Louis Rilling wrote: >>> On 04/03/11 11:07 -0500, Oren Laadan wrote: >>>> On 03/03/2011 11:35 AM, Louis Rilling wrote: >>>>> On 03/03/11 10:38 -0500, Oren Laadan wrote: >>>>>> On 03/01/2011 10:31 AM, Louis Rilling wrote: >>>>>>> On 28/02/11 17:10 -0500, Oren Laadan wrote: >>>>>>>> So looking at the code again, we could add one condition in exit.c >>>>>>>> at wait_consider_task(), after the test of p->exit_state == EXIT_DEAD, >>>>>>>> to also test: >>>>>>>> >>>>>>>> inline static bool is_ghost_task(p) >>>>>>>> { >>>>>>>> return (p->flags & (PF_EXITING|PF_RESTARTING) == >>>>>>>> PF_EXITING|PF_RESTARTING) && task_detached(p) >>>>>>>> } >>>>>>>> >>>>>>>> if (p->flags & is_ghost_task(p)) >>>>>>>> return 0; >>>>>>>> >>>>>>>> Or something along the lines (e.g. used EXIT_ZOMBIE comparison instead >>>>>>>> of PF_EXITING). While requiring a kernel patch, it is relatively short, >>>>>>>> clean and easy to review. >>>>> >>>>> EXIT_ZOMBIE comparison would not optimize much imho, since p->flags must be >>>>> checked anyway. >>>>> >>>>> Nit1: I don't think that checking p->flags saves anything before calling >>>>> is_ghost_task(). >>>> >>>> Hmm.. right - >>>> That's a leftover from before I decided to introduce is_ghost_task() >>>> >>>>> >>>>> Nit2: why would you like to check that PF_EXITING and PF_RESTARTING come >>>>> together? Is it to make sure that no "real" restarted thread will be skipped >>>>> this way? >>>> >>>> If wait() is called to get the state of stopped children, and for >>>> whatever reason the ghost is stopped or being ptraced (we should >>>> probably prevent that... but ok) - testing for the exiting/zombie >>>> condition is an extra safety measure: only skip this task when it >>>> is actually exiting. >>> >>> I don't see how a ghost task could be stopped or ptraced, since it calls >>> do_exit() right after becoming detached, and thus identifiable as a ghost. >>> Unless it gets ptraced right before calling sys_restart()? Even in that case, >>> it's not reapable by ptrace since it's not in stopped state. OTOH, it may still >>> be reaped in wait_task_continued() (see below). >>> >>>> >>>> Do you not think it's needed ? >>> >>> Not sure. As far as I can see, other restarting (with PF_RESTARTING) and >>> detached tasks can only be sub-threads, and are mostly not reapable in any way >>> as long as PF_RESTARTING is set. They can surely be reaped neither by >>> wait_task_zombie(), nor by wait_task_stopped(). The only possibility I see is by >>> wait_task_continued(), because a previous "wakeup from stopped" has not been >>> consumed before the checkpoint. >>> >>> But, and I think that this is a good reason to check PF_EXITING (or >>> ->exit_state), if threads are skipped this way, then wait() might incorrectly >>> return -ECHILD instead of sleeping. >>> >>> Wait. Even with this, after ->exit_signal is set to -1, and before PF_EXITING is >>> set, wait_consider_task() can still consider the ghost as potentially reapable >>> in the future. Deadlock again. >>> >>> In fact, it's probably much saner to have something atomic, like: >>> >>> write_lock_irq(&tasklist_lock); >>> p->flags |= PF_EXITING; >>> p->exit_signal = -1; >>> __wake_up_parent(p, p->parent); >>> write_unlock_irq(&tasklist_lock); >>> >>> Unfortunately this is not accepted by do_exit(). So two kinds of solutions: >>> either set a new flag à la PF_RESTART_GHOST, and only check for this flag in >>> wait_consider_task(), >>> or somewhere in do_exit() (latest in exit_notify()), have >>> another mean to recognize ghost tasks, and do the ->exit_signal = -1 + >>> __wake_up_parent() there. >>> >>> What's your opinion? >>> >> >> Doing it in wait_consider_task() may be a problem since we only mark >> a task as ghost after it has lived for a while, so wait() would have >> already considered it a valid child to wait for. >> >> If I had to choose, then I'd do the snippet you suggest above - and >> in particular where PF_EXITING is already set, which is exit_signals(). >> >> Adding a means to recognize ghost tasks is simple: we ran out of >> task->flags, but we can add a c/r related field to hold such a flag >> (we already add one field to the task_struct). >> >> Do you think that will do it ? > > Yup, any way to have a flag protected by tasklist_lock would be ok. For > instance, use some bit near ->did_exec. IMHO of course :) Good idea. How about this patch: diff --git a/include/linux/sched.h b/include/linux/sched.h index e42bf29..5a08d49 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1267,6 +1267,9 @@ struct task_struct { unsigned in_execve:1; /* Tell the LSMs that the process is doing an * execve */ unsigned in_iowait:1; +#ifdef CONFIG_CHECKPOINT + unsigned ckpt_ghost:1; /* ghost task in c/r - auto-reap */ +#endif /* Revert to default priority/policy when forking */ diff --git a/kernel/checkpoint/restart.c b/kernel/checkpoint/restart.c index 01da67f..880d456 100644 --- a/kernel/checkpoint/restart.c +++ b/kernel/checkpoint/restart.c @@ -971,6 +971,8 @@ static int do_ghost_task(void) restore_debug_error(ctx, ret); if (ret < 0) ckpt_err(ctx, ret, "ghost restart failed\n"); + else + current->ckpt_ghost = 1; restore_debug_exit(ctx); ckpt_ctx_put(ctx); diff --git a/kernel/signal.c b/kernel/signal.c index b1e6a31..8bc7c9e 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2017,6 +2017,13 @@ void exit_signals(struct task_struct *tsk) struct task_struct *t; if (thread_group_empty(tsk) || signal_group_exit(tsk->signal)) { + if (tsk->ckpt_ghost) { + write_lock_irq(&tasklist_lock); + p->flags |= PF_EXITING; + p->exit_signal = -1; + __wake_up_parent(p, p->parent); + write_unlock_irq(&tasklist_lock); + } tsk->flags |= PF_EXITING; return; } _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers