Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Tomasz Buchert]

Hi,
I'm trying to understand now why I did that change in

2d3cbf8bc (the bug itself was in the if-the-else clause in 
update_if_frozen , anyway).
Well, when I look at this now I think that there is nothing wrong with 
your patch.
You can try my testcases from 2d3cbf8bc and 0bdba580, but it should be ok.


One thing I am not sure completely of is the following situation. So the 
group is frozen
with the STOPPED task inside. There are few questions:
* if you send SIGCONT to the task now, will it wake up?
* if yes - will it get *immediately* frozen?
I im going to check it myself, but maybe you can answer faster :).


Tomek

Le 16/11/2011 22:50, Michal Hocko a écrit :
> 2d3cbf8b (cgroup_freezer: update_freezer_state() does incorrect state
> transitions) removed is_task_frozen_enough and replaced it with a simple
> frozen call. This, however, breaks freezing for a group with stopped tasks
> because those cannot be frozen and so the group remains in CGROUP_FREEZING
> state (update_if_frozen doesn't count stopped tasks) and never reaches
> CGROUP_FROZEN.
>
> Let's add is_task_frozen_enough back and use it at the original locations
> (update_if_frozen and try_to_freeze_cgroup). Semantically we consider
> stopped tasks as frozen enough so we should consider both cases when
> testing frozen tasks.
>
> Testcase:
> mkdir /dev/freezer
> mount -t cgroup -o freezer none /dev/freezer
> mkdir /dev/freezer/foo
> sleep 1h&
> pid=$!
> kill -STOP $pid
> echo $pid>  /dev/freezer/foo/tasks
> echo FROZEN>  /dev/freezer/foo/freezer.state
> while true
> do
>          cat /dev/freezer/foo/freezer.state
>          [ "`cat /dev/freezer/foo/freezer.state`" = "FROZEN" ]&&  break
>          sleep 1
> done
> echo OK
>
> Signed-off-by: Michal Hocko<mhocko-AlSwsSmVLrQ@public.gmane.org>
> Cc: Tomasz Buchert<tomasz.buchert-MZpvjPyXg2s@public.gmane.org>
> Cc: Paul Menage<paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
> Cc: Li Zefan<lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> Cc: Andrew Morton<akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Tejun Heo<htejun-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   kernel/cgroup_freezer.c |   11 +++++++++--
>   1 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 5e828a2..4d073dc 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -153,6 +153,13 @@ static void freezer_destroy(struct cgroup_subsys *ss,
>   	kfree(cgroup_freezer(cgroup));
>   }
>
> +/* Task is frozen or will freeze immediately when next it gets woken */
> +static bool is_task_frozen_enough(struct task_struct *task)
> +{
> +	return frozen(task) ||
> +		(task_is_stopped_or_traced(task)&&  freezing(task));
> +}
> +
>   /*
>    * The call to cgroup_lock() in the freezer.state write method prevents
>    * a write to that file racing against an attach, and hence the
> @@ -231,7 +238,7 @@ static void update_if_frozen(struct cgroup *cgroup,
>   	cgroup_iter_start(cgroup,&it);
>   	while ((task = cgroup_iter_next(cgroup,&it))) {
>   		ntotal++;
> -		if (frozen(task))
> +		if (is_task_frozen_enough(task))
>   			nfrozen++;
>   	}
>
> @@ -284,7 +291,7 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
>   	while ((task = cgroup_iter_next(cgroup,&it))) {
>   		if (!freeze_task(task, true))
>   			continue;
> -		if (frozen(task))
> +		if (is_task_frozen_enough(task))
>   			continue;
>   		if (!freezing(task)&&  !freezer_should_skip(task))
>   			num_cant_freeze_now++;

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Michal Hocko]

On Thu 17-11-11 11:14:53, Tomasz Buchert wrote:
> Hi,
> I'm trying to understand now why I did that change in
> 
> 2d3cbf8bc (the bug itself was in the if-the-else clause in
> update_if_frozen , anyway).
> Well, when I look at this now I think that there is nothing wrong
> with your patch.
> You can try my testcases from 2d3cbf8bc and 0bdba580, but it should be ok.

Yes, I have tried it before sending the patch.

> One thing I am not sure completely of is the following situation. So
> the group is frozen
> with the STOPPED task inside. There are few questions:
> * if you send SIGCONT to the task now, will it wake up?

No, it will enter refrigerator and wake up on thawing the group. Check
out `goto relock' after do_signal_stop returns.

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Tomasz Buchert]

Le 17/11/2011 17:13, Michal Hocko a écrit :
> On Thu 17-11-11 11:14:53, Tomasz Buchert wrote:
>> Hi,
>> I'm trying to understand now why I did that change in
>>
>> 2d3cbf8bc (the bug itself was in the if-the-else clause in
>> update_if_frozen , anyway).
>> Well, when I look at this now I think that there is nothing wrong
>> with your patch.
>> You can try my testcases from 2d3cbf8bc and 0bdba580, but it should be ok.
> Yes, I have tried it before sending the patch.
>
>> One thing I am not sure completely of is the following situation. So
>> the group is frozen
>> with the STOPPED task inside. There are few questions:
>> * if you send SIGCONT to the task now, will it wake up?
> No, it will enter refrigerator and wake up on thawing the group. Check
> out `goto relock' after do_signal_stop returns.
>
I've checked your patch empirically few hours before :).
Also I've read get_signal_to_deliver() and you are right - it's gonna be 
fine.
The decision is not mine, of course, but I suggest to merge it.
Good work!

Tomek

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Michal Hocko]

Hi Tejun, Li Zefan,
did you have time to look at this patch? I haven't found any
cgroup_freezer maintainer so I guess it should go via generic cgroups
maintainers, right?

Anyway, this is probably not the number one urgent fix as it is broken
since .37 and nobody complained (perhaps people do not tend to freeze
groups with stopped tasks) but LTP does fail on this issue so more
people doing some QA with post .37 kernels will notice.

On Wed 16-11-11 22:50:34, Michal Hocko wrote:
> 2d3cbf8b (cgroup_freezer: update_freezer_state() does incorrect state
> transitions) removed is_task_frozen_enough and replaced it with a simple
> frozen call. This, however, breaks freezing for a group with stopped tasks
> because those cannot be frozen and so the group remains in CGROUP_FREEZING
> state (update_if_frozen doesn't count stopped tasks) and never reaches
> CGROUP_FROZEN.
> 
> Let's add is_task_frozen_enough back and use it at the original locations
> (update_if_frozen and try_to_freeze_cgroup). Semantically we consider
> stopped tasks as frozen enough so we should consider both cases when
> testing frozen tasks.
> 
> Testcase:
> mkdir /dev/freezer
> mount -t cgroup -o freezer none /dev/freezer
> mkdir /dev/freezer/foo
> sleep 1h &
> pid=$!
> kill -STOP $pid
> echo $pid > /dev/freezer/foo/tasks
> echo FROZEN > /dev/freezer/foo/freezer.state
> while true
> do
>         cat /dev/freezer/foo/freezer.state
>         [ "`cat /dev/freezer/foo/freezer.state`" = "FROZEN" ] && break
>         sleep 1
> done
> echo OK
> 
> Signed-off-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
> Cc: Tomasz Buchert <tomasz.buchert-MZpvjPyXg2s@public.gmane.org>
> Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
> Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Tejun Heo <htejun-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  kernel/cgroup_freezer.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
> index 5e828a2..4d073dc 100644
> --- a/kernel/cgroup_freezer.c
> +++ b/kernel/cgroup_freezer.c
> @@ -153,6 +153,13 @@ static void freezer_destroy(struct cgroup_subsys *ss,
>  	kfree(cgroup_freezer(cgroup));
>  }
>  
> +/* Task is frozen or will freeze immediately when next it gets woken */
> +static bool is_task_frozen_enough(struct task_struct *task)
> +{
> +	return frozen(task) ||
> +		(task_is_stopped_or_traced(task) && freezing(task));
> +}
> +
>  /*
>   * The call to cgroup_lock() in the freezer.state write method prevents
>   * a write to that file racing against an attach, and hence the
> @@ -231,7 +238,7 @@ static void update_if_frozen(struct cgroup *cgroup,
>  	cgroup_iter_start(cgroup, &it);
>  	while ((task = cgroup_iter_next(cgroup, &it))) {
>  		ntotal++;
> -		if (frozen(task))
> +		if (is_task_frozen_enough(task))
>  			nfrozen++;
>  	}
>  
> @@ -284,7 +291,7 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
>  	while ((task = cgroup_iter_next(cgroup, &it))) {
>  		if (!freeze_task(task, true))
>  			continue;
> -		if (frozen(task))
> +		if (is_task_frozen_enough(task))
>  			continue;
>  		if (!freezing(task) && !freezer_should_skip(task))
>  			num_cant_freeze_now++;
> -- 
> 1.7.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe cgroups" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Tejun Heo]

Hello, Michal.

On Wed, Nov 16, 2011 at 10:50:34PM +0100, Michal Hocko wrote:
> +/* Task is frozen or will freeze immediately when next it gets woken */
> +static bool is_task_frozen_enough(struct task_struct *task)
> +{
> +	return frozen(task) ||
> +		(task_is_stopped_or_traced(task) && freezing(task));
> +}

Hmmm... w/ pending freezer updates, the above would always return
%true if there's freezing in progress, which can't be right.  Maybe
just test stopped/traced?

  http://git.kernel.org/?p=linux/kernel/git/tj/misc.git;a=shortlog;h=refs/heads/pm-freezer

Thanks.

-- 
tejun

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Li Zefan]

