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

[Patch v2] freezer: check OOM kill 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 page fault
then trigger oom
B=oom_scan_process_thread()
                                        cgroup freezer freeze(A, B)
                        ...
                        try_to_freeze()
                        stay in D state
oom_kill_process(B)
restart page fault
...

In this case, process A triggered a page fault in user-space,
and the kernel page fault handler triggered OOM, then kernel
selected 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 not care about pending signals any more.

David Rientjes tried to fix same issue with commit
f660daac474c6f (oom: thaw threads if oom killed thread is
frozen before deferring) but it doesn't work any more, because
__thaw_task() just checks if it's frozen and then wakes it up,
but the frozen task, after waking up, will check if freezing()
is still true 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().

Fix this straightly by checking if the frozen process itself
has been killed by OOM killer, so that the frozen process will
recover itself and 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>
---
 kernel/freezer.c | 19 +++++++++++--------
 mm/oom_kill.c    |  2 --
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/kernel/freezer.c b/kernel/freezer.c
index aa6a8aa..1f90d70 100644
--- a/kernel/freezer.c
+++ b/kernel/freezer.c
@@ -52,6 +52,16 @@ bool freezing_slow_path(struct task_struct *p)
 }
 EXPORT_SYMBOL(freezing_slow_path);
 
+static bool i_should_thaw_myself(bool check_kthr_stop)
+{
+	if (!freezing(current) ||
+	    (check_kthr_stop && kthread_should_stop()) ||
+	    test_thread_flag(TIF_MEMDIE))
+		return true;
+	else
+		return false;
+}
+
 /* Refrigerator is place where frozen processes are stored :-). */
 bool __refrigerator(bool check_kthr_stop)
 {
@@ -67,8 +77,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 (i_should_thaw_myself(check_kthr_stop))
 			current->flags &= ~PF_FROZEN;
 		spin_unlock_irq(&freezer_lock);
 
@@ -147,12 +156,6 @@ void __thaw_task(struct task_struct *p)
 {
 	unsigned long flags;
 
-	/*
-	 * Clear freezing and kick @p if FROZEN.  Clearing is guaranteed to
-	 * be visible to @p as waking up implies wmb.  Waking up inside
-	 * freezer_lock also prevents wakeups from leaking outside
-	 * refrigerator.
-	 */
 	spin_lock_irqsave(&freezer_lock, flags);
 	if (frozen(p))
 		wake_up_process(p);
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 1e11df8..112c278 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -266,8 +266,6 @@ enum oom_scan_t oom_scan_process_thread(struct task_struct *task,
 	 * 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;
 	}
-- 
1.8.3.1

[Patch] x86,mm: check freeze request in page fault handler

[Cong Wang]

When a process triggers a page fault and kernel keeps
trying to retry the fault, there is no chance for this process
to be frozen, so the freeze request will always be pending.

This patch lets the page fault handler check pending
freeze request and freeze current process if so.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
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>
---
 arch/x86/mm/fault.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index a241946..ad9728a 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -14,6 +14,7 @@
 #include <linux/hugetlb.h>		/* hstate_index_to_shift	*/
 #include <linux/prefetch.h>		/* prefetchw			*/
 #include <linux/context_tracking.h>	/* exception_enter(), ...	*/
+#include <linux/freezer.h>		/* try_to_freeze()		*/
 
 #include <asm/traps.h>			/* dotraplinkage, ...		*/
 #include <asm/pgalloc.h>		/* pgd_*(), ...			*/
@@ -885,6 +886,9 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code,
 		up_read(&current->mm->mmap_sem);
 		no_context(regs, error_code, address, 0, 0);
 		return;
+	} else if (signal_pending(current) && (error_code & PF_USER)) {
+		if (try_to_freeze())
+			return;
 	}
 
 	if (fault & VM_FAULT_OOM) {
-- 
1.8.3.1

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

[Michal Hocko]

On Mon 11-08-14 17:53:54, 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 page fault
> then trigger oom
> B=oom_scan_process_thread()
>                                         cgroup freezer freeze(A, B)
>                         ...
>                         try_to_freeze()
>                         stay in D state
> oom_kill_process(B)
> restart page fault
> ...
> 
> In this case, process A triggered a page fault in user-space,
> and the kernel page fault handler triggered OOM, then kernel
> selected 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 not care about pending signals any more.
> 
> David Rientjes tried to fix same issue with commit
> f660daac474c6f (oom: thaw threads if oom killed thread is
> frozen before deferring) but it doesn't work any more, because

Has it stopped working as a result of a3201227f803 (freezer: make
freezing() test freeze conditions in effect instead of TIF_FREEZE)?
It removes clear_freeze_flag from __thaw_task and so the OOM victim
cannot escape from the refrigerator. But there were a lot of changes in
that area at the time so I might be missing some subtle details.

> __thaw_task() just checks if it's frozen and then wakes it up,
> but the frozen task, after waking up, will check if freezing()
> is still true 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().
> 
> Fix this straightly by checking if the frozen process itself
> has been killed by OOM killer, so that the frozen process will
> recover itself and be killed finally.

OK, this should work. I remember Tejun mentioned that he wanted frozen
tasks to be killable but I have no idea where this went. Maybe we want
to go with the OOM part now as it fixes a real bug.

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

I think this should go to stable as well.

Acked-by: Michal Hocko <mhocko@suse.cz>

Two minor notes below:

> ---
>  kernel/freezer.c | 19 +++++++++++--------
>  mm/oom_kill.c    |  2 --
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/freezer.c b/kernel/freezer.c
> index aa6a8aa..1f90d70 100644
> --- a/kernel/freezer.c
> +++ b/kernel/freezer.c
> @@ -52,6 +52,16 @@ bool freezing_slow_path(struct task_struct *p)
>  }
>  EXPORT_SYMBOL(freezing_slow_path);
>  
> +static bool i_should_thaw_myself(bool check_kthr_stop)

should_thaw_current?

> +{
> +	if (!freezing(current) ||
> +	    (check_kthr_stop && kthread_should_stop()) ||
> +	    test_thread_flag(TIF_MEMDIE))
> +		return true;
> +	else
> +		return false;
> +}
> +
>  /* Refrigerator is place where frozen processes are stored :-). */
>  bool __refrigerator(bool check_kthr_stop)
>  {
> @@ -67,8 +77,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 (i_should_thaw_myself(check_kthr_stop))
>  			current->flags &= ~PF_FROZEN;
>  		spin_unlock_irq(&freezer_lock);
>  
> @@ -147,12 +156,6 @@ void __thaw_task(struct task_struct *p)
>  {
>  	unsigned long flags;
>  
> -	/*
> -	 * Clear freezing and kick @p if FROZEN.  Clearing is guaranteed to
> -	 * be visible to @p as waking up implies wmb.  Waking up inside
> -	 * freezer_lock also prevents wakeups from leaking outside
> -	 * refrigerator.
> -	 */

This is an unrelated change.

>  	spin_lock_irqsave(&freezer_lock, flags);
>  	if (frozen(p))
>  		wake_up_process(p);
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 1e11df8..112c278 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -266,8 +266,6 @@ enum oom_scan_t oom_scan_process_thread(struct task_struct *task,
>  	 * 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;
>  	}
> -- 
> 1.8.3.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [Patch] x86,mm: check freeze request in page fault handler

[Michal Hocko]

On Mon 11-08-14 17:53:55, Cong Wang wrote:
> When a process triggers a page fault and kernel keeps
> trying to retry the fault, there is no chance for this process
> to be frozen, so the freeze request will always be pending.

The retry cannot happen indefinitely, no?

Besides that the patch is broken in at least 2 ways. You are not
releasing mmap_sem and this will break memcg OOM killer handling.

If a memcg is under OOM (because of hard limit) then try_charge
calls mem_cgroup_oom which marks the current task with OOM
information. Notably takes a reference to memcg->css. The charge fail
will then gets up the pagefault stack until we get to mm_fault_error
where you put the task into freezer and then returns without
pagefault_out_of_memory which would handle memcg specific parts in
mem_cgroup_oom_synchronize. If the task wakes up and the page fault
retry succeeds (because some charges were released in the meantime) then
you leak a reference to memcg->css.

Besides that the whole change would need a better justification. Why
other archs do not need this?

> This patch lets the page fault handler check pending
> freeze request and freeze current process if so.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> 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>
> ---
>  arch/x86/mm/fault.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index a241946..ad9728a 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -14,6 +14,7 @@
>  #include <linux/hugetlb.h>		/* hstate_index_to_shift	*/
>  #include <linux/prefetch.h>		/* prefetchw			*/
>  #include <linux/context_tracking.h>	/* exception_enter(), ...	*/
> +#include <linux/freezer.h>		/* try_to_freeze()		*/
>  
>  #include <asm/traps.h>			/* dotraplinkage, ...		*/
>  #include <asm/pgalloc.h>		/* pgd_*(), ...			*/
> @@ -885,6 +886,9 @@ mm_fault_error(struct pt_regs *regs, unsigned long error_code,
>  		up_read(&current->mm->mmap_sem);
>  		no_context(regs, error_code, address, 0, 0);
>  		return;
> +	} else if (signal_pending(current) && (error_code & PF_USER)) {
> +		if (try_to_freeze())
> +			return;
>  	}
>  
>  	if (fault & VM_FAULT_OOM) {
> -- 
> 1.8.3.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [Patch] x86,mm: check freeze request in page fault handler

[Cong Wang]

On Tue, Aug 12, 2014 at 5:09 AM, Michal Hocko <mhocko@suse.cz> wrote:
> On Mon 11-08-14 17:53:55, Cong Wang wrote:
>> When a process triggers a page fault and kernel keeps
>> trying to retry the fault, there is no chance for this process
>> to be frozen, so the freeze request will always be pending.
>
> The retry cannot happen indefinitely, no?
>
> Besides that the patch is broken in at least 2 ways. You are not
> releasing mmap_sem and this will break memcg OOM killer handling.

Ah, it's my bad to miss the up_read().

>
> If a memcg is under OOM (because of hard limit) then try_charge
> calls mem_cgroup_oom which marks the current task with OOM
> information. Notably takes a reference to memcg->css. The charge fail
> will then gets up the pagefault stack until we get to mm_fault_error
> where you put the task into freezer and then returns without
> pagefault_out_of_memory which would handle memcg specific parts in
> mem_cgroup_oom_synchronize. If the task wakes up and the page fault
> retry succeeds (because some charges were released in the meantime) then
> you leak a reference to memcg->css.

OK, I thought skipping pagefault_out_of_memory() is safe, you are right here,
thanks for explanation.

>
> Besides that the whole change would need a better justification. Why
> other archs do not need this?
>

They probably need this as well, just that I only have x86 to care and test. :)

Does the following updated patch make any sense to you? If not, I will just
drop it.

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index a241946..8c0a7d8 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -14,6 +14,7 @@
 #include <linux/hugetlb.h>             /* hstate_index_to_shift        */
 #include <linux/prefetch.h>            /* prefetchw                    */
 #include <linux/context_tracking.h>    /* exception_enter(), ...       */
+#include <linux/freezer.h>             /* try_to_freeze()              */

 #include <asm/traps.h>                 /* dotraplinkage, ...           */
 #include <asm/pgalloc.h>               /* pgd_*(), ...                 */
@@ -904,6 +905,9 @@ mm_fault_error(struct pt_regs *regs, unsigned long
error_code,
                 * oom-killed):
                 */
                pagefault_out_of_memory();
+
+               if (signal_pending(current))
+                       try_to_freeze();
        } else {
                if (fault & (VM_FAULT_SIGBUS|VM_FAULT_HWPOISON|
                             VM_FAULT_HWPOISON_LARGE))

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

[Cong Wang]

On Tue, Aug 12, 2014 at 4:44 AM, Michal Hocko <mhocko@suse.cz> wrote:
> Has it stopped working as a result of a3201227f803 (freezer: make
> freezing() test freeze conditions in effect instead of TIF_FREEZE)?
> It removes clear_freeze_flag from __thaw_task and so the OOM victim
> cannot escape from the refrigerator. But there were a lot of changes in
> that area at the time so I might be missing some subtle details.

Probably, I don't have time to dig the history.

>
>> __thaw_task() just checks if it's frozen and then wakes it up,
>> but the frozen task, after waking up, will check if freezing()
>> is still true 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().
>>
>> Fix this straightly by checking if the frozen process itself
>> has been killed by OOM killer, so that the frozen process will
>> recover itself and be killed finally.
>
> OK, this should work. I remember Tejun mentioned that he wanted frozen
> tasks to be killable but I have no idea where this went. Maybe we want
> to go with the OOM part now as it fixes a real bug.


I think he rejected the patch from Bart which makes frozen tasks killable.

>> +static bool i_should_thaw_myself(bool check_kthr_stop)
>
> should_thaw_current?
>

I have no strong preference, whatever you prefer...

>> @@ -147,12 +156,6 @@ void __thaw_task(struct task_struct *p)
>>  {
>>       unsigned long flags;
>>
>> -     /*
>> -      * Clear freezing and kick @p if FROZEN.  Clearing is guaranteed to
>> -      * be visible to @p as waking up implies wmb.  Waking up inside
>> -      * freezer_lock also prevents wakeups from leaking outside
>> -      * refrigerator.
>> -      */
>
> This is an unrelated change.
>

It is, because as I explained, __thaw_task() doesn't do what this comment
says therefore needs this patch. Or you prefer to make it a separated patch?

Thanks.

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

[Michal Hocko]

On Tue 12-08-14 17:53:33, Cong Wang wrote:
> On Tue, Aug 12, 2014 at 4:44 AM, Michal Hocko <mhocko@suse.cz> wrote:
[...]
> >> @@ -147,12 +156,6 @@ void __thaw_task(struct task_struct *p)
> >>  {
> >>       unsigned long flags;
> >>
> >> -     /*
> >> -      * Clear freezing and kick @p if FROZEN.  Clearing is guaranteed to
> >> -      * be visible to @p as waking up implies wmb.  Waking up inside
> >> -      * freezer_lock also prevents wakeups from leaking outside
> >> -      * refrigerator.
> >> -      */
> >
> > This is an unrelated change.
> >
> 
> It is, because as I explained, __thaw_task() doesn't do what this comment
> says therefore needs this patch. Or you prefer to make it a separated patch?

It is outdated since a3201227f803. I would do it in a separate patch but
maybe just mention that the above commit has changed the implementation.

-- 
Michal Hocko
SUSE Labs

Re: [Patch] x86,mm: check freeze request in page fault handler

[Michal Hocko]

On Tue 12-08-14 17:47:02, Cong Wang wrote:
[...]
> Does the following updated patch make any sense to you? If not, I will just
> drop it.

Not really to be honest. I do not see what problem you are trying to fix.

> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index a241946..8c0a7d8 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -14,6 +14,7 @@
>  #include <linux/hugetlb.h>             /* hstate_index_to_shift        */
>  #include <linux/prefetch.h>            /* prefetchw                    */
>  #include <linux/context_tracking.h>    /* exception_enter(), ...       */
> +#include <linux/freezer.h>             /* try_to_freeze()              */
> 
>  #include <asm/traps.h>                 /* dotraplinkage, ...           */
>  #include <asm/pgalloc.h>               /* pgd_*(), ...                 */
> @@ -904,6 +905,9 @@ mm_fault_error(struct pt_regs *regs, unsigned long
> error_code,
>                  * oom-killed):
>                  */
>                 pagefault_out_of_memory();
> +
> +               if (signal_pending(current))
> +                       try_to_freeze();
>         } else {
>                 if (fault & (VM_FAULT_SIGBUS|VM_FAULT_HWPOISON|
>                              VM_FAULT_HWPOISON_LARGE))

-- 
Michal Hocko
SUSE Labs

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

[Cong Wang]

On Wed, Aug 13, 2014 at 2:26 AM, Michal Hocko <mhocko@suse.cz> wrote:
> On Tue 12-08-14 17:53:33, Cong Wang wrote:
>> On Tue, Aug 12, 2014 at 4:44 AM, Michal Hocko <mhocko@suse.cz> wrote:
> [...]
>> >> @@ -147,12 +156,6 @@ void __thaw_task(struct task_struct *p)
>> >>  {
>> >>       unsigned long flags;
>> >>
>> >> -     /*
>> >> -      * Clear freezing and kick @p if FROZEN.  Clearing is guaranteed to
>> >> -      * be visible to @p as waking up implies wmb.  Waking up inside
>> >> -      * freezer_lock also prevents wakeups from leaking outside
>> >> -      * refrigerator.
>> >> -      */
>> >
>> > This is an unrelated change.
>> >
>>
>> It is, because as I explained, __thaw_task() doesn't do what this comment
>> says therefore needs this patch. Or you prefer to make it a separated patch?
>
> It is outdated since a3201227f803. I would do it in a separate patch but
> maybe just mention that the above commit has changed the implementation.
>

OK, I will split this patch.

Re: [Patch] x86,mm: check freeze request in page fault handler

[Cong Wang]

On Wed, Aug 13, 2014 at 2:36 AM, Michal Hocko <mhocko@suse.cz> wrote:
> On Tue 12-08-14 17:47:02, Cong Wang wrote:
> [...]
>> Does the following updated patch make any sense to you? If not, I will just
>> drop it.
>
> Not really to be honest. I do not see what problem you are trying to fix.
>

In my case, a page faulted process triggered OOM, kernel kept
retrying this fault since without my fix oom killer couldn't kill a
frozen process.
This led to that this process is always running even after we tried to
freeze it,
the whole memory cgroup just stayed in FREEZING state.

Although with my fix, OOM will kill the frozen process so that the page faulted
process could probably stop retrying the fault finally, the question
is that is OOM
the only case blocks page fault? Looking at the code, there could be other case
blocking page fault handler as well, so instead of keep retrying for
unknown times
with ignoring freeze request, why not just freeze it since this is not
dangerous?

Or I am missing anything?

Re: [Patch] x86,mm: check freeze request in page fault handler

[Michal Hocko]

On Wed 13-08-14 17:26:48, Cong Wang wrote:
> On Wed, Aug 13, 2014 at 2:36 AM, Michal Hocko <mhocko@suse.cz> wrote:
> > On Tue 12-08-14 17:47:02, Cong Wang wrote:
> > [...]
> >> Does the following updated patch make any sense to you? If not, I will just
> >> drop it.
> >
> > Not really to be honest. I do not see what problem you are trying to fix.
> >
> 
> In my case, a page faulted process triggered OOM, kernel kept retrying
> this fault since without my fix oom killer couldn't kill a frozen
> process.
> This led to that this process is always running even after we tried to
> freeze it, the whole memory cgroup just stayed in FREEZING state.
> 
> Although with my fix, OOM will kill the frozen process so that the
> page faulted process could probably stop retrying the fault finally,
> the question is that is OOM the only case blocks page fault? Looking
> at the code, there could be other case blocking page fault handler
> as well, so instead of keep retrying for unknown times with ignoring
> freeze request, why not just freeze it since this is not dangerous?

I do not think that spreading freezing points is a good idea. The task
can be blocked during page fault for many other reasons so this point
doesn't solve anything IMO.

> Or I am missing anything?

Page fault shouldn't retry too many times without returning to
userspace.
-- 
Michal Hocko
SUSE Labs