Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Oleg Nesterov]

> Fix the issue by checking for TIF_MEMDIE thread flag and get away from the
> fridge if it is set.  oom_scan_process_thread doesn't have to check for
> the frozen task anymore because do_send_sig_info will wake up the thread
> and TIF_MEMDIE is already set by that time.

I must have missed something... but __refrigerator() sleeps in
TASK_UNINTERRUPTIBLE and do_send_sig_info() won't wake it up?

Oleg.

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Cong Wang]

On Thu, Oct 16, 2014 at 1:39 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> Fix the issue by checking for TIF_MEMDIE thread flag and get away from the
>> fridge if it is set.  oom_scan_process_thread doesn't have to check for
>> the frozen task anymore because do_send_sig_info will wake up the thread
>> and TIF_MEMDIE is already set by that time.
>
> I must have missed something... but __refrigerator() sleeps in
> TASK_UNINTERRUPTIBLE and do_send_sig_info() won't wake it up?
>

This is exactly what we are trying to fix. Make sure you read the patch
as well before reply? Otherwise, I must have missed your point. :)

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Oleg Nesterov]

On 10/16, Cong Wang wrote:
>
> On Thu, Oct 16, 2014 at 1:39 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> >> Fix the issue by checking for TIF_MEMDIE thread flag and get away from the
> >> fridge if it is set.  oom_scan_process_thread doesn't have to check for
> >> the frozen task anymore because do_send_sig_info will wake up the thread
> >> and TIF_MEMDIE is already set by that time.
> >
> > I must have missed something... but __refrigerator() sleeps in
> > TASK_UNINTERRUPTIBLE and do_send_sig_info() won't wake it up?
> >
>
> This is exactly what we are trying to fix. Make sure you read the patch
> as well before reply?

I did read the patch, but I can't understand it. I am sorry about that,
and I am asking for your help.

I agree that oom_scan_process_thread()->__thaw_task() doesn't really
help.

But also I can't understand why this patch helps. The changelog says:

	do_send_sig_info will wake up the thread

why?

Oleg.

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Cong Wang]

On Thu, Oct 16, 2014 at 2:11 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> But also I can't understand why this patch helps. The changelog says:
>
>         do_send_sig_info will wake up the thread
>
> why?
>

This is a question for Michal who rewrites my changelog:

http://marc.info/?l=linux-kernel&m=140986986423092&w=2

:)

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Oleg Nesterov]

On 10/16, Cong Wang wrote:
>
> On Thu, Oct 16, 2014 at 2:11 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > But also I can't understand why this patch helps. The changelog says:
> >
> >         do_send_sig_info will wake up the thread
> >
> > why?
> >
>
> This is a question for Michal who rewrites my changelog:
>
> http://marc.info/?l=linux-kernel&m=140986986423092&w=2
>
> :)

OK, I hope Michal can answer my question if you do not want to
do this ;) So far I think this patch is not right.

If a task B is already frozen, it sleeps in D state.

If OOM selects B as a victim after that, it won't be woken by
SIGKILL, thus it obviously can't call should_thaw_current() and
notice TIF_MEMDIE.

Btw, I also do not understand the cgroup_freezing() check in
should_thaw_current(), but this is another story.

Oleg.	

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Cong Wang]

On Thu, Oct 16, 2014 at 2:35 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> If a task B is already frozen, it sleeps in D state.
>
> If OOM selects B as a victim after that, it won't be woken by
> SIGKILL, thus it obviously can't call should_thaw_current() and
> notice TIF_MEMDIE.

I see your point now, it would be more clear if you can just quote
the patch instead of changelog.

So are you saying the loop in __refrigerator() is useless? Since
it will always stay in asleep after schedule()?

>
> Btw, I also do not understand the cgroup_freezing() check in
> should_thaw_current(), but this is another story.
>

I hate to repeat the previous discussion. Maybe you can just follow
the link I gave to you? :)