Tejun Heo wrote:
> Hello, Michal.
> 
> On Wed, Nov 16, 2011 at 10:50:34PM +0100, Michal Hocko wrote:
>> +/* Task is frozen or will freeze immediately when next it gets woken */
>> +static bool is_task_frozen_enough(struct task_struct *task)
>> +{
>> +	return frozen(task) ||
>> +		(task_is_stopped_or_traced(task) && freezing(task));
>> +}
> 
> Hmmm... w/ pending freezer updates, the above would always return
> %true if there's freezing in progress, which can't be right.  Maybe

Only if the task is stopped/trace.

If we try to freeze a stopped task, it will be kept in freezing state.

> just test stopped/traced?
> 

This can trigger a BUG_ON in update_if_frozen(), because we always count a
stopped task as frozen.

>   http://git.kernel.org/?p=linux/kernel/git/tj/misc.git;a=shortlog;h=refs/heads/pm-freezer
> 
> Thanks.
> 

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Tejun Heo]

On Tue, Nov 22, 2011 at 10:20:02AM +0800, Li Zefan wrote:
> Tejun Heo wrote:
> > Hello, Michal.
> > 
> > On Wed, Nov 16, 2011 at 10:50:34PM +0100, Michal Hocko wrote:
> >> +/* Task is frozen or will freeze immediately when next it gets woken */
> >> +static bool is_task_frozen_enough(struct task_struct *task)
> >> +{
> >> +	return frozen(task) ||
> >> +		(task_is_stopped_or_traced(task) && freezing(task));
> >> +}
> > 
> > Hmmm... w/ pending freezer updates, the above would always return
> > %true if there's freezing in progress, which can't be right.  Maybe
> 
> Only if the task is stopped/trace.

