The BUG_ON() should be protected by freezer->lock, otherwise it
can be triggered easily when a task has been unfreezed but the
corresponding cgroup hasn't been changed to FROZEN state.
Signed-off-by: Li Zefan <[email protected]>
---
kernel/cgroup_freezer.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index e950569..7f54d1c 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -190,8 +190,9 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
freezer = task_freezer(task);
task_unlock(task);
- BUG_ON(freezer->state == CGROUP_FROZEN);
spin_lock_irq(&freezer->lock);
+ BUG_ON(freezer->state == CGROUP_FROZEN);
+
/* Locking avoids race with FREEZING -> THAWED transitions. */
if (freezer->state == CGROUP_FREEZING)
freeze_task(task, true);
--
1.5.4.rc3
It is sufficient to check if @task is frozen, and no need to check if
the original freezer is frozen.
Signed-off-by: Li Zefan <[email protected]>
---
kernel/cgroup_freezer.c | 16 +++++++---------
1 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index 7f54d1c..6fadafe 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
struct task_struct *task)
{
struct freezer *freezer;
- int retval;
- /* Anything frozen can't move or be moved to/from */
+ /*
+ * Anything frozen can't move or be moved to/from.
+ *
+ * Since orig_freezer->state == FROZEN means that @task has been
+ * frozen, so it's sufficient to check the latter condition.
+ */
if (is_task_frozen_enough(task))
return -EBUSY;
@@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
if (freezer->state == CGROUP_FROZEN)
return -EBUSY;
- retval = 0;
- task_lock(task);
- freezer = task_freezer(task);
- if (freezer->state == CGROUP_FROZEN)
- retval = -EBUSY;
- task_unlock(task);
- return retval;
+ return 0;
}
static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
--
1.5.4.rc3
Don't duplicate the implementation of thaw_process().
Signed-off-by: Li Zefan <[email protected]>
---
kernel/cgroup_freezer.c | 15 ++++-----------
1 files changed, 4 insertions(+), 11 deletions(-)
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index 6fadafe..3ea57e4 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -275,25 +275,18 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
return num_cant_freeze_now ? -EBUSY : 0;
}
-static int unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
+static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
{
struct cgroup_iter it;
struct task_struct *task;
cgroup_iter_start(cgroup, &it);
while ((task = cgroup_iter_next(cgroup, &it))) {
- int do_wake;
-
- task_lock(task);
- do_wake = __thaw_process(task);
- task_unlock(task);
- if (do_wake)
- wake_up_process(task);
+ thaw_process(task);
}
cgroup_iter_end(cgroup, &it);
- freezer->state = CGROUP_THAWED;
- return 0;
+ freezer->state = CGROUP_THAWED;
}
static int freezer_change_state(struct cgroup *cgroup,
@@ -320,7 +313,7 @@ static int freezer_change_state(struct cgroup *cgroup,
}
/* state == FREEZING and goal_state == THAWED, so unfreeze */
case CGROUP_FROZEN:
- retval = unfreeze_cgroup(cgroup, freezer);
+ unfreeze_cgroup(cgroup, freezer);
break;
default:
break;
--
1.5.4.rc3
Just call unfreeze_cgroup() if goal_state == THAWED, and call
try_to_freeze_cgroup() if goal_state == FROZEN.
No behavior has been changed.
Signed-off-by: Li Zefan <[email protected]>
---
kernel/cgroup_freezer.c | 19 +++++++------------
1 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index 3ea57e4..cdef2d8 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -296,27 +296,22 @@ static int freezer_change_state(struct cgroup *cgroup,
int retval = 0;
freezer = cgroup_freezer(cgroup);
+
spin_lock_irq(&freezer->lock);
+
update_freezer_state(cgroup, freezer);
if (goal_state == freezer->state)
goto out;
- switch (freezer->state) {
+
+ switch (goal_state) {
case CGROUP_THAWED:
- retval = try_to_freeze_cgroup(cgroup, freezer);
+ unfreeze_cgroup(cgroup, freezer);
break;
- case CGROUP_FREEZING:
- if (goal_state == CGROUP_FROZEN) {
- /* Userspace is retrying after
- * "/bin/echo FROZEN > freezer.state" returned -EBUSY */
- retval = try_to_freeze_cgroup(cgroup, freezer);
- break;
- }
- /* state == FREEZING and goal_state == THAWED, so unfreeze */
case CGROUP_FROZEN:
- unfreeze_cgroup(cgroup, freezer);
+ retval = try_to_freeze_cgroup(cgroup, freezer);
break;
default:
- break;
+ BUG();
}
out:
spin_unlock_irq(&freezer->lock);
--
1.5.4.rc3
Li Zefan wrote:
> The BUG_ON() should be protected by freezer->lock, otherwise it
> can be triggered easily when a task has been unfreezed but the
> corresponding cgroup hasn't been changed to FROZEN state.
yes. thanks,
Acked-by: Cedric Le Goater <[email protected]>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/cgroup_freezer.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index e950569..7f54d1c 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -190,8 +190,9 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
> freezer = task_freezer(task);
> task_unlock(task);
>
> - BUG_ON(freezer->state == CGROUP_FROZEN);
> spin_lock_irq(&freezer->lock);
> + BUG_ON(freezer->state == CGROUP_FROZEN);
> +
> /* Locking avoids race with FREEZING -> THAWED transitions. */
> if (freezer->state == CGROUP_FREEZING)
> freeze_task(task, true);
Li Zefan wrote:
> Don't duplicate the implementation of thaw_process().
looks OK but you should remove __thaw_process() which is unused
now.
thanks,
C.
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/cgroup_freezer.c | 15 ++++-----------
> 1 files changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 6fadafe..3ea57e4 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -275,25 +275,18 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
> return num_cant_freeze_now ? -EBUSY : 0;
> }
>
> -static int unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
> +static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
> {
> struct cgroup_iter it;
> struct task_struct *task;
>
> cgroup_iter_start(cgroup, &it);
> while ((task = cgroup_iter_next(cgroup, &it))) {
> - int do_wake;
> -
> - task_lock(task);
> - do_wake = __thaw_process(task);
> - task_unlock(task);
> - if (do_wake)
> - wake_up_process(task);
> + thaw_process(task);
> }
> cgroup_iter_end(cgroup, &it);
> - freezer->state = CGROUP_THAWED;
>
> - return 0;
> + freezer->state = CGROUP_THAWED;
> }
>
> static int freezer_change_state(struct cgroup *cgroup,
> @@ -320,7 +313,7 @@ static int freezer_change_state(struct cgroup *cgroup,
> }
> /* state == FREEZING and goal_state == THAWED, so unfreeze */
> case CGROUP_FROZEN:
> - retval = unfreeze_cgroup(cgroup, freezer);
> + unfreeze_cgroup(cgroup, freezer);
> break;
> default:
> break;
Li Zefan wrote:
> It is sufficient to check if @task is frozen, and no need to check if
> the original freezer is frozen.
hmm, a task being frozen does not mean that its freezer cgroup is
frozen. So the extra check seems valid but looking at the comment :
/*
* The call to cgroup_lock() in the freezer.state write method prevents
* a write to that file racing against an attach, and hence the
* can_attach() result will remain valid until the attach completes.
*/
static int freezer_can_attach(struct cgroup_subsys *ss,
how do we know that the task_freezer(task), which is not protected by
the cgroup_lock(), is not going to change its state to CGROUP_FROZEN
it looks racy.
C.
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/cgroup_freezer.c | 16 +++++++---------
> 1 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 7f54d1c..6fadafe 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
> struct task_struct *task)
> {
> struct freezer *freezer;
> - int retval;
>
> - /* Anything frozen can't move or be moved to/from */
> + /*
> + * Anything frozen can't move or be moved to/from.
> + *
> + * Since orig_freezer->state == FROZEN means that @task has been
> + * frozen, so it's sufficient to check the latter condition.
> + */
>
> if (is_task_frozen_enough(task))
> return -EBUSY;
> @@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
> if (freezer->state == CGROUP_FROZEN)
> return -EBUSY;
>
> - retval = 0;
> - task_lock(task);
> - freezer = task_freezer(task);
> - if (freezer->state == CGROUP_FROZEN)
> - retval = -EBUSY;
> - task_unlock(task);
> - return retval;
> + return 0;
> }
>
> static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
Cedric Le Goater wrote:
> Li Zefan wrote:
>> Don't duplicate the implementation of thaw_process().
>
> looks OK but you should remove __thaw_process() which is unused
> now.
>
Then I'll make it a static function and remove the declaration from .h file.
Cedric Le Goater wrote:
> Li Zefan wrote:
>> It is sufficient to check if @task is frozen, and no need to check if
>> the original freezer is frozen.
>
> hmm, a task being frozen does not mean that its freezer cgroup is
> frozen.
If a task has being frozen, then can_attach() returns -EBUSY at once.
If a task isn't frozen, then we have orig_freezer->state == THAWED.
So we need to check the state of the task only.
> So the extra check seems valid but looking at the comment :
>
> /*
> * The call to cgroup_lock() in the freezer.state write method prevents
> * a write to that file racing against an attach, and hence the
> * can_attach() result will remain valid until the attach completes.
> */
> static int freezer_can_attach(struct cgroup_subsys *ss,
>
> how do we know that the task_freezer(task), which is not protected by
> the cgroup_lock(), is not going to change its state to CGROUP_FROZEN
> it looks racy.
>
Since freezer_can_attach() is called by cgroup core with cgroup_lock held, there is
no race with task attaching and state changing.
> C.
>
>> Signed-off-by: Li Zefan <[email protected]>
>> ---
>> kernel/cgroup_freezer.c | 16 +++++++---------
>> 1 files changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
>> index 7f54d1c..6fadafe 100644
>> --- a/kernel/cgroup_freezer.c
>> +++ b/kernel/cgroup_freezer.c
>> @@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
>> struct task_struct *task)
>> {
>> struct freezer *freezer;
>> - int retval;
>>
>> - /* Anything frozen can't move or be moved to/from */
>> + /*
>> + * Anything frozen can't move or be moved to/from.
>> + *
>> + * Since orig_freezer->state == FROZEN means that @task has been
>> + * frozen, so it's sufficient to check the latter condition.
>> + */
>>
>> if (is_task_frozen_enough(task))
>> return -EBUSY;
>> @@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
>> if (freezer->state == CGROUP_FROZEN)
>> return -EBUSY;
>>
>> - retval = 0;
>> - task_lock(task);
>> - freezer = task_freezer(task);
>> - if (freezer->state == CGROUP_FROZEN)
>> - retval = -EBUSY;
>> - task_unlock(task);
>> - return retval;
>> + return 0;
>> }
>>
>> static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
>
>
Li Zefan wrote:
> Cedric Le Goater wrote:
>> Li Zefan wrote:
>>> It is sufficient to check if @task is frozen, and no need to check if
>>> the original freezer is frozen.
>> hmm, a task being frozen does not mean that its freezer cgroup is
>> frozen.
>
> If a task has being frozen, then can_attach() returns -EBUSY at once.
> If a task isn't frozen, then we have orig_freezer->state == THAWED.
>
> So we need to check the state of the task only.
>
>> So the extra check seems valid but looking at the comment :
>>
>> /*
>> * The call to cgroup_lock() in the freezer.state write method prevents
>> * a write to that file racing against an attach, and hence the
>> * can_attach() result will remain valid until the attach completes.
>> */
>> static int freezer_can_attach(struct cgroup_subsys *ss,
>>
>> how do we know that the task_freezer(task), which is not protected by
>> the cgroup_lock(), is not going to change its state to CGROUP_FROZEN
>> it looks racy.
>>
>
> Since freezer_can_attach() is called by cgroup core with cgroup_lock held, there is
> no race with task attaching and state changing.
ok I see. cgroup_mutex is global, I thought it had changed and that we
were only locking the cgroup the task was being attached to.
Acked-by: Cedric Le Goater <[email protected]>
thanks,
C.
Li Zefan wrote:
> Just call unfreeze_cgroup() if goal_state == THAWED, and call
> try_to_freeze_cgroup() if goal_state == FROZEN.
>
> No behavior has been changed.
Looks good.
Acked-by: Cedric Le Goater <[email protected]>
thanks,
C.
>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/cgroup_freezer.c | 19 +++++++------------
> 1 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 3ea57e4..cdef2d8 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -296,27 +296,22 @@ static int freezer_change_state(struct cgroup *cgroup,
> int retval = 0;
>
> freezer = cgroup_freezer(cgroup);
> +
> spin_lock_irq(&freezer->lock);
> +
> update_freezer_state(cgroup, freezer);
> if (goal_state == freezer->state)
> goto out;
> - switch (freezer->state) {
> +
> + switch (goal_state) {
> case CGROUP_THAWED:
> - retval = try_to_freeze_cgroup(cgroup, freezer);
> + unfreeze_cgroup(cgroup, freezer);
> break;
> - case CGROUP_FREEZING:
> - if (goal_state == CGROUP_FROZEN) {
> - /* Userspace is retrying after
> - * "/bin/echo FROZEN > freezer.state" returned -EBUSY */
> - retval = try_to_freeze_cgroup(cgroup, freezer);
> - break;
> - }
> - /* state == FREEZING and goal_state == THAWED, so unfreeze */
> case CGROUP_FROZEN:
> - unfreeze_cgroup(cgroup, freezer);
> + retval = try_to_freeze_cgroup(cgroup, freezer);
> break;
> default:
> - break;
> + BUG();
> }
> out:
> spin_unlock_irq(&freezer->lock);
On Tue, 21 Oct 2008 09:16:08 +0200
Cedric Le Goater <[email protected]> wrote:
> Li Zefan wrote:
> > Don't duplicate the implementation of thaw_process().
>
> looks OK but you should remove __thaw_process() which is unused
> now.
It's called by thaw_process().
But that's the only callsite, I believe, so...
--- a/include/linux/freezer.h~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix
+++ a/include/linux/freezer.h
@@ -44,11 +44,6 @@ static inline bool should_send_signal(st
return !(p->flags & PF_FREEZER_NOSIG);
}
-/*
- * Wake up a frozen process
- */
-extern int __thaw_process(struct task_struct *p);
-
/* Takes and releases task alloc lock using task_lock() */
extern int thaw_process(struct task_struct *p);
diff -puN kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix kernel/freezer.c
--- a/kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix
+++ a/kernel/freezer.c
@@ -121,16 +121,7 @@ void cancel_freezing(struct task_struct
}
}
-/*
- * Wake up a frozen process
- *
- * task_lock() is needed to prevent the race with refrigerator() which may
- * occur if the freezing of tasks fails. Namely, without the lock, if the
- * freezing of tasks failed, thaw_tasks() might have run before a task in
- * refrigerator() could call frozen_process(), in which case the task would be
- * frozen and no one would thaw it.
- */
-int __thaw_process(struct task_struct *p)
+static int __thaw_process(struct task_struct *p)
{
if (frozen(p)) {
p->flags &= ~PF_FROZEN;
@@ -140,6 +131,15 @@ int __thaw_process(struct task_struct *p
return 0;
}
+/*
+ * Wake up a frozen process
+ *
+ * task_lock() is needed to prevent the race with refrigerator() which may
+ * occur if the freezing of tasks fails. Namely, without the lock, if the
+ * freezing of tasks failed, thaw_tasks() might have run before a task in
+ * refrigerator() could call frozen_process(), in which case the task would be
+ * frozen and no one would thaw it.
+ */
int thaw_process(struct task_struct *p)
{
task_lock(p);
_
On Tue, 2008-10-21 at 08:48 +0800, Li Zefan wrote:
> The BUG_ON() should be protected by freezer->lock, otherwise it
> can be triggered easily when a task has been unfreezed but the
> corresponding cgroup hasn't been changed to FROZEN state.
>
> Signed-off-by: Li Zefan <[email protected]>
Acked-by: Matt Helsley <[email protected]>
> ---
> kernel/cgroup_freezer.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index e950569..7f54d1c 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -190,8 +190,9 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
> freezer = task_freezer(task);
> task_unlock(task);
>
> - BUG_ON(freezer->state == CGROUP_FROZEN);
> spin_lock_irq(&freezer->lock);
> + BUG_ON(freezer->state == CGROUP_FROZEN);
> +
> /* Locking avoids race with FREEZING -> THAWED transitions. */
> if (freezer->state == CGROUP_FREEZING)
> freeze_task(task, true);
On Tue, 2008-10-21 at 08:50 +0800, Li Zefan wrote:
> It is sufficient to check if @task is frozen, and no need to check if
> the original freezer is frozen.
Looks great! Thanks!
Acked-by: Matt Helsley <[email protected]>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/cgroup_freezer.c | 16 +++++++---------
> 1 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 7f54d1c..6fadafe 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -162,9 +162,13 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
> struct task_struct *task)
> {
> struct freezer *freezer;
> - int retval;
>
> - /* Anything frozen can't move or be moved to/from */
> + /*
> + * Anything frozen can't move or be moved to/from.
> + *
> + * Since orig_freezer->state == FROZEN means that @task has been
> + * frozen, so it's sufficient to check the latter condition.
> + */
>
> if (is_task_frozen_enough(task))
> return -EBUSY;
> @@ -173,13 +177,7 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
> if (freezer->state == CGROUP_FROZEN)
> return -EBUSY;
>
> - retval = 0;
> - task_lock(task);
> - freezer = task_freezer(task);
> - if (freezer->state == CGROUP_FROZEN)
> - retval = -EBUSY;
> - task_unlock(task);
> - return retval;
> + return 0;
> }
>
> static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
On Tue, 2008-10-21 at 08:51 +0800, Li Zefan wrote:
> Don't duplicate the implementation of thaw_process().
>
> Signed-off-by: Li Zefan <[email protected]>
Acked-by: Matt Helsley <[email protected]>
> ---
> kernel/cgroup_freezer.c | 15 ++++-----------
> 1 files changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 6fadafe..3ea57e4 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -275,25 +275,18 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
> return num_cant_freeze_now ? -EBUSY : 0;
> }
>
> -static int unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
> +static void unfreeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
> {
> struct cgroup_iter it;
> struct task_struct *task;
>
> cgroup_iter_start(cgroup, &it);
> while ((task = cgroup_iter_next(cgroup, &it))) {
> - int do_wake;
> -
> - task_lock(task);
> - do_wake = __thaw_process(task);
> - task_unlock(task);
> - if (do_wake)
> - wake_up_process(task);
> + thaw_process(task);
> }
> cgroup_iter_end(cgroup, &it);
> - freezer->state = CGROUP_THAWED;
>
> - return 0;
> + freezer->state = CGROUP_THAWED;
> }
>
> static int freezer_change_state(struct cgroup *cgroup,
> @@ -320,7 +313,7 @@ static int freezer_change_state(struct cgroup *cgroup,
> }
> /* state == FREEZING and goal_state == THAWED, so unfreeze */
> case CGROUP_FROZEN:
> - retval = unfreeze_cgroup(cgroup, freezer);
> + unfreeze_cgroup(cgroup, freezer);
> break;
> default:
> break;
On Tue, 2008-10-21 at 08:52 +0800, Li Zefan wrote:
> Just call unfreeze_cgroup() if goal_state == THAWED, and call
> try_to_freeze_cgroup() if goal_state == FROZEN.
>
> No behavior has been changed.
Another good one. Thanks!
Acked-by: Matt Helsley <[email protected]>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> kernel/cgroup_freezer.c | 19 +++++++------------
> 1 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 3ea57e4..cdef2d8 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -296,27 +296,22 @@ static int freezer_change_state(struct cgroup *cgroup,
> int retval = 0;
>
> freezer = cgroup_freezer(cgroup);
> +
> spin_lock_irq(&freezer->lock);
> +
> update_freezer_state(cgroup, freezer);
> if (goal_state == freezer->state)
> goto out;
> - switch (freezer->state) {
> +
> + switch (goal_state) {
> case CGROUP_THAWED:
> - retval = try_to_freeze_cgroup(cgroup, freezer);
> + unfreeze_cgroup(cgroup, freezer);
> break;
> - case CGROUP_FREEZING:
> - if (goal_state == CGROUP_FROZEN) {
> - /* Userspace is retrying after
> - * "/bin/echo FROZEN > freezer.state" returned -EBUSY */
> - retval = try_to_freeze_cgroup(cgroup, freezer);
> - break;
> - }
> - /* state == FREEZING and goal_state == THAWED, so unfreeze */
> case CGROUP_FROZEN:
> - unfreeze_cgroup(cgroup, freezer);
> + retval = try_to_freeze_cgroup(cgroup, freezer);
> break;
> default:
> - break;
> + BUG();
> }
> out:
> spin_unlock_irq(&freezer->lock);
Andrew Morton wrote:
> On Tue, 21 Oct 2008 09:16:08 +0200
> Cedric Le Goater <[email protected]> wrote:
>
>> Li Zefan wrote:
>>> Don't duplicate the implementation of thaw_process().
>> looks OK but you should remove __thaw_process() which is unused
>> now.
>
> It's called by thaw_process().
>
> But that's the only callsite, I believe, so...
yes it is. it was added by dc52ddc0e6f45b04780b26fc0813509f8e798c42
and was inline before.
thanks,
C.
>
> --- a/include/linux/freezer.h~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix
> +++ a/include/linux/freezer.h
> @@ -44,11 +44,6 @@ static inline bool should_send_signal(st
> return !(p->flags & PF_FREEZER_NOSIG);
> }
>
> -/*
> - * Wake up a frozen process
> - */
> -extern int __thaw_process(struct task_struct *p);
> -
> /* Takes and releases task alloc lock using task_lock() */
> extern int thaw_process(struct task_struct *p);
>
> diff -puN kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix kernel/freezer.c
> --- a/kernel/freezer.c~freezer_cg-use-thaw_process-in-unfreeze_cgroup-fix
> +++ a/kernel/freezer.c
> @@ -121,16 +121,7 @@ void cancel_freezing(struct task_struct
> }
> }
>
> -/*
> - * Wake up a frozen process
> - *
> - * task_lock() is needed to prevent the race with refrigerator() which may
> - * occur if the freezing of tasks fails. Namely, without the lock, if the
> - * freezing of tasks failed, thaw_tasks() might have run before a task in
> - * refrigerator() could call frozen_process(), in which case the task would be
> - * frozen and no one would thaw it.
> - */
> -int __thaw_process(struct task_struct *p)
> +static int __thaw_process(struct task_struct *p)
> {
> if (frozen(p)) {
> p->flags &= ~PF_FROZEN;
> @@ -140,6 +131,15 @@ int __thaw_process(struct task_struct *p
> return 0;
> }
>
> +/*
> + * Wake up a frozen process
> + *
> + * task_lock() is needed to prevent the race with refrigerator() which may
> + * occur if the freezing of tasks fails. Namely, without the lock, if the
> + * freezing of tasks failed, thaw_tasks() might have run before a task in
> + * refrigerator() could call frozen_process(), in which case the task would be
> + * frozen and no one would thaw it.
> + */
> int thaw_process(struct task_struct *p)
> {
> task_lock(p);
> _
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>