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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/