Thanks.

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Oleg Nesterov]

On 10/16, Cong Wang wrote:
>
> On Thu, Oct 16, 2014 at 2:35 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > If a task B is already frozen, it sleeps in D state.
> >
> > If OOM selects B as a victim after that, it won't be woken by
> > SIGKILL, thus it obviously can't call should_thaw_current() and
> > notice TIF_MEMDIE.
>
> I see your point now, it would be more clear if you can just quote
> the patch instead of changelog.
>
> So are you saying the loop in __refrigerator() is useless?

No.

> Since
> it will always stay in asleep after schedule()?

Not always. But it will stay asleep in this particular case.

> > Btw, I also do not understand the cgroup_freezing() check in
> > should_thaw_current(), but this is another story.
>
> I hate to repeat the previous discussion. Maybe you can just follow
> the link I gave to you? :)

May be, but this thread is huge. Will try tomorrow to read it tomorrow,
but you know, I hope that someone else from cc list can copy-and-paste the
relevant part of this discussion, or give me the link to some specific
email. To me the comment should be more clear in any case, but perhaps
it is just me who can't understand it immediately.

Oleg.

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Cong Wang]

On Thu, Oct 16, 2014 at 3:22 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 10/16, Cong Wang wrote:
>>
>> On Thu, Oct 16, 2014 at 2:35 PM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> > If a task B is already frozen, it sleeps in D state.
>> >
>> > If OOM selects B as a victim after that, it won't be woken by
>> > SIGKILL, thus it obviously can't call should_thaw_current() and
>> > notice TIF_MEMDIE.
>>
>> I see your point now, it would be more clear if you can just quote
>> the patch instead of changelog.
>>
>> So are you saying the loop in __refrigerator() is useless?
>
> No.
>
>> Since
>> it will always stay in asleep after schedule()?
>
> Not always. But it will stay asleep in this particular case.

Hmm, so we still need to wake it up in oom killer:

        if (test_tsk_thread_flag(task, TIF_MEMDIE)) {
               if (unlikely(frozen(task)))
                      wake_up_state(task, TASK_UNINTERRUPTIBLE);

I will update the patch if Michal doesn't.

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Michal Hocko]

On Thu 16-10-14 22:39:54, Oleg Nesterov wrote:
> > Fix the issue by checking for TIF_MEMDIE thread flag and get away from the
> > fridge if it is set.  oom_scan_process_thread doesn't have to check for
> > the frozen task anymore because do_send_sig_info will wake up the thread
> > and TIF_MEMDIE is already set by that time.
> 
> I must have missed something... but __refrigerator() sleeps in
> TASK_UNINTERRUPTIBLE and do_send_sig_info() won't wake it up?

Ouch, I have completely missed this part when reviewing Cong's original
patch. I got confused by the retry loop. Anyway you are right and the
patch as is currently doesn't work as intented.

Can we simply make the task sleep interruptible? It retries and rechecks
the freezing conditions after wakeup anyway. It is true this would be a
user visible change because frozen tasks won't be in D state anymore
(this would make a difference for cgroup freezing because nobody would
see this in PM freezer). Does anybody depend on this?

Another possible way would be reintroducing freezer check into OOM path
and kick the task even when it is in UN state.

Thanks!
-- 
Michal Hocko
SUSE Labs

[PATCH -v2] freezer: check OOM kill while being frozen

[Michal Hocko]

