Hello, Oleg.
On Thu, Oct 25, 2012 at 06:39:59PM +0200, Oleg Nesterov wrote:
> Change ptrace_stop() and do_signal_stop() to use freezable_schedule()
> rather than rely on subsequent try_to_freeze().
>
> This allows to remove the task_is_stopped_or_traced() checks from
> try_to_freeze_tasks() and update_if_frozen(), and this fixes the
> unlikely race with ptrace_stop(). If the tracee does not schedule()
> it can miss a freezing condition.
I think it would be great if the description is more detailed. This
code path always makes my head spin and I think we can definitely use
some more guiding in understanding this dang thing. :)
> @@ -48,18 +48,7 @@ static int try_to_freeze_tasks(bool user_only)
> if (p == current || !freeze_task(p))
> continue;
>
> - /*
> - * Now that we've done set_freeze_flag, don't
> - * perturb a task in TASK_STOPPED or TASK_TRACED.
> - * It is "frozen enough". If the task does wake
> - * up, it will immediately call try_to_freeze.
> - *
> - * Because freeze_task() goes through p's scheduler lock, it's
> - * guaranteed that TASK_STOPPED/TRACED -> TASK_RUNNING
> - * transition can't race with task state testing here.
> - */
> - if (!task_is_stopped_or_traced(p) &&
> - !freezer_should_skip(p))
> + if (!freezer_should_skip(p))
> todo++;
> } while_each_thread(g, p);
> read_unlock(&tasklist_lock);
This looks really good.
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 0af8868..1660d7d 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1908,7 +1908,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
> preempt_disable();
> read_unlock(&tasklist_lock);
> preempt_enable_no_resched();
> - schedule();
> + freezable_schedule();
> } else {
> /*
> * By the time we got the lock, our tracer went away.
> @@ -1930,13 +1930,6 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
> }
>
> /*
> - * While in TASK_TRACED, we were considered "frozen enough".
> - * Now that we woke up, it's crucial if we're supposed to be
> - * frozen that we freeze now before running anything substantial.
> - */
> - try_to_freeze();
> -
> - /*
> * We are back. Now reacquire the siglock before touching
> * last_siginfo, so that we are sure to have synchronized with
> * any signal-sending on another CPU that wants to examine it.
> @@ -2092,7 +2085,7 @@ static bool do_signal_stop(int signr)
> }
>
> /* Now we don't run again until woken by SIGCONT or SIGKILL */
> - schedule();
> + freezable_schedule();
This makes me wonder whether we still need try_to_freeze() in
get_signal_to_deliver() right after the relock: label. Freezer no
longer treats STOPPED/TRACED special and both sleeping sites in signal
deliver path are marked freezable_schedule(). We shouldn't need the
explicit try_to_freeze(), right?
Thanks.
--
tejun
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/containers