You're right, missed parantheses.

> If we try to freeze a stopped task, it will be kept in freezing state.
> 
> > just test stopped/traced?
> 
> This can trigger a BUG_ON in update_if_frozen(), because we always count a
> stopped task as frozen.

So, yeah, the patch looks good to me, but it still isn't difficult to
trigger BUG_ON() there.  We need a lot of fixes in cgroup_freezer.

Thanks.

-- 
tejun

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Michal Hocko]

On Mon 21-11-11 18:21:18, Tejun Heo wrote:
> On Tue, Nov 22, 2011 at 10:20:02AM +0800, Li Zefan wrote:
> > Tejun Heo wrote:
> > > Hello, Michal.
> > > 
> > > On Wed, Nov 16, 2011 at 10:50:34PM +0100, Michal Hocko wrote:
> > >> +/* Task is frozen or will freeze immediately when next it gets woken */
> > >> +static bool is_task_frozen_enough(struct task_struct *task)
> > >> +{
> > >> +	return frozen(task) ||
> > >> +		(task_is_stopped_or_traced(task) && freezing(task));
> > >> +}
> > > 
> > > Hmmm... w/ pending freezer updates, the above would always return
> > > %true if there's freezing in progress, which can't be right.  Maybe
> > 
> > Only if the task is stopped/trace.
> 
> You're right, missed parantheses.
> 
> > If we try to freeze a stopped task, it will be kept in freezing state.
> > 
> > > just test stopped/traced?
> > 
> > This can trigger a BUG_ON in update_if_frozen(), because we always count a
> > stopped task as frozen.
> 
> So, yeah, the patch looks good to me, but it still isn't difficult to
> trigger BUG_ON() there.  We need a lot of fixes in cgroup_freezer.

