[Patch] freezer: check OOM kill signal while being frozen

[Patch] freezer: check OOM kill signal while being frozen

[Cong Wang]

There is a race condition between OOM killer and freezer when
they try to operate on the same process, something like below:

	Process A	Process B		Process C
trigger oom
B=oom_scan_process_thread()
					cgroup_freezer_freeze(B)
			...
			try_to_freeze()
			stay in D state
oom_kill_process(B)

In this case, process A triggers OOM and kernel selects process B
as the victim, right before being killed process B was frozen by
process C therefore went to D state, then kernel sent SIGKILL but
it is already too late as process B will never care about pending
signals any more. Fix this straightly by checking fatal pending signal
from OOM killer, so that the frozen process will recover itself
and then be killed finally.

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>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
diff --git a/kernel/freezer.c b/kernel/freezer.c
index aa6a8aa..c6d189d 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -68,7 +68,9 @@ bool __refrigerator(bool check_kthr_stop)
 		spin_lock_irq(&freezer_lock);
 		current->flags |= PF_FROZEN;
 		if (!freezing(current) ||
-		    (check_kthr_stop && kthread_should_stop()))
+		    (check_kthr_stop && kthread_should_stop()) ||
+		    (test_tsk_thread_flag(current, TIF_MEMDIE) &&
+			 fatal_signal_pending(current)))
 			current->flags &= ~PF_FROZEN;
 		spin_unlock_irq(&freezer_lock);
 

Re: [Patch] freezer: check OOM kill signal while being frozen

[Kyungmin Park]

On Sat, Aug 9, 2014 at 9:46 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> There is a race condition between OOM killer and freezer when
> they try to operate on the same process, something like below:
>
>         Process A       Process B               Process C
> trigger oom
> B=oom_scan_process_thread()
>                                         cgroup_freezer_freeze(B)
>                         ...
>                         try_to_freeze()
>                         stay in D state
> oom_kill_process(B)
>
> In this case, process A triggers OOM and kernel selects process B
> as the victim, right before being killed process B was frozen by
> process C therefore went to D state, then kernel sent SIGKILL but
> it is already too late as process B will never care about pending
> signals any more. Fix this straightly by checking fatal pending signal
> from OOM killer, so that the frozen process will recover itself
> and then be killed finally.

Similar patch is posted and discussed but no conclusion.
http://marc.info/?t=137699769400004&r=1&w=1

Adding Bart,

Thank you,
Kyungmin Park
>
> 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>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index aa6a8aa..c6d189d 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -68,7 +68,9 @@ bool __refrigerator(bool check_kthr_stop)
>                 spin_lock_irq(&freezer_lock);
>                 current->flags |= PF_FROZEN;
>                 if (!freezing(current) ||
> -                   (check_kthr_stop && kthread_should_stop()))
> +                   (check_kthr_stop && kthread_should_stop()) ||
> +                   (test_tsk_thread_flag(current, TIF_MEMDIE) &&
> +                        fatal_signal_pending(current)))
>                         current->flags &= ~PF_FROZEN;
>                 spin_unlock_irq(&freezer_lock);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [Patch] freezer: check OOM kill signal while being frozen

[Cong Wang]

On Fri, Aug 8, 2014 at 7:00 PM, Kyungmin Park <kmpark@infradead.org> wrote:
>
> Similar patch is posted and discussed but no conclusion.
> http://marc.info/?t=137699769400004&r=1&w=1
>

I noticed this thread before sending my patch. I don't think
we are trying to solve the same problem:

a) I only care OOM kill, not other SIGKILL cases
b) I don't set the frozen process state to TASK_KILLABLE
   at all

I think Bartlomiej was trying to make frozen processes
killable by user-space SIGKILL, which is beyond my interest.

Thanks.

Re: [Patch] freezer: check OOM kill signal while being frozen

[David Rientjes]

On Fri, 8 Aug 2014, Cong Wang wrote:

> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index aa6a8aa..c6d189d 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -68,7 +68,9 @@ bool __refrigerator(bool check_kthr_stop)
>  		spin_lock_irq(&freezer_lock);
>  		current->flags |= PF_FROZEN;
>  		if (!freezing(current) ||
> -		    (check_kthr_stop && kthread_should_stop()))
> +		    (check_kthr_stop && kthread_should_stop()) ||
> +		    (test_tsk_thread_flag(current, TIF_MEMDIE) &&
> +			 fatal_signal_pending(current)))
>  			current->flags &= ~PF_FROZEN;
>  		spin_unlock_irq(&freezer_lock);
>  

All threads with TIF_MEMDIE set would have been killed, why is the check 
for fatal_signal_pending(current) needed?