On Thu 16-10-14 19:33:39, Cong Wang wrote:
> On Thu, Oct 16, 2014 at 3:22 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 10/16, Cong Wang wrote:
> >>
> >> On Thu, Oct 16, 2014 at 2:35 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> >> >
> >> > If a task B is already frozen, it sleeps in D state.
> >> >
> >> > If OOM selects B as a victim after that, it won't be woken by
> >> > SIGKILL, thus it obviously can't call should_thaw_current() and
> >> > notice TIF_MEMDIE.
> >>
> >> I see your point now, it would be more clear if you can just quote
> >> the patch instead of changelog.
> >>
> >> So are you saying the loop in __refrigerator() is useless?
> >
> > No.
> >
> >> Since
> >> it will always stay in asleep after schedule()?
> >
> > Not always. But it will stay asleep in this particular case.
> 
> Hmm, so we still need to wake it up in oom killer:
> 
>         if (test_tsk_thread_flag(task, TIF_MEMDIE)) {
>                if (unlikely(frozen(task)))
>                       wake_up_state(task, TASK_UNINTERRUPTIBLE);
> 
> I will update the patch if Michal doesn't.

I think we should rather get back to __thaw_task here.

Andrew could you replace the previous version by this one, please? Again
I am sorry I haven't caught that before.
---
>From 830324c8be8b499e1d18a73076cf4d126c4ac486 Mon Sep 17 00:00:00 2001
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu, 4 Sep 2014 15:30:41 -0700
Subject: [PATCH -v2] freezer: check OOM kill while being frozen

Since f660daac474c6f (oom: thaw threads if oom killed thread is frozen
before deferring) OOM killer relies on being able to thaw a frozen task
to handle OOM situation but a3201227f803 (freezer: make freezing() test
freeze conditions in effect instead of TIF_FREEZE) has reorganized the
code and stopped clearing freeze flag in __thaw_task. This means that
the target task only wakes up and goes into the fridge again because the
freezing condition hasn't changed for it. This reintroduces the bug
fixed by f660daac474c6f.

Fix the issue by checking for TIF_MEMDIE thread flag and get away from
the fridge if it is set.

Changes since v1
- return __thaw_task into oom_scan_process_thread because
  oom_kill_process will not wake task in the fridge because it is
  sleeping uninterruptible

[mhocko@suse.cz: rewrote the changelog]
Fixes: a3201227f803 (freezer: make freezing() test freeze conditions in effect instead of TIF_FREEZE)
Cc: stable@vger.kernel.org # 3.3+
Cc: David Rientjes <rientjes@google.com>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Acked-by: Michal Hocko <mhocko@suse.cz>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 kernel/freezer.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index aa6a8aadb911..77ad6794b610 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -45,13 +45,28 @@ bool freezing_slow_path(struct task_struct *p)
 	if (pm_nosig_freezing || cgroup_freezing(p))
 		return true;
 
-	if (pm_freezing && !(p->flags & PF_KTHREAD))
+	if (!(p->flags & PF_KTHREAD))
 		return true;
 
 	return false;
 }
 EXPORT_SYMBOL(freezing_slow_path);
 
+static bool should_thaw_current(bool check_kthr_stop)
+{
+	if (!freezing(current))
+		return true;
+
+	if (check_kthr_stop && kthread_should_stop())
+		return true;
+
+	/* It might not be safe to check TIF_MEMDIE for pm freeze. */
+	if (cgroup_freezing(current) && test_thread_flag(TIF_MEMDIE))
+		return true;
+
+	return false;
+}
+
 /* Refrigerator is place where frozen processes are stored :-). */
 bool __refrigerator(bool check_kthr_stop)
 {
@@ -67,8 +82,7 @@ bool __refrigerator(bool check_kthr_stop)
 
 		spin_lock_irq(&freezer_lock);
 		current->flags |= PF_FROZEN;
-		if (!freezing(current) ||
-		    (check_kthr_stop && kthread_should_stop()))
+		if (should_thaw_current(check_kthr_stop))
 			current->flags &= ~PF_FROZEN;
 		spin_unlock_irq(&freezer_lock);
 
-- 
2.1.1

-- 
Michal Hocko
SUSE Labs

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Oleg Nesterov]