I am not sure which BUG_ON you have in mind.
update_if_frozen:
	BUG_ON(nfrozen != ntotal);

Should be OK because this patch does:
@@ -231,7 +238,7 @@ static void update_if_frozen(struct cgroup *cgroup,
        cgroup_iter_start(cgroup, &it);
        while ((task = cgroup_iter_next(cgroup, &it))) {
                ntotal++;
-               if (frozen(task))
+               if (is_task_frozen_enough(task))

AFAICS your current implementation in (pm-freezer) uses (freezing || frozen)
so the patch should be updated to (freezing || is_task_frozen_enough).

Updated patch - on top of your pm-freezer branch
---
From aba1042a96b6e79e0b14e3c397389389c9e2f522 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
Date: Wed, 16 Nov 2011 22:38:42 +0100
Subject: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

2d3cbf8b (cgroup_freezer: update_freezer_state() does incorrect state
transitions) removed is_task_frozen_enough and replaced it with a simple
frozen call. This, however, breaks freezing for a group with stopped tasks
because those cannot be frozen and so the group remains in CGROUP_FREEZING
state (update_if_frozen doesn't count stopped tasks) and never reaches
CGROUP_FROZEN.

Let's add is_task_frozen_enough back and use it at the original locations
(update_if_frozen and try_to_freeze_cgroup). Semantically we consider
stopped tasks as frozen enough so we should consider both cases when
testing frozen tasks.

Testcase:
mkdir /dev/freezer
mount -t cgroup -o freezer none /dev/freezer
mkdir /dev/freezer/foo
sleep 1h &
pid=$!
kill -STOP $pid
echo $pid > /dev/freezer/foo/tasks
echo FROZEN > /dev/freezer/foo/freezer.state
while true
do
        cat /dev/freezer/foo/freezer.state
        [ "`cat /dev/freezer/foo/freezer.state`" = "FROZEN" ] && break
        sleep 1
done
echo OK

Signed-off-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
Cc: Tomasz Buchert <tomasz.buchert-MZpvjPyXg2s@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Tejun Heo <htejun-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 kernel/cgroup_freezer.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index e411a60..96090a5 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -152,6 +152,13 @@ static void freezer_destroy(struct cgroup_subsys *ss,
 	kfree(freezer);
 }
 
+/* Task is frozen or will freeze immediately when next it gets woken */
+static bool is_task_frozen_enough(struct task_struct *task)
+{
+	return frozen(task) ||
+		(task_is_stopped_or_traced(task) && freezing(task));
+}
+
 /*
  * The call to cgroup_lock() in the freezer.state write method prevents
  * a write to that file racing against an attach, and hence the
@@ -224,7 +231,7 @@ static void update_if_frozen(struct cgroup *cgroup,
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
 		ntotal++;
-		if (freezing(task) && frozen(task))
+		if (freezing(task) || is_task_frozen_enough(task))
 			nfrozen++;
 	}
 
@@ -276,7 +283,7 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 	while ((task = cgroup_iter_next(cgroup, &it))) {
 		if (!freeze_task(task))
 			continue;
-		if (frozen(task))
+		if (is_task_frozen_enough(task))
 			continue;
 		if (!freezing(task) && !freezer_should_skip(task))
 			num_cant_freeze_now++;
-- 
1.7.7.1



Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Tejun Heo]

Hello, Michal.

On Tue, Nov 22, 2011 at 10:05:22AM +0100, Michal Hocko wrote:
> I am not sure which BUG_ON you have in mind.
> update_if_frozen:
> 	BUG_ON(nfrozen != ntotal);

Heh, try to put a kthread into the cgroup and freeze it. :)

> From aba1042a96b6e79e0b14e3c397389389c9e2f522 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
> Date: Wed, 16 Nov 2011 22:38:42 +0100
> Subject: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks
> 
> 2d3cbf8b (cgroup_freezer: update_freezer_state() does incorrect state
> transitions) removed is_task_frozen_enough and replaced it with a simple
> frozen call. This, however, breaks freezing for a group with stopped tasks
> because those cannot be frozen and so the group remains in CGROUP_FREEZING
> state (update_if_frozen doesn't count stopped tasks) and never reaches
> CGROUP_FROZEN.
> 
> Let's add is_task_frozen_enough back and use it at the original locations
> (update_if_frozen and try_to_freeze_cgroup). Semantically we consider
> stopped tasks as frozen enough so we should consider both cases when
> testing frozen tasks.
> 
> Testcase:
> mkdir /dev/freezer
> mount -t cgroup -o freezer none /dev/freezer
> mkdir /dev/freezer/foo
> sleep 1h &
> pid=$!
> kill -STOP $pid
> echo $pid > /dev/freezer/foo/tasks
> echo FROZEN > /dev/freezer/foo/freezer.state
> while true
> do
>         cat /dev/freezer/foo/freezer.state
>         [ "`cat /dev/freezer/foo/freezer.state`" = "FROZEN" ] && break
>         sleep 1
> done
> echo OK
> 
> Signed-off-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
> Cc: Tomasz Buchert <tomasz.buchert-MZpvjPyXg2s@public.gmane.org>
> Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
> Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Tejun Heo <htejun-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Applying to cgroup/for-3.2-fixes.

Thank you.

-- 
tejun

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Tejun Heo]

On Tue, Nov 22, 2011 at 07:39:48AM -0800, Tejun Heo wrote:
> > Signed-off-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
> > Cc: Tomasz Buchert <tomasz.buchert-MZpvjPyXg2s@public.gmane.org>
> > Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
> > Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> > Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> > Cc: Tejun Heo <htejun-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Applying to cgroup/for-3.2-fixes.

I'll apply the original version in cgroup/for-3.2-fixes and deal with
conflict once pm-freezer branch is merged.  Li, can you please ack
this?

Thank you.

-- 
tejun

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Michal Hocko]

On Tue 22-11-11 07:39:48, Tejun Heo wrote:
> Hello, Michal.
> 
> On Tue, Nov 22, 2011 at 10:05:22AM +0100, Michal Hocko wrote:
> > I am not sure which BUG_ON you have in mind.
> > update_if_frozen:
> > 	BUG_ON(nfrozen != ntotal);
> 
> Heh, try to put a kthread into the cgroup and freeze it. :)

Ahh got it. So not related to the original issue. Just yet another issue
in the area.

Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Li Zefan]

Tejun Heo wrote:
> On Tue, Nov 22, 2011 at 07:39:48AM -0800, Tejun Heo wrote:
>>> Signed-off-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
>>> Cc: Tomasz Buchert <tomasz.buchert-MZpvjPyXg2s@public.gmane.org>
>>> Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
>>> Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
>>> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
>>> Cc: Tejun Heo <htejun-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> Applying to cgroup/for-3.2-fixes.
> 
> I'll apply the original version in cgroup/for-3.2-fixes and deal with
> conflict once pm-freezer branch is merged.  Li, can you please ack
> this?
> 

Sure.

Acked-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Michal Hocko]

On Tue 22-11-11 07:48:06, Tejun Heo wrote:
> On Tue, Nov 22, 2011 at 07:39:48AM -0800, Tejun Heo wrote:
> > > Signed-off-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
> > > Cc: Tomasz Buchert <tomasz.buchert-MZpvjPyXg2s@public.gmane.org>
> > > Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
> > > Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> > > Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> > > Cc: Tejun Heo <htejun-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 
> > Applying to cgroup/for-3.2-fixes.
> 
> I'll apply the original version in cgroup/for-3.2-fixes and deal with
> conflict once pm-freezer branch is merged.  

Are you going to push this to the stable trees as well, or should I do
it?

Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Tejun Heo]

On Thu, Nov 24, 2011 at 11:04:54AM +0100, Michal Hocko wrote:
> On Tue 22-11-11 07:48:06, Tejun Heo wrote:
> > On Tue, Nov 22, 2011 at 07:39:48AM -0800, Tejun Heo wrote:
> > > > Signed-off-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
> > > > Cc: Tomasz Buchert <tomasz.buchert-MZpvjPyXg2s@public.gmane.org>
> > > > Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
> > > > Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> > > > Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> > > > Cc: Tejun Heo <htejun-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > 
> > > Applying to cgroup/for-3.2-fixes.
> > 
> > I'll apply the original version in cgroup/for-3.2-fixes and deal with
> > conflict once pm-freezer branch is merged.  
> 
> Are you going to push this to the stable trees as well, or should I do
> it?

Tagged w/ stable.  Will push it upstream next week and the next stable
release will pick it up.

Thanks.

-- 
tejun

Re: [PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Michal Hocko]

On Thu 24-11-11 12:00:09, Tejun Heo wrote:
> On Thu, Nov 24, 2011 at 11:04:54AM +0100, Michal Hocko wrote:
> > On Tue 22-11-11 07:48:06, Tejun Heo wrote:
> > > On Tue, Nov 22, 2011 at 07:39:48AM -0800, Tejun Heo wrote:
> > > > > Signed-off-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
> > > > > Cc: Tomasz Buchert <tomasz.buchert-MZpvjPyXg2s@public.gmane.org>
> > > > > Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
> > > > > Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> > > > > Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> > > > > Cc: Tejun Heo <htejun-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > 
> > > > Applying to cgroup/for-3.2-fixes.
> > > 
> > > I'll apply the original version in cgroup/for-3.2-fixes and deal with
> > > conflict once pm-freezer branch is merged.  
> > 
> > Are you going to push this to the stable trees as well, or should I do
> > it?
> 
> Tagged w/ stable.  Will push it upstream next week and the next stable
> release will pick it up.

Great, thanks!

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

[PATCH] cgroup_freezer: fix freezing groups with stopped tasks

[Michal Hocko]

2d3cbf8b (cgroup_freezer: update_freezer_state() does incorrect state
transitions) removed is_task_frozen_enough and replaced it with a simple
frozen call. This, however, breaks freezing for a group with stopped tasks
because those cannot be frozen and so the group remains in CGROUP_FREEZING
state (update_if_frozen doesn't count stopped tasks) and never reaches
CGROUP_FROZEN.

Let's add is_task_frozen_enough back and use it at the original locations
(update_if_frozen and try_to_freeze_cgroup). Semantically we consider
stopped tasks as frozen enough so we should consider both cases when
testing frozen tasks.

Testcase:
mkdir /dev/freezer
mount -t cgroup -o freezer none /dev/freezer
mkdir /dev/freezer/foo
sleep 1h &
pid=$!
kill -STOP $pid
echo $pid > /dev/freezer/foo/tasks
echo FROZEN > /dev/freezer/foo/freezer.state
while true
do
        cat /dev/freezer/foo/freezer.state
        [ "`cat /dev/freezer/foo/freezer.state`" = "FROZEN" ] && break
        sleep 1
done
echo OK

Signed-off-by: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
Cc: Tomasz Buchert <tomasz.buchert-MZpvjPyXg2s@public.gmane.org>
Cc: Paul Menage <paul-inf54ven1CmVyaH7bEyXVA@public.gmane.org>
Cc: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
Cc: Tejun Heo <htejun-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 kernel/cgroup_freezer.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index 5e828a2..4d073dc 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -153,6 +153,13 @@ static void freezer_destroy(struct cgroup_subsys *ss,
 	kfree(cgroup_freezer(cgroup));
 }
 
+/* Task is frozen or will freeze immediately when next it gets woken */
+static bool is_task_frozen_enough(struct task_struct *task)
+{
+	return frozen(task) ||
+		(task_is_stopped_or_traced(task) && freezing(task));
+}
+
 /*
  * The call to cgroup_lock() in the freezer.state write method prevents
  * a write to that file racing against an attach, and hence the
@@ -231,7 +238,7 @@ static void update_if_frozen(struct cgroup *cgroup,
 	cgroup_iter_start(cgroup, &it);
 	while ((task = cgroup_iter_next(cgroup, &it))) {
 		ntotal++;
-		if (frozen(task))
+		if (is_task_frozen_enough(task))
 			nfrozen++;
 	}
 
@@ -284,7 +291,7 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 	while ((task = cgroup_iter_next(cgroup, &it))) {
 		if (!freeze_task(task, true))
 			continue;
-		if (frozen(task))
+		if (is_task_frozen_enough(task))
 			continue;
 		if (!freezing(task) && !freezer_should_skip(task))
 			num_cant_freeze_now++;
-- 
1.7.7.1