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

PATCH In review Oleg Nesterov 3 participants 18 messages Oct 16, 2014 – Oct 20, 2014 Freezer

Share & tools

Oleg Nesterov

author

2014-10-16 20:43 UTC #0 lore raw

Quoted 4 lines

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

2014-10-16 20:53 UTC #1 lore raw

Quoted 8 lines · Oleg Nesterov

Quoted 4 lines
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

author

2014-10-16 21:14 UTC #2 lore raw

Quoted 7 lines · Cong Wang

Quoted 5 lines · Oleg Nesterov
Quoted 4 lines
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.

Quoted 6 lines

Quoted 3 lines
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

2014-10-16 21:19 UTC #3 lore raw

Quoted 7 lines · Oleg Nesterov

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

Oleg Nesterov

author

2014-10-16 21:38 UTC #4 lore raw

Quoted 7 lines · Cong Wang

Quoted 5 lines · Oleg Nesterov
But also I can't understand why this patch helps. The changelog says:
do_send_sig_info will wake up the thread

Quoted 8 lines

Quoted 2 lines
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.

Cong Wang

2014-10-16 21:52 UTC #5 lore raw

Quoted 6 lines · 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()?

Quoted 4 lines

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

author

2014-10-16 22:25 UTC #6 lore raw

Quoted 5 lines · Cong Wang

Quoted 3 lines · Oleg Nesterov
If a task B is already frozen, it sleeps in D state.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 2 lines

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

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

Quoted 5 lines

Quoted 2 lines
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

2014-10-17 02:33 UTC #7 lore raw

Quoted 13 lines · Oleg Nesterov

Quoted 12 lines · Cong Wang
Quoted 6 lines · 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.

Quoted 8 lines

Quoted 1 line
So are you saying the loop in __refrigerator() is useless?
No.
Quoted 2 lines
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

2014-10-17 07:46 UTC #8 lore raw

PATCH 1/1 freezer: check OOM kill while being frozen

Quoted 23 lines · Cong Wang

Quoted 13 lines · Oleg Nesterov
Quoted 12 lines · Cong Wang
Quoted 6 lines · 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.
Quoted 8 lines
Quoted 1 line
So are you saying the loop in __refrigerator() is useless?
No.
Quoted 2 lines
Since
it will always stay in asleep after schedule()?
Not always. But it will stay asleep in this particular case.

Quoted 7 lines

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

Quoted 1 line

From 830324c8be8b499e1d18a73076cf4d126c4ac486 Mon Sep 17 00:00:00 2001

From: Cong Wang <[email protected]>
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

[[email protected]: rewrote the changelog]

Fixes a3201227f803 (freezer: make freezing() test freeze conditions in effect instead of TIF_FREEZE)

Cc [email protected] # 3.3+

Cc David Rientjes <[email protected]>

Cc Michal Hocko <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Andrew Morton <[email protected]>

Acked-by Michal Hocko <[email protected]>

Signed-off-by Cong Wang <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 kernel/freezer.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

kernel/freezer.c +17 -3

index aa6a8aadb911..77ad6794b610 100644
@@ -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);

Signature · 5 lines

2.1.1

-- 
Michal Hocko
SUSE Labs

Oleg Nesterov

author

2014-10-17 16:13 UTC #9 lore raw

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

Quoted 2 lines · Michal Hocko

I think we should rather get back to __thaw_task here.

Yes, agreed.

Quoted 1 line

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 ;)

kernel/freezer.c bool freezing_slow_path(struct task_struct *p) · line 48

Quoted 8 lines

kernel/freezer.c +1 -1

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

kernel/freezer.c bool freezing_slow_path(struct task_struct *p) · line 64

Quoted 10 lines

kernel/freezer.c +10 -0

+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

2014-10-17 16:20 UTC #10 lore raw

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

Quoted 9 lines · Oleg Nesterov

Quoted 2 lines · Michal Hocko
I think we should rather get back to __thaw_task here.
Yes, agreed.
Quoted 1 line
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.

Signature · 2 lines

Michal Hocko
SUSE Labs

Michal Hocko

Acked-by

2014-10-20 15:17 UTC #11 lore raw

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

In reply to Oleg Nesterov

Quoted 20 lines · Oleg Nesterov

Quoted 2 lines · Michal Hocko
I think we should rather get back to __thaw_task here.
Yes, agreed.
Quoted 1 line
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 ;)
Quoted 6 lines
kernel/freezer.c +0 -0
@@ -45,13 +45,28 @@ bool freezing_slow_path(struct task_struct *p)
 	if (pm_nosig_freezing || cgroup_freezing(p))
 		return true;
 