On 10/16, Cong Wang wrote:
>
> On Thu, Oct 16, 2014 at 3:22 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 10/16, Cong Wang wrote:
> >>
> >> it will always stay in asleep after schedule()?
> >
> > Not always. But it will stay asleep in this particular case.
>
> Hmm, so we still need to wake it up in oom killer:
>
>         if (test_tsk_thread_flag(task, TIF_MEMDIE)) {
>                if (unlikely(frozen(task)))
>                       wake_up_state(task, TASK_UNINTERRUPTIBLE);
>
> I will update the patch if Michal doesn't.

I think it would be better to simply keep that __thaw_task() in
oom_scan_process_thread().

Oleg.

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Oleg Nesterov]

On 10/17, Michal Hocko wrote:
>
> Can we simply make the task sleep interruptible?

No, this will turn __refrigerator() into the busy-wait loop if
signal_pending() == T.


(but I still think it makes sense to kill PF_FROZEN and introduce
 TASK_FROZEN state, however this is offtopic).

Oleg.

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Michal Hocko]

On Fri 17-10-14 17:31:55, Oleg Nesterov wrote:
> On 10/17, Michal Hocko wrote:
> >
> > Can we simply make the task sleep interruptible?
> 
> No, this will turn __refrigerator() into the busy-wait loop if
> signal_pending() == T.

good point.
 
> (but I still think it makes sense to kill PF_FROZEN and introduce
>  TASK_FROZEN state, however this is offtopic).
> 
> Oleg.
> 

-- 
Michal Hocko
SUSE Labs

Re: + freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[Michal Hocko]

On Fri 17-10-14 17:24:29, Oleg Nesterov wrote:
> On 10/16, Cong Wang wrote:
> >
> > On Thu, Oct 16, 2014 at 3:22 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> > > On 10/16, Cong Wang wrote:
> > >>
> > >> it will always stay in asleep after schedule()?
> > >
> > > Not always. But it will stay asleep in this particular case.
> >
> > Hmm, so we still need to wake it up in oom killer:
> >
> >         if (test_tsk_thread_flag(task, TIF_MEMDIE)) {
> >                if (unlikely(frozen(task)))
> >                       wake_up_state(task, TASK_UNINTERRUPTIBLE);
> >
> > I will update the patch if Michal doesn't.
> 
> I think it would be better to simply keep that __thaw_task() in
> oom_scan_process_thread().

yeah, v2 of the patch (I guess you were on CC) does exactly this.

Thanks!
-- 
Michal Hocko
SUSE Labs

Re: [PATCH -v2] freezer: check OOM kill while being frozen

[Oleg Nesterov]

On 10/17, Michal Hocko wrote:
>
> I think we should rather get back to __thaw_task here.

Yes, agreed.

> Andrew could you replace the previous version by this one, please?

Yes, that patch should be dropped...


And can't resist... please look at
http://marc.info/?l=linux-kernel&m=138427535430827 ;)

> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -45,13 +45,28 @@ bool freezing_slow_path(struct task_struct *p)
>  	if (pm_nosig_freezing || cgroup_freezing(p))
>  		return true;
>
> -	if (pm_freezing && !(p->flags & PF_KTHREAD))
> +	if (!(p->flags & PF_KTHREAD))

Why? Doesn't this mean that try_to_freeze() can race with thaw_processes()
and then this task can be frozen for no reazon?

> +static bool should_thaw_current(bool check_kthr_stop)
> +{
> +	if (!freezing(current))
> +		return true;
> +
> +	if (check_kthr_stop && kthread_should_stop())
> +		return true;
> +
> +	/* It might not be safe to check TIF_MEMDIE for pm freeze. */
> +	if (cgroup_freezing(current) && test_thread_flag(TIF_MEMDIE))

I still think that the comment should tell more to explain why this
is not safe.

And if this is not safe, it is not clear how/why cgroup_freezing() can
save us, both pm_freezing and CGROUP_FREEZING can be true?

And I think that this TIF_MEMDIE should go into freezing_slow_path(),
so we do not even need should_thaw_current().

This also looks more safe to me. Suppose that a task does

	while (try_to_freeze())
		;

Yes, this is pointless but correct. And in fact I think this pattern
is possible. If this task is killed by OOM, it will spin forever.

Oleg.

Re: [PATCH -v2] freezer: check OOM kill while being frozen

[Michal Hocko]

On Fri 17-10-14 18:10:21, Oleg Nesterov wrote:
> On 10/17, Michal Hocko wrote:
> >
> > I think we should rather get back to __thaw_task here.
> 
> Yes, agreed.
> 
> > Andrew could you replace the previous version by this one, please?
> 
> Yes, that patch should be dropped...

Andrew, drop all three patches. I will think about things raised here by
Oleg and come back sometimes next week.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH -v2] freezer: check OOM kill while being frozen

[Michal Hocko]

On Fri 17-10-14 18:10:21, Oleg Nesterov wrote:
> On 10/17, Michal Hocko wrote:
> >
> > I think we should rather get back to __thaw_task here.
> 
> Yes, agreed.
> 
> > Andrew could you replace the previous version by this one, please?
> 
> Yes, that patch should be dropped...
> 
> 
> And can't resist... please look at
> http://marc.info/?l=linux-kernel&m=138427535430827 ;)
> 
> > --- a/kernel/freezer.c
> > +++ b/kernel/freezer.c
> > @@ -45,13 +45,28 @@ bool freezing_slow_path(struct task_struct *p)
> >  	if (pm_nosig_freezing || cgroup_freezing(p))
> >  		return true;
> >
> > -	if (pm_freezing && !(p->flags & PF_KTHREAD))
> > +	if (!(p->flags & PF_KTHREAD))
> 
> Why? Doesn't this mean that try_to_freeze() can race with thaw_processes()
> and then this task can be frozen for no reazon?

Hmm, this wasn't there in the v4 of the original patch from Cong.
http://marc.info/?l=linux-kernel&m=140986986423092. I cannot find any
reference to this hunk and it might be misapplied patch when I took the
patch from the list. I do not see any reason for this change.

> > +static bool should_thaw_current(bool check_kthr_stop)
> > +{
> > +	if (!freezing(current))
> > +		return true;
> > +
> > +	if (check_kthr_stop && kthread_should_stop())
> > +		return true;
> > +
> > +	/* It might not be safe to check TIF_MEMDIE for pm freeze. */
> > +	if (cgroup_freezing(current) && test_thread_flag(TIF_MEMDIE))
> 
> I still think that the comment should tell more to explain why this
> is not safe.

I have suggested removing this check here:
http://marc.info/?l=linux-kernel&m=141078015130905 and it shouldn't be
really here. I must have screwed something up when rebasing the
series... Sorry about that!

Anyway the leader of the series should describe what was unsafe and how
it got fixed: http://marc.info/?l=linux-mm&m=141277728508500&w=2
 
> And if this is not safe, it is not clear how/why cgroup_freezing() can
> save us, both pm_freezing and CGROUP_FREEZING can be true?

You mean that the pm_freezer would race with cgroup one?

> And I think that this TIF_MEMDIE should go into freezing_slow_path(),
> so we do not even need should_thaw_current().

OK, it would make the patch simpler. On the other hand having the check
in the __refrigerator makes it easier to follow. freezing is called from
too many places. But I see your point, I guess. It really doesn't make
sense to go into fridge when it is clear that the task wouldn't get
frozen anyway. Some users even check the return value of freezing and do
different things in two paths. Those seem to be mostly kernel threads
but I haven't checked all the places. Anyway this should be irrelevant 
to the OOM POV.

> This also looks more safe to me. Suppose that a task does
> 
> 	while (try_to_freeze())
> 		;
> 
> Yes, this is pointless but correct. And in fact I think this pattern
> is possible. If this task is killed by OOM, it will spin forever.

I am really not sure what such a code would be supposed to do.

Anyway, updated patch is below. I have still kept Cong as the original
author but please let me know if this is not OK after considerable
changes in the patch.
Does it make more sense to you now, Oleg?
---
>From 6e8b92e7133307e30afe35c6a0637cb58c0fc147 Mon Sep 17 00:00:00 2001
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 20 Oct 2014 17:16:01 +0200
Subject: [PATCH] freezer: check OOM kill while being frozen

Since f660daac474c6f (oom: thaw threads if oom killed thread is frozen
before deferring) OOM killer relies on being able to thaw a frozen task
to handle OOM situation but a3201227f803 (freezer: make freezing() test
freeze conditions in effect instead of TIF_FREEZE) has reorganized the
code and stopped clearing freeze flag in __thaw_task. This means that
the target task only wakes up and goes into the fridge again because the
freezing condition hasn't changed for it. This reintroduces the bug
fixed by f660daac474c6f.

Fix the issue by checking for TIF_MEMDIE thread flag in
freezing_slow_path and exclude the task from freezing completely. If a
task was already frozen it would get woken by __thaw_task from OOM killer
and get out of freezer after rechecking freezing().

Changes since v1
- put TIF_MEMDIE check into freezing_slowpath rather than in __refrigerator
  as per Oleg
- return __thaw_task into oom_scan_process_thread because
  oom_kill_process will not wake task in the fridge because it is
  sleeping uninterruptible

[mhocko@suse.cz: rewrote the changelog]
Fixes: a3201227f803 (freezer: make freezing() test freeze conditions in effect instead of TIF_FREEZE)
Cc: stable@vger.kernel.org # 3.3+
Cc: David Rientjes <rientjes@google.com>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Acked-by: Michal Hocko <mhocko@suse.cz>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 kernel/freezer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index aa6a8aadb911..8f9279b9c6d7 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -42,6 +42,9 @@ bool freezing_slow_path(struct task_struct *p)
 	if (p->flags & (PF_NOFREEZE | PF_SUSPEND_TASK))
 		return false;
 
+	if (test_thread_flag(TIF_MEMDIE))
+		return false;
+
 	if (pm_nosig_freezing || cgroup_freezing(p))
 		return true;
 
-- 
2.1.1

-- 
Michal Hocko
SUSE Labs

Re: [PATCH -v2] freezer: check OOM kill while being frozen

[Oleg Nesterov]

On 10/20, Michal Hocko wrote:
>
> On Fri 17-10-14 18:10:21, Oleg Nesterov wrote:
>
> > And if this is not safe, it is not clear how/why cgroup_freezing() can
> > save us, both pm_freezing and CGROUP_FREEZING can be true?
>
> You mean that the pm_freezer would race with cgroup one?

Yes, so if we actually want this check we should probably check
!pm_freezing or update the comment.

Nevermind, you removed this check and I agree. Even if we add it back
for some reason, it can come in a separate patch with the detailed
explanation.

> > And I think that this TIF_MEMDIE should go into freezing_slow_path(),
> > so we do not even need should_thaw_current().
>
> OK, it would make the patch simpler. On the other hand having the check
> in the __refrigerator makes it easier to follow. freezing is called from
> too many places. But I see your point, I guess. It really doesn't make
> sense to go into fridge when it is clear that the task wouldn't get
> frozen anyway. Some users even check the return value of freezing and do
> different things in two paths. Those seem to be mostly kernel threads
> but I haven't checked all the places. Anyway this should be irrelevant
> to the OOM POV.

Yes, thanks.

> > This also looks more safe to me. Suppose that a task does
> >
> > 	while (try_to_freeze())
> > 		;
> >
> > Yes, this is pointless but correct. And in fact I think this pattern
> > is possible. If this task is killed by OOM, it will spin forever.
>
> I am really not sure what such a code would be supposed to do.

and I actually meant

	while (freezing())
		try_to_freeze();

yes, sure, this looks strange and pointless. But still correct. And you
never know what some driver can do. This pattern can be hidden in a more
complex code, say,

	for (;;) {
		lock_something();
		// we can't use wait_event_freezable() under the lock
		wait_event_interruptible(condition() || freezing());
		// check this before signal_pending() to avoid the restart,
		// or we can't restart, or it was just written this way for
		// no reason.
		if (freezing()) {
			unlock_something();
			try_to_freeze();
			continue;
		}
		unlock_something();

		if (signal_pending())
			break;

		...
	}

sure, most probably the code like this asks for cleanups, but it is easy
to notice that it is actually wrong if try_to_freeze() could return with
freezing() == T. But I agree, this issue is minor.

> Does it make more sense to you now, Oleg?

Thanks!

Acked-by: Oleg Nesterov <oleg@redhat.com>





> From 6e8b92e7133307e30afe35c6a0637cb58c0fc147 Mon Sep 17 00:00:00 2001
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Mon, 20 Oct 2014 17:16:01 +0200
> Subject: [PATCH] freezer: check OOM kill while being frozen
> 
> Since f660daac474c6f (oom: thaw threads if oom killed thread is frozen
> before deferring) OOM killer relies on being able to thaw a frozen task
> to handle OOM situation but a3201227f803 (freezer: make freezing() test
> freeze conditions in effect instead of TIF_FREEZE) has reorganized the
> code and stopped clearing freeze flag in __thaw_task. This means that
> the target task only wakes up and goes into the fridge again because the
> freezing condition hasn't changed for it. This reintroduces the bug
> fixed by f660daac474c6f.
> 
> Fix the issue by checking for TIF_MEMDIE thread flag in
> freezing_slow_path and exclude the task from freezing completely. If a
> task was already frozen it would get woken by __thaw_task from OOM killer
> and get out of freezer after rechecking freezing().
> 
> Changes since v1
> - put TIF_MEMDIE check into freezing_slowpath rather than in __refrigerator
>   as per Oleg
> - return __thaw_task into oom_scan_process_thread because
>   oom_kill_process will not wake task in the fridge because it is
>   sleeping uninterruptible
> 
> [mhocko@suse.cz: rewrote the changelog]
> Fixes: a3201227f803 (freezer: make freezing() test freeze conditions in effect instead of TIF_FREEZE)
> Cc: stable@vger.kernel.org # 3.3+
> Cc: David Rientjes <rientjes@google.com>
> Cc: Michal Hocko <mhocko@suse.cz>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Acked-by: Michal Hocko <mhocko@suse.cz>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> ---
>  kernel/freezer.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index aa6a8aadb911..8f9279b9c6d7 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -42,6 +42,9 @@ bool freezing_slow_path(struct task_struct *p)
>  	if (p->flags & (PF_NOFREEZE | PF_SUSPEND_TASK))
>  		return false;
>  
> +	if (test_thread_flag(TIF_MEMDIE))
> +		return false;
> +
>  	if (pm_nosig_freezing || cgroup_freezing(p))
>  		return true;
>  
> -- 
> 2.1.1
> 
> -- 
> Michal Hocko
> SUSE Labs

+ freezer-check-oom-kill-while-being-frozen.patch added to -mm tree

[akpm]

The patch titled
     Subject: freezer: check OOM kill while being frozen
has been added to the -mm tree.  Its filename is
     freezer-check-oom-kill-while-being-frozen.patch

This patch should soon appear at
    http://ozlabs.org/~akpm/mmots/broken-out/freezer-check-oom-kill-while-being-frozen.patch
and later at
    http://ozlabs.org/~akpm/mmotm/broken-out/freezer-check-oom-kill-while-being-frozen.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

The -mm tree is included into linux-next and is updated
there every 3-4 working days

------------------------------------------------------
From: Cong Wang <xiyou.wangcong@gmail.com>
Subject: freezer: check OOM kill while being frozen

Since f660daac474c6f ("oom: thaw threads if oom killed thread is frozen
before deferring") OOM killer relies on being able to thaw a frozen task
to handle OOM situation but a3201227f803 (freezer: make freezing() test
freeze conditions in effect instead of TIF_FREEZE) has reorganized the
code and stopped clearing freeze flag in __thaw_task.  This means that the
target task only wakes up and goes into the fridge again because the
freezing condition hasn't changed for it.  This reintroduces the bug fixed
by f660daac474c6f.

Fix the issue by checking for TIF_MEMDIE thread flag and get away from the
fridge if it is set.  oom_scan_process_thread doesn't have to check for
the frozen task anymore because do_send_sig_info will wake up the thread
and TIF_MEMDIE is already set by that time.

Fixes: a3201227f803 (freezer: make freezing() test freeze conditions in effect instead of TIF_FREEZE)
[mhocko@suse.cz: rewrote the changelog]
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
Acked-by: Michal Hocko <mhocko@suse.cz>
Cc: David Rientjes <rientjes@google.com>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Tejun Heo <tj@kernel.org>
Cc: <stable@vger.kernel.org>	[3.3+]
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/freezer.c |   20 +++++++++++++++++---
 mm/oom_kill.c    |    2 --
 2 files changed, 17 insertions(+), 5 deletions(-)

diff -puN kernel/freezer.c~freezer-check-oom-kill-while-being-frozen kernel/freezer.c
--- a/kernel/freezer.c~freezer-check-oom-kill-while-being-frozen
+++ a/kernel/freezer.c
@@ -45,13 +45,28 @@ bool freezing_slow_path(struct task_stru
 	if (pm_nosig_freezing || cgroup_freezing(p))
 		return true;
 
-	if (pm_freezing && !(p->flags & PF_KTHREAD))
+	if (!(p->flags & PF_KTHREAD))
 		return true;
 
 	return false;
 }
 EXPORT_SYMBOL(freezing_slow_path);
 
+static bool should_thaw_current(bool check_kthr_stop)
+{
+	if (!freezing(current))
+		return true;
+
+	if (check_kthr_stop && kthread_should_stop())
+		return true;
+
+	/* It might not be safe to check TIF_MEMDIE for pm freeze. */
+	if (cgroup_freezing(current) && test_thread_flag(TIF_MEMDIE))
+		return true;
+
+	return false;
+}
+
 /* Refrigerator is place where frozen processes are stored :-). */
 bool __refrigerator(bool check_kthr_stop)
 {
@@ -67,8 +82,7 @@ bool __refrigerator(bool check_kthr_stop
 
 		spin_lock_irq(&freezer_lock);
 		current->flags |= PF_FROZEN;
-		if (!freezing(current) ||
-		    (check_kthr_stop && kthread_should_stop()))
+		if (should_thaw_current(check_kthr_stop))
 			current->flags &= ~PF_FROZEN;
 		spin_unlock_irq(&freezer_lock);
 
diff -puN mm/oom_kill.c~freezer-check-oom-kill-while-being-frozen mm/oom_kill.c
--- a/mm/oom_kill.c~freezer-check-oom-kill-while-being-frozen
+++ a/mm/oom_kill.c
@@ -266,8 +266,6 @@ enum oom_scan_t oom_scan_process_thread(
 	 * Don't allow any other task to have access to the reserves.
 	 */
 	if (test_tsk_thread_flag(task, TIF_MEMDIE)) {
-		if (unlikely(frozen(task)))
-			__thaw_task(task);
 		if (!force_kill)
 			return OOM_SCAN_ABORT;
 	}
_

Patches currently in -mm which might be from xiyou.wangcong@gmail.com are

origin.patch
freezer-check-oom-kill-while-being-frozen.patch
freezer-remove-obsolete-comments-in-__thaw_task.patch
oom-pm-oom-killed-task-cannot-escape-pm-suspend.patch
linux-next.patch