+ 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.

[Cong Wang]

Post by 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?

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. :)

[Oleg Nesterov]

Post by Cong Wang

Post by 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?

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.

[Cong Wang]

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

:)

[Oleg Nesterov]

Post by Cong Wang

do_send_sig_info will wake up the thread
why?

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.

[Cong Wang]

Post by Oleg Nesterov
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()?

Post by Oleg Nesterov
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.

[Oleg Nesterov]

Post by Cong Wang

Post by Oleg Nesterov
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.

Post by Cong Wang
Since
it will always stay in asleep after schedule()?

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

Post by Cong Wang

Post by Oleg Nesterov
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.

[Cong Wang]

Post by Oleg Nesterov

Post by Cong Wang

Post by Oleg Nesterov
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.

Post by Cong Wang
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.

[Michal Hocko]

Post by Cong Wang

Post by Oleg Nesterov

Post by Cong Wang

Post by Oleg Nesterov
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.

Post by Cong Wang
Since
it will always stay in asleep after schedule()?

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

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 <***@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

[***@suse.cz: rewrote the changelog]
Fixes: a3201227f803 (freezer: make freezing() test freeze conditions in effect instead of TIF_FREEZE)
Cc: ***@vger.kernel.org # 3.3+
Cc: David Rientjes <***@google.com>
Cc: Michal Hocko <***@suse.cz>
Cc: "Rafael J. Wysocki" <***@rjwysocki.net>
Cc: Tejun Heo <***@kernel.org>
Cc: Andrew Morton <***@linux-foundation.org>
Acked-by: Michal Hocko <***@suse.cz>
Signed-off-by: Cong Wang <***@gmail.com>
Signed-off-by: Michal Hocko <***@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);

[Oleg Nesterov]

Post by Michal Hocko
I think we should rather get back to __thaw_task here.

Yes, agreed.

Post by Michal Hocko
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 ;)

Post by Michal Hocko
--- 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?

Post by Michal Hocko
+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.

[Michal Hocko]

Post by Oleg Nesterov

Post by Michal Hocko
I think we should rather get back to __thaw_task here.

Yes, agreed.

Post by Michal Hocko
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]

Post by Oleg Nesterov

Post by Michal Hocko
I think we should rather get back to __thaw_task here.

Yes, agreed.

Post by Michal Hocko
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 ;)

Post by Michal Hocko
--- 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.

Post by Oleg Nesterov

Post by Michal Hocko
+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

Post by Oleg Nesterov
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?

Post by Oleg Nesterov
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.

Post by Oleg Nesterov
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 <***@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

[***@suse.cz: rewrote the changelog]
Fixes: a3201227f803 (freezer: make freezing() test freeze conditions in effect instead of TIF_FREEZE)
Cc: ***@vger.kernel.org # 3.3+
Cc: David Rientjes <***@google.com>
Cc: Michal Hocko <***@suse.cz>
Cc: "Rafael J. Wysocki" <***@rjwysocki.net>
Cc: Tejun Heo <***@kernel.org>
Cc: Andrew Morton <***@linux-foundation.org>
Acked-by: Michal Hocko <***@suse.cz>
Signed-off-by: Cong Wang <***@gmail.com>
Signed-off-by: Michal Hocko <***@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;

[Oleg Nesterov]

Post by Michal Hocko

Post by Oleg Nesterov
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.

Post by Michal Hocko

Post by Oleg Nesterov
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.

Post by Michal Hocko

Post by Oleg Nesterov
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.

Post by Michal Hocko
Does it make more sense to you now, Oleg?

Thanks!

Post by Michal Hocko
From 6e8b92e7133307e30afe35c6a0637cb58c0fc147 Mon Sep 17 00:00:00 2001
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
Fixes: a3201227f803 (freezer: make freezing() test freeze conditions in effect instead of TIF_FREEZE)
---
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

[Oleg Nesterov]

Post by Cong Wang

Post by Oleg Nesterov

Post by Cong Wang
it will always stay in asleep after schedule()?

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

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.

[Michal Hocko]

Post by Oleg Nesterov

Post by Cong Wang

Post by Oleg Nesterov

Post by Cong Wang
it will always stay in asleep after schedule()?

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

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]

Post by 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?

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!

[Oleg Nesterov]

Post by Michal Hocko
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.

[Michal Hocko]

Post by Oleg Nesterov

Post by Michal Hocko
Can we simply make the task sleep interruptible?

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

good point.

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