Quoted 5 lines

kernel/freezer.c bool freezing_slow_path(struct task_struct *p) · line 48
Quoted 2 lines
kernel/freezer.c +1 -1
-	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.

Quoted 5 lines

kernel/freezer.c +5 -0

+static bool should_thaw_current(bool check_kthr_stop)
+{
+	if (!freezing(current))
+		return true;
+

Quoted 8 lines

kernel/freezer.c bool freezing_slow_path(struct task_struct *p) · line 58
Quoted 5 lines
kernel/freezer.c +5 -0
+	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

Quoted 2 lines

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?

Quoted 2 lines

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.

Quoted 7 lines

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

Quoted 1 line

From 6e8b92e7133307e30afe35c6a0637cb58c0fc147 Mon Sep 17 00:00:00 2001

From: Cong Wang <[email protected]>
Date: Mon, 20 Oct 2014 17:16:01 +0200
Subject: [PATCH] freezer: check OOM kill while being frozen

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

[[email protected]: rewrote the changelog]

Fixes a3201227f803 (freezer: make freezing() test freeze conditions in effect instead of TIF_FREEZE)

Cc [email protected] # 3.3+

Cc David Rientjes <[email protected]>

Cc Michal Hocko <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Andrew Morton <[email protected]>

Acked-by Michal Hocko <[email protected]>

Signed-off-by Cong Wang <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 kernel/freezer.c | 3 +++
 1 file changed, 3 insertions(+)

kernel/freezer.c +3 -0

index aa6a8aadb911..8f9279b9c6d7 100644
@@ -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;

Signature · 5 lines

2.1.1

-- 
Michal Hocko
SUSE Labs

Oleg Nesterov

author Acked-by

2014-10-20 17:44 UTC #12 lore raw

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

Quoted 7 lines · Michal Hocko

Quoted 2 lines · 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.

Quoted 11 lines

Quoted 2 lines
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.

Quoted 9 lines

Quoted 7 lines
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.

Quoted 1 line

Does it make more sense to you now, Oleg?

Thanks!

Acked-by Oleg Nesterov <[email protected]>

Quoted 61 lines

From 6e8b92e7133307e30afe35c6a0637cb58c0fc147 Mon Sep 17 00:00:00 2001
From: Cong Wang <[email protected]>
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
[[email protected]: rewrote the changelog]
Fixes a3201227f803 (freezer: make freezing() test freeze conditions in effect instead of TIF_FREEZE)
Cc [email protected] # 3.3+
Cc David Rientjes <[email protected]>
Cc Michal Hocko <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Andrew Morton <[email protected]>
Acked-by Michal Hocko <[email protected]>
Signed-off-by Cong Wang <[email protected]>
Signed-off-by Michal Hocko <[email protected]>
---
 kernel/freezer.c | 3 +++
 1 file changed, 3 insertions(+)
kernel/freezer.c +3 -0
index aa6a8aadb911..8f9279b9c6d7 100644
@@ -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;
 
Signature · 5 lines
2.1.1

-- 
Michal Hocko
SUSE Labs

Oleg Nesterov

author

2014-10-17 15:27 UTC #13 lore raw

In reply to Cong Wang

Quoted 8 lines · Cong Wang

Quoted 5 lines · Oleg Nesterov
Quoted 2 lines · Cong Wang
it will always stay in asleep after schedule()?
Not always. But it will stay asleep in this particular case.

Quoted 7 lines

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.

Michal Hocko

2014-10-17 16:07 UTC #14 lore raw

Quoted 11 lines · Oleg Nesterov

Quoted 10 lines · Cong Wang
Quoted 5 lines · Oleg Nesterov
Quoted 2 lines · Cong Wang
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:

Quoted 8 lines

Quoted 5 lines
        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!

Signature · 2 lines

Michal Hocko
SUSE Labs

Michal Hocko

2014-10-17 06:59 UTC #15 lore raw

In reply to Oleg Nesterov

Quoted 7 lines · Oleg Nesterov

Quoted 4 lines
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!

Signature · 2 lines

Michal Hocko
SUSE Labs

Oleg Nesterov

author

2014-10-17 15:35 UTC #16 lore raw

Quoted 2 lines · 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

2014-10-17 16:06 UTC #17 lore raw

Quoted 6 lines · Oleg Nesterov

Quoted 2 lines · 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.

Quoted 5 lines

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

Signature · 2 lines

Michal Hocko
SUSE Labs