Re: [PATCH v2 1/1] freezer: change ptrace_stop/do_signal_stop to use freezable_schedule()
On 10/27, Ben Hutchings wrote: > > On Fri, 2012-10-26 at 19:46 +0200, Oleg Nesterov wrote: > > try_to_freeze_tasks() and cgroup_freezer rely on scheduler locks > > to ensure that a task doing STOPPED/TRACED -> RUNNING transition > > can't escape freezing. This mostly works, but ptrace_stop() does > > not necessarily call schedule(), it can change task->state back to > > RUNNING and check freezing() without any lock/barrier in between. > > > > We could add the necessary barrier, but this patch changes > > ptrace_stop() and do_signal_stop() to use freezable_schedule(). > > This fixes the race, freezer_count() and freezer_should_skip() > > carefully avoid the race. > > > > And this simplifies the code, try_to_freeze_tasks/update_if_frozen > > no longer need to use task_is_stopped_or_traced() checks with the > > non trivial assumptions. We can rely on the mechanism which was > > specially designed to mark the sleeping task as "frozen enough". > > > > v2: As Tejun pointed out, we can also change get_signal_to_deliver() > > and move try_to_freeze() up before 'relock' label. > > > > Signed-off-by: Oleg Nesterov > [...] > > This is not the correct way to submit a change to stable. Please see > Documentation/stable_kernel_rules.txt Sorry for confusion, it is not for stable@, it was cc'ed by mistake. Oleg. -- 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/
Re: [PATCH v2 1/1] freezer: change ptrace_stop/do_signal_stop to use freezable_schedule()
On Friday, October 26, 2012 02:29:09 PM Tejun Heo wrote: > Hello, > > On Fri, Oct 26, 2012 at 11:29:56PM +0200, Rafael J. Wysocki wrote: > > Actually, what tree is it supposed to apply to? > > > > The change in kernel/cgroup_freezer.c doesn't look like anything in > > the current Linus' tree to me. > > Ooh, right. This depends on the earlier cgroup_freezer changes. > Sorry about the confusion. I'll apply it to the following branch (the > same one used for the previous cgroup_freezer updates). > > git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git cgroup-freezer OK I haven't merged it yet, so I'll get this fix along with the rest. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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/
Re: [PATCH v2 1/1] freezer: change ptrace_stop/do_signal_stop to use freezable_schedule()
On Fri, 2012-10-26 at 19:46 +0200, Oleg Nesterov wrote: > try_to_freeze_tasks() and cgroup_freezer rely on scheduler locks > to ensure that a task doing STOPPED/TRACED -> RUNNING transition > can't escape freezing. This mostly works, but ptrace_stop() does > not necessarily call schedule(), it can change task->state back to > RUNNING and check freezing() without any lock/barrier in between. > > We could add the necessary barrier, but this patch changes > ptrace_stop() and do_signal_stop() to use freezable_schedule(). > This fixes the race, freezer_count() and freezer_should_skip() > carefully avoid the race. > > And this simplifies the code, try_to_freeze_tasks/update_if_frozen > no longer need to use task_is_stopped_or_traced() checks with the > non trivial assumptions. We can rely on the mechanism which was > specially designed to mark the sleeping task as "frozen enough". > > v2: As Tejun pointed out, we can also change get_signal_to_deliver() > and move try_to_freeze() up before 'relock' label. > > Signed-off-by: Oleg Nesterov [...] This is not the correct way to submit a change to stable. Please see Documentation/stable_kernel_rules.txt Ben. -- Ben Hutchings Never attribute to conspiracy what can adequately be explained by stupidity. signature.asc Description: This is a digitally signed message part
Re: [PATCH v2 1/1] freezer: change ptrace_stop/do_signal_stop to use freezable_schedule()
Hello, On Fri, Oct 26, 2012 at 11:29:56PM +0200, Rafael J. Wysocki wrote: > Actually, what tree is it supposed to apply to? > > The change in kernel/cgroup_freezer.c doesn't look like anything in > the current Linus' tree to me. Ooh, right. This depends on the earlier cgroup_freezer changes. Sorry about the confusion. I'll apply it to the following branch (the same one used for the previous cgroup_freezer updates). git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git cgroup-freezer 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/
Re: [PATCH v2 1/1] freezer: change ptrace_stop/do_signal_stop to use freezable_schedule()
On Friday, October 26, 2012 11:14:17 PM Rafael J. Wysocki wrote: > On Friday, October 26, 2012 08:01:49 PM Oleg Nesterov wrote: > > On 10/26, Tejun Heo wrote: > > > > > > Acked-by: Tejun Heo > > > > Thanks! > > > > > Rafael, sorry that this one doesn't have pm cc'd > > > > Ah, sorry Rafael. Yes, I have read you email, and I was going to > > add linux-pm but forgot. > > > > > but can you please > > > pick up this one too? > > > > Please, and thanks. > > OK, but that will go to Linus in the next batch. Actually, what tree is it supposed to apply to? The change in kernel/cgroup_freezer.c doesn't look like anything in the current Linus' tree to me. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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/
Re: [PATCH v2 1/1] freezer: change ptrace_stop/do_signal_stop to use freezable_schedule()
On Friday, October 26, 2012 08:01:49 PM Oleg Nesterov wrote: > On 10/26, Tejun Heo wrote: > > > > Acked-by: Tejun Heo > > Thanks! > > > Rafael, sorry that this one doesn't have pm cc'd > > Ah, sorry Rafael. Yes, I have read you email, and I was going to > add linux-pm but forgot. > > > but can you please > > pick up this one too? > > Please, and thanks. OK, but that will go to Linus in the next batch. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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/
Re: [PATCH v2 1/1] freezer: change ptrace_stop/do_signal_stop to use freezable_schedule()
On 10/26, Tejun Heo wrote: > > Acked-by: Tejun Heo Thanks! > Rafael, sorry that this one doesn't have pm cc'd Ah, sorry Rafael. Yes, I have read you email, and I was going to add linux-pm but forgot. > but can you please > pick up this one too? Please, and thanks. Oleg. -- 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/
Re: [PATCH v2 1/1] freezer: change ptrace_stop/do_signal_stop to use freezable_schedule()
On Fri, Oct 26, 2012 at 07:46:06PM +0200, Oleg Nesterov wrote: > try_to_freeze_tasks() and cgroup_freezer rely on scheduler locks > to ensure that a task doing STOPPED/TRACED -> RUNNING transition > can't escape freezing. This mostly works, but ptrace_stop() does > not necessarily call schedule(), it can change task->state back to > RUNNING and check freezing() without any lock/barrier in between. > > We could add the necessary barrier, but this patch changes > ptrace_stop() and do_signal_stop() to use freezable_schedule(). > This fixes the race, freezer_count() and freezer_should_skip() > carefully avoid the race. > > And this simplifies the code, try_to_freeze_tasks/update_if_frozen > no longer need to use task_is_stopped_or_traced() checks with the > non trivial assumptions. We can rely on the mechanism which was > specially designed to mark the sleeping task as "frozen enough". > > v2: As Tejun pointed out, we can also change get_signal_to_deliver() > and move try_to_freeze() up before 'relock' label. > > Signed-off-by: Oleg Nesterov Looks good to me. :) Acked-by: Tejun Heo Rafael, sorry that this one doesn't have pm cc'd but can you please pick up this one too? Thanks a lot. -- 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/
[PATCH v2 1/1] freezer: change ptrace_stop/do_signal_stop to use freezable_schedule()
try_to_freeze_tasks() and cgroup_freezer rely on scheduler locks
to ensure that a task doing STOPPED/TRACED -> RUNNING transition
can't escape freezing. This mostly works, but ptrace_stop() does
not necessarily call schedule(), it can change task->state back to
RUNNING and check freezing() without any lock/barrier in between.
We could add the necessary barrier, but this patch changes
ptrace_stop() and do_signal_stop() to use freezable_schedule().
This fixes the race, freezer_count() and freezer_should_skip()
carefully avoid the race.
And this simplifies the code, try_to_freeze_tasks/update_if_frozen
no longer need to use task_is_stopped_or_traced() checks with the
non trivial assumptions. We can rely on the mechanism which was
specially designed to mark the sleeping task as "frozen enough".
v2: As Tejun pointed out, we can also change get_signal_to_deliver()
and move try_to_freeze() up before 'relock' label.
Signed-off-by: Oleg Nesterov
---
include/linux/freezer.h |7 +++
kernel/cgroup_freezer.c |3 +--
kernel/freezer.c| 11 ++-
kernel/power/process.c | 13 +
kernel/signal.c | 20 ++--
5 files changed, 13 insertions(+), 41 deletions(-)
diff --git a/include/linux/freezer.h b/include/linux/freezer.h
index ee89932..8039893 100644
--- a/include/linux/freezer.h
+++ b/include/linux/freezer.h
@@ -134,10 +134,9 @@ static inline bool freezer_should_skip(struct task_struct
*p)
}
/*
- * These macros are intended to be used whenever you want allow a task that's
- * sleeping in TASK_UNINTERRUPTIBLE or TASK_KILLABLE state to be frozen. Note
- * that neither return any clear indication of whether a freeze event happened
- * while in this function.
+ * These macros are intended to be used whenever you want allow a sleeping
+ * task to be frozen. Note that neither return any clear indication of
+ * whether a freeze event happened while in this function.
*/
/* Like schedule(), but should not block the freezer. */
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index 8a92b0e..bedefd9 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -198,8 +198,7 @@ static void update_if_frozen(struct cgroup *cgroup, struct
freezer *freezer)
* completion. Consider it frozen in addition to
* the usual frozen condition.
*/
- if (!frozen(task) && !task_is_stopped_or_traced(task) &&
- !freezer_should_skip(task))
+ if (!frozen(task) && !freezer_should_skip(task))
goto notyet;
}
}
diff --git a/kernel/freezer.c b/kernel/freezer.c
index 11f82a4..c38893b 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -116,17 +116,10 @@ bool freeze_task(struct task_struct *p)
return false;
}
- if (!(p->flags & PF_KTHREAD)) {
+ if (!(p->flags & PF_KTHREAD))
fake_signal_wake_up(p);
- /*
-* fake_signal_wake_up() goes through p's scheduler
-* lock and guarantees that TASK_STOPPED/TRACED ->
-* TASK_RUNNING transition can't race with task state
-* testing in try_to_freeze_tasks().
-*/
- } else {
+ else
wake_up_state(p, TASK_INTERRUPTIBLE);
- }
spin_unlock_irqrestore(&freezer_lock, flags);
return true;
diff --git a/kernel/power/process.c b/kernel/power/process.c
index 87da817..d5a258b 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -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);
diff --git a/kernel/signal.c b/kernel/signal.c
index 0af8868..5ffb562 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)
pree