You can also use test_thread_flag(TIF_MEMDIE) instead since you're only 
concerned with current.

Furthermore, that conditional is getting difficult to read.  Maybe it 
would help to have a helper function that returns bool that determines 
whether PF_FROZEN should be cleared?

Re: [Patch] freezer: check OOM kill signal while being frozen

[Cong Wang]

On Sat, Aug 9, 2014 at 1:55 AM, David Rientjes <rientjes@google.com> wrote:
> On Fri, 8 Aug 2014, Cong Wang wrote:
>
>> diff --git a/kernel/freezer.c b/kernel/freezer.c
>> index aa6a8aa..c6d189d 100644
>> --- a/kernel/freezer.c
>> +++ b/kernel/freezer.c
>> @@ -68,7 +68,9 @@ bool __refrigerator(bool check_kthr_stop)
>>               spin_lock_irq(&freezer_lock);
>>               current->flags |= PF_FROZEN;
>>               if (!freezing(current) ||
>> -                 (check_kthr_stop && kthread_should_stop()))
>> +                 (check_kthr_stop && kthread_should_stop()) ||
>> +                 (test_tsk_thread_flag(current, TIF_MEMDIE) &&
>> +                      fatal_signal_pending(current)))
>>                       current->flags &= ~PF_FROZEN;
>>               spin_unlock_irq(&freezer_lock);
>>
>
> All threads with TIF_MEMDIE set would have been killed, why is the check
> for fatal_signal_pending(current) needed?

So just checking TIF_MEMDIE is enough, right?

>
> You can also use test_thread_flag(TIF_MEMDIE) instead since you're only
> concerned with current.
>

OK, will do.

> Furthermore, that conditional is getting difficult to read.  Maybe it
> would help to have a helper function that returns bool that determines
> whether PF_FROZEN should be cleared?

Makes sense.

Thanks.

Re: [Patch] freezer: check OOM kill signal while being frozen

[Michal Hocko]

On Fri 08-08-14 17:46:38, Cong Wang wrote:
> There is a race condition between OOM killer and freezer when
> they try to operate on the same process, something like below:
> 
> 	Process A	Process B		Process C
> trigger oom
> B=oom_scan_process_thread()
> 					cgroup_freezer_freeze(B)
> 			...
> 			try_to_freeze()
> 			stay in D state
> oom_kill_process(B)
> 
> In this case, process A triggers OOM and kernel selects process B
> as the victim, right before being killed process B was frozen by
> process C therefore went to D state, then kernel sent SIGKILL but
> it is already too late as process B will never care about pending
> signals any more.

OK, so the system/memcg is still OOM and a new allocation/charge
would trigger killer again, right? Then oom_scan_process_thread sees
TIF_MEMDIE frozen task and thaw it so it can go away and die. So this
shouldn't be a permanent state. Or am I missing something?

> Fix this straightly by checking fatal pending signal
> from OOM killer, so that the frozen process will recover itself
> and then be killed finally.
> 
> 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>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index aa6a8aa..c6d189d 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -68,7 +68,9 @@ bool __refrigerator(bool check_kthr_stop)
>  		spin_lock_irq(&freezer_lock);
>  		current->flags |= PF_FROZEN;
>  		if (!freezing(current) ||
> -		    (check_kthr_stop && kthread_should_stop()))
> +		    (check_kthr_stop && kthread_should_stop()) ||
> +		    (test_tsk_thread_flag(current, TIF_MEMDIE) &&
> +			 fatal_signal_pending(current)))
>  			current->flags &= ~PF_FROZEN;
>  		spin_unlock_irq(&freezer_lock);
>  

-- 
Michal Hocko
SUSE Labs

Re: [Patch] freezer: check OOM kill signal while being frozen

[Cong Wang]

On Mon, Aug 11, 2014 at 6:18 AM, Michal Hocko <mhocko@suse.cz> wrote:
>
> OK, so the system/memcg is still OOM and a new allocation/charge
> would trigger killer again, right? Then oom_scan_process_thread sees
> TIF_MEMDIE frozen task and thaw it so it can go away and die. So this
> shouldn't be a permanent state. Or am I missing something?
>

Good point!

Reading the code again, I think David's commit (f660daac) doesn't
work any more, __thaw_task() just checks if it's frozen and then wakes
it up, but the frozen task, after waking up, will check if it's freezing() and
continue to freeze itself if so. __thaw_task() can't make freezing() return
false since it doesn't change any of these conditions, especially
cgroup_freezing().

This reminds me I should revert that code in oom_killer, checking
TIF_MEMDIE in __refrigerator() is more correct and clear.

I will update my patch.

Thanks!