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

[Michal Hocko <mhocko@suse.cz>]

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