Re: [PATCH 1/1] freezer: change ptrace_stop/do_signal_stop to use freezable_schedule()
From: Tejun Heo <hidden>
Date: 2012-10-25 17:18:19
Also in:
lkml, stable
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. :)
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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