[PATCH] oom_reaper: close race without using oom_lock

[PATCH] oom_reaper: close race without using oom_lock

[Tetsuo Handa]

Commit e2fe14564d3316d1 ("oom_reaper: close race with exiting task")
guarded whole OOM reaping operations using oom_lock. But there was no
need to guard whole operations. We needed to guard only setting of
MMF_OOM_REAPED flag because get_page_from_freelist() in
__alloc_pages_may_oom() is called with oom_lock held.

If we change to guard only setting of MMF_OOM_SKIP flag, the OOM reaper
can start reaping operations as soon as wake_oom_reaper() is called.
But since setting of MMF_OOM_SKIP flag at __mmput() is not guarded with
oom_lock, guarding only the OOM reaper side is not sufficient.

If we change the OOM killer side to ignore MMF_OOM_SKIP flag once,
there is no need to guard setting of MMF_OOM_SKIP flag, and we can
guarantee a chance to call get_page_from_freelist() in
__alloc_pages_may_oom() without depending on oom_lock serialization.

This patch makes MMF_OOM_SKIP act as if MMF_OOM_REAPED, and adds a new
flag which acts as if MMF_OOM_SKIP, in order to close both race window
(the OOM reaper side and __mmput() side) without using oom_lock.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 include/linux/mm_types.h |  1 +
 mm/oom_kill.c            | 42 +++++++++++++++---------------------------
 2 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index ff15181..3184b7a 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -495,6 +495,7 @@ struct mm_struct {
 	 */
 	bool tlb_flush_pending;
 #endif
+	bool oom_killer_synchronized;
 	struct uprobes_state uprobes_state;
 #ifdef CONFIG_HUGETLB_PAGE
 	atomic_long_t hugetlb_usage;
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 9e8b4f0..1710133 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -300,11 +300,17 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
 	 * This task already has access to memory reserves and is being killed.
 	 * Don't allow any other task to have access to the reserves unless
 	 * the task has MMF_OOM_SKIP because chances that it would release
-	 * any memory is quite low.
+	 * any memory is quite low. But ignore MMF_OOM_SKIP once, for there is
+	 * still possibility that get_page_from_freelist() with oom_lock held
+	 * succeeds because MMF_OOM_SKIP is set without oom_lock held.
 	 */
 	if (!is_sysrq_oom(oc) && tsk_is_oom_victim(task)) {
-		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags))
+		struct mm_struct *mm = task->signal->oom_mm;
+
+		if (mm->oom_killer_synchronized)
 			goto next;
+		if (test_bit(MMF_OOM_SKIP, &mm->flags))
+			mm->oom_killer_synchronized = true;
 		goto abort;
 	}
 
@@ -470,28 +476,10 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 {
 	struct mmu_gather tlb;
 	struct vm_area_struct *vma;
-	bool ret = true;
-
-	/*
-	 * We have to make sure to not race with the victim exit path
-	 * and cause premature new oom victim selection:
-	 * __oom_reap_task_mm		exit_mm
-	 *   mmget_not_zero
-	 *				  mmput
-	 *				    atomic_dec_and_test
-	 *				  exit_oom_victim
-	 *				[...]
-	 *				out_of_memory
-	 *				  select_bad_process
-	 *				    # no TIF_MEMDIE task selects new victim
-	 *  unmap_page_range # frees some memory
-	 */
-	mutex_lock(&oom_lock);
 
 	if (!down_read_trylock(&mm->mmap_sem)) {
-		ret = false;
 		trace_skip_task_reaping(tsk->pid);
-		goto unlock_oom;
+		return false;
 	}
 
 	/*
@@ -502,7 +490,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 	if (!mmget_not_zero(mm)) {
 		up_read(&mm->mmap_sem);
 		trace_skip_task_reaping(tsk->pid);
-		goto unlock_oom;
+		return true;
 	}
 
 	trace_start_task_reaping(tsk->pid);
@@ -549,9 +537,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 	 */
 	mmput_async(mm);
 	trace_finish_task_reaping(tsk->pid);
-unlock_oom:
-	mutex_unlock(&oom_lock);
-	return ret;
+	return true;
 }
 
 #define MAX_OOM_REAP_RETRIES 10
@@ -661,8 +647,10 @@ static void mark_oom_victim(struct task_struct *tsk)
 		return;
 
 	/* oom_mm is bound to the signal struct life time. */
-	if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm))
-		mmgrab(tsk->signal->oom_mm);
+	if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm)) {
+		mmgrab(mm);
+		mm->oom_killer_synchronized = false;
+	}
 
 	/*
 	 * Make sure that the task is woken up from uninterruptible sleep
-- 
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Michal Hocko]

On Tue 18-07-17 23:06:50, Tetsuo Handa wrote:
> Commit e2fe14564d3316d1 ("oom_reaper: close race with exiting task")
> guarded whole OOM reaping operations using oom_lock. But there was no
> need to guard whole operations. We needed to guard only setting of
> MMF_OOM_REAPED flag because get_page_from_freelist() in
> __alloc_pages_may_oom() is called with oom_lock held.
> 
> If we change to guard only setting of MMF_OOM_SKIP flag, the OOM reaper
> can start reaping operations as soon as wake_oom_reaper() is called.
> But since setting of MMF_OOM_SKIP flag at __mmput() is not guarded with
> oom_lock, guarding only the OOM reaper side is not sufficient.
> 
> If we change the OOM killer side to ignore MMF_OOM_SKIP flag once,
> there is no need to guard setting of MMF_OOM_SKIP flag, and we can
> guarantee a chance to call get_page_from_freelist() in
> __alloc_pages_may_oom() without depending on oom_lock serialization.
> 
> This patch makes MMF_OOM_SKIP act as if MMF_OOM_REAPED, and adds a new
> flag which acts as if MMF_OOM_SKIP, in order to close both race window
> (the OOM reaper side and __mmput() side) without using oom_lock.

Why do we need this patch when
http://lkml.kernel.org/r/20170626130346.26314-1-mhocko@kernel.org
already removes the lock and solves another problem at once?

> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  include/linux/mm_types.h |  1 +
>  mm/oom_kill.c            | 42 +++++++++++++++---------------------------
>  2 files changed, 16 insertions(+), 27 deletions(-)
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index ff15181..3184b7a 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -495,6 +495,7 @@ struct mm_struct {
>  	 */
>  	bool tlb_flush_pending;
>  #endif
> +	bool oom_killer_synchronized;
>  	struct uprobes_state uprobes_state;
>  #ifdef CONFIG_HUGETLB_PAGE
>  	atomic_long_t hugetlb_usage;
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 9e8b4f0..1710133 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -300,11 +300,17 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
>  	 * This task already has access to memory reserves and is being killed.
>  	 * Don't allow any other task to have access to the reserves unless
>  	 * the task has MMF_OOM_SKIP because chances that it would release
> -	 * any memory is quite low.
> +	 * any memory is quite low. But ignore MMF_OOM_SKIP once, for there is
> +	 * still possibility that get_page_from_freelist() with oom_lock held
> +	 * succeeds because MMF_OOM_SKIP is set without oom_lock held.
>  	 */
>  	if (!is_sysrq_oom(oc) && tsk_is_oom_victim(task)) {
> -		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags))
> +		struct mm_struct *mm = task->signal->oom_mm;
> +
> +		if (mm->oom_killer_synchronized)
>  			goto next;
> +		if (test_bit(MMF_OOM_SKIP, &mm->flags))
> +			mm->oom_killer_synchronized = true;
>  		goto abort;
>  	}
>  
> @@ -470,28 +476,10 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
>  {
>  	struct mmu_gather tlb;
>  	struct vm_area_struct *vma;
> -	bool ret = true;
> -
> -	/*
> -	 * We have to make sure to not race with the victim exit path
> -	 * and cause premature new oom victim selection:
> -	 * __oom_reap_task_mm		exit_mm
> -	 *   mmget_not_zero
> -	 *				  mmput
> -	 *				    atomic_dec_and_test
> -	 *				  exit_oom_victim
> -	 *				[...]
> -	 *				out_of_memory
> -	 *				  select_bad_process
> -	 *				    # no TIF_MEMDIE task selects new victim
> -	 *  unmap_page_range # frees some memory
> -	 */
> -	mutex_lock(&oom_lock);
>  
>  	if (!down_read_trylock(&mm->mmap_sem)) {
> -		ret = false;
>  		trace_skip_task_reaping(tsk->pid);
> -		goto unlock_oom;
> +		return false;
>  	}
>  
>  	/*
> @@ -502,7 +490,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
>  	if (!mmget_not_zero(mm)) {
>  		up_read(&mm->mmap_sem);
>  		trace_skip_task_reaping(tsk->pid);
> -		goto unlock_oom;
> +		return true;
>  	}
>  
>  	trace_start_task_reaping(tsk->pid);
> @@ -549,9 +537,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
>  	 */
>  	mmput_async(mm);
>  	trace_finish_task_reaping(tsk->pid);
> -unlock_oom:
> -	mutex_unlock(&oom_lock);
> -	return ret;
> +	return true;
>  }
>  
>  #define MAX_OOM_REAP_RETRIES 10
> @@ -661,8 +647,10 @@ static void mark_oom_victim(struct task_struct *tsk)
>  		return;
>  
>  	/* oom_mm is bound to the signal struct life time. */
> -	if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm))
> -		mmgrab(tsk->signal->oom_mm);
> +	if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm)) {
> +		mmgrab(mm);
> +		mm->oom_killer_synchronized = false;
> +	}
>  
>  	/*
>  	 * Make sure that the task is woken up from uninterruptible sleep
> -- 
> 1.8.3.1

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Tetsuo Handa]

Michal Hocko wrote:
> On Tue 18-07-17 23:06:50, Tetsuo Handa wrote:
> > Commit e2fe14564d3316d1 ("oom_reaper: close race with exiting task")
> > guarded whole OOM reaping operations using oom_lock. But there was no
> > need to guard whole operations. We needed to guard only setting of
> > MMF_OOM_REAPED flag because get_page_from_freelist() in
> > __alloc_pages_may_oom() is called with oom_lock held.
> > 
> > If we change to guard only setting of MMF_OOM_SKIP flag, the OOM reaper
> > can start reaping operations as soon as wake_oom_reaper() is called.
> > But since setting of MMF_OOM_SKIP flag at __mmput() is not guarded with
> > oom_lock, guarding only the OOM reaper side is not sufficient.
> > 
> > If we change the OOM killer side to ignore MMF_OOM_SKIP flag once,
> > there is no need to guard setting of MMF_OOM_SKIP flag, and we can
> > guarantee a chance to call get_page_from_freelist() in
> > __alloc_pages_may_oom() without depending on oom_lock serialization.
> > 
> > This patch makes MMF_OOM_SKIP act as if MMF_OOM_REAPED, and adds a new
> > flag which acts as if MMF_OOM_SKIP, in order to close both race window
> > (the OOM reaper side and __mmput() side) without using oom_lock.
> 
> Why do we need this patch when
> http://lkml.kernel.org/r/20170626130346.26314-1-mhocko@kernel.org
> already removes the lock and solves another problem at once?

We haven't got an answer from Hugh and/or Andrea whether that patch is safe.
Even if that patch is safe, this patch still helps with CONFIG_MMU=n case.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Michal Hocko]

On Wed 19-07-17 05:51:03, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Tue 18-07-17 23:06:50, Tetsuo Handa wrote:
> > > Commit e2fe14564d3316d1 ("oom_reaper: close race with exiting task")
> > > guarded whole OOM reaping operations using oom_lock. But there was no
> > > need to guard whole operations. We needed to guard only setting of
> > > MMF_OOM_REAPED flag because get_page_from_freelist() in
> > > __alloc_pages_may_oom() is called with oom_lock held.
> > > 
> > > If we change to guard only setting of MMF_OOM_SKIP flag, the OOM reaper
> > > can start reaping operations as soon as wake_oom_reaper() is called.
> > > But since setting of MMF_OOM_SKIP flag at __mmput() is not guarded with
> > > oom_lock, guarding only the OOM reaper side is not sufficient.
> > > 
> > > If we change the OOM killer side to ignore MMF_OOM_SKIP flag once,
> > > there is no need to guard setting of MMF_OOM_SKIP flag, and we can
> > > guarantee a chance to call get_page_from_freelist() in
> > > __alloc_pages_may_oom() without depending on oom_lock serialization.
> > > 
> > > This patch makes MMF_OOM_SKIP act as if MMF_OOM_REAPED, and adds a new
> > > flag which acts as if MMF_OOM_SKIP, in order to close both race window
> > > (the OOM reaper side and __mmput() side) without using oom_lock.
> > 
> > Why do we need this patch when
> > http://lkml.kernel.org/r/20170626130346.26314-1-mhocko@kernel.org
> > already removes the lock and solves another problem at once?
> 
> We haven't got an answer from Hugh and/or Andrea whether that patch is safe.

So what? I haven't see anybody disputing the correctness. And to be
honest I really dislike your patch. Yet another round kind of solutions
are just very ugly hacks usually because they are highly timing
sensitive.

> Even if that patch is safe, this patch still helps with CONFIG_MMU=n case.

Could you explain how?
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Tetsuo Handa]

Michal Hocko wrote:
> On Wed 19-07-17 05:51:03, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Tue 18-07-17 23:06:50, Tetsuo Handa wrote:
> > > > Commit e2fe14564d3316d1 ("oom_reaper: close race with exiting task")
> > > > guarded whole OOM reaping operations using oom_lock. But there was no
> > > > need to guard whole operations. We needed to guard only setting of
> > > > MMF_OOM_REAPED flag because get_page_from_freelist() in
> > > > __alloc_pages_may_oom() is called with oom_lock held.
> > > > 
> > > > If we change to guard only setting of MMF_OOM_SKIP flag, the OOM reaper
> > > > can start reaping operations as soon as wake_oom_reaper() is called.
> > > > But since setting of MMF_OOM_SKIP flag at __mmput() is not guarded with
> > > > oom_lock, guarding only the OOM reaper side is not sufficient.
> > > > 
> > > > If we change the OOM killer side to ignore MMF_OOM_SKIP flag once,
> > > > there is no need to guard setting of MMF_OOM_SKIP flag, and we can
> > > > guarantee a chance to call get_page_from_freelist() in
> > > > __alloc_pages_may_oom() without depending on oom_lock serialization.
> > > > 
> > > > This patch makes MMF_OOM_SKIP act as if MMF_OOM_REAPED, and adds a new
> > > > flag which acts as if MMF_OOM_SKIP, in order to close both race window
> > > > (the OOM reaper side and __mmput() side) without using oom_lock.
> > > 
> > > Why do we need this patch when
> > > http://lkml.kernel.org/r/20170626130346.26314-1-mhocko@kernel.org
> > > already removes the lock and solves another problem at once?
> > 
> > We haven't got an answer from Hugh and/or Andrea whether that patch is safe.
> 
> So what? I haven't see anybody disputing the correctness. And to be
> honest I really dislike your patch. Yet another round kind of solutions
> are just very ugly hacks usually because they are highly timing
> sensitive.

Yes, OOM killer is highly timing sensitive.

> 
> > Even if that patch is safe, this patch still helps with CONFIG_MMU=n case.
> 
> Could you explain how?

Nothing prevents sequence below.

    Process-1              Process-2

    Takes oom_lock.
    Fails get_page_from_freelist().
    Enters out_of_memory().
    Gets SIGKILL.
    Gets TIF_MEMDIE.
    Leaves out_of_memory().
    Releases oom_lock.
    Enters do_exit().
    Calls __mmput().
                           Takes oom_lock.
                           Fails get_page_from_freelist().
    Releases some memory.
    Sets MMF_OOM_SKIP.
                           Enters out_of_memory().
                           Selects next victim because there is no !MMF_OOM_SKIP mm.
                           Sends SIGKILL needlessly.

If we ignore MMF_OOM_SKIP once, we can avoid sequence above.

    Process-1              Process-2

    Takes oom_lock.
    Fails get_page_from_freelist().
    Enters out_of_memory().
    Get SIGKILL.
    Get TIF_MEMDIE.
    Leaves out_of_memory().
    Releases oom_lock.
    Enters do_exit().
    Calls __mmput().
                           Takes oom_lock.
                           Fails get_page_from_freelist().
    Releases some memory.
    Sets MMF_OOM_SKIP.
                           Enters out_of_memory().
                           Ignores MMF_OOM_SKIP mm once.
                           Leaves out_of_memory().
                           Releases oom_lock.
                           Succeeds get_page_from_freelist().

Strictly speaking, this patch is independent with OOM reaper.
This patch increases possibility of succeeding get_page_from_freelist()
without sending SIGKILL. Your patch is trying to drop it silently.

Serializing setting of MMF_OOM_SKIP with oom_lock is one approach,
and ignoring MMF_OOM_SKIP once without oom_lock is another approach.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Michal Hocko]

On Fri 21-07-17 06:47:11, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Wed 19-07-17 05:51:03, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > On Tue 18-07-17 23:06:50, Tetsuo Handa wrote:
> > > > > Commit e2fe14564d3316d1 ("oom_reaper: close race with exiting task")
> > > > > guarded whole OOM reaping operations using oom_lock. But there was no
> > > > > need to guard whole operations. We needed to guard only setting of
> > > > > MMF_OOM_REAPED flag because get_page_from_freelist() in
> > > > > __alloc_pages_may_oom() is called with oom_lock held.
> > > > > 
> > > > > If we change to guard only setting of MMF_OOM_SKIP flag, the OOM reaper
> > > > > can start reaping operations as soon as wake_oom_reaper() is called.
> > > > > But since setting of MMF_OOM_SKIP flag at __mmput() is not guarded with
> > > > > oom_lock, guarding only the OOM reaper side is not sufficient.
> > > > > 
> > > > > If we change the OOM killer side to ignore MMF_OOM_SKIP flag once,
> > > > > there is no need to guard setting of MMF_OOM_SKIP flag, and we can
> > > > > guarantee a chance to call get_page_from_freelist() in
> > > > > __alloc_pages_may_oom() without depending on oom_lock serialization.
> > > > > 
> > > > > This patch makes MMF_OOM_SKIP act as if MMF_OOM_REAPED, and adds a new
> > > > > flag which acts as if MMF_OOM_SKIP, in order to close both race window
> > > > > (the OOM reaper side and __mmput() side) without using oom_lock.
> > > > 
> > > > Why do we need this patch when
> > > > http://lkml.kernel.org/r/20170626130346.26314-1-mhocko@kernel.org
> > > > already removes the lock and solves another problem at once?
> > > 
> > > We haven't got an answer from Hugh and/or Andrea whether that patch is safe.
> > 
> > So what? I haven't see anybody disputing the correctness. And to be
> > honest I really dislike your patch. Yet another round kind of solutions
> > are just very ugly hacks usually because they are highly timing
> > sensitive.
> 
> Yes, OOM killer is highly timing sensitive.
> 
> > 
> > > Even if that patch is safe, this patch still helps with CONFIG_MMU=n case.
> > 
> > Could you explain how?
> 
> Nothing prevents sequence below.
> 
>     Process-1              Process-2
> 
>     Takes oom_lock.
>     Fails get_page_from_freelist().
>     Enters out_of_memory().
>     Gets SIGKILL.
>     Gets TIF_MEMDIE.
>     Leaves out_of_memory().
>     Releases oom_lock.
>     Enters do_exit().
>     Calls __mmput().
>                            Takes oom_lock.
>                            Fails get_page_from_freelist().
>     Releases some memory.
>     Sets MMF_OOM_SKIP.
>                            Enters out_of_memory().
>                            Selects next victim because there is no !MMF_OOM_SKIP mm.
>                            Sends SIGKILL needlessly.
> 
> If we ignore MMF_OOM_SKIP once, we can avoid sequence above.

But we set MMF_OOM_SKIP _after_ the process lost its address space (well
after the patch which allows to race oom reaper with the exit_mmap).

> 
>     Process-1              Process-2
> 
>     Takes oom_lock.
>     Fails get_page_from_freelist().
>     Enters out_of_memory().
>     Get SIGKILL.
>     Get TIF_MEMDIE.
>     Leaves out_of_memory().
>     Releases oom_lock.
>     Enters do_exit().
>     Calls __mmput().
>                            Takes oom_lock.
>                            Fails get_page_from_freelist().
>     Releases some memory.
>     Sets MMF_OOM_SKIP.
>                            Enters out_of_memory().
>                            Ignores MMF_OOM_SKIP mm once.
>                            Leaves out_of_memory().
>                            Releases oom_lock.
>                            Succeeds get_page_from_freelist().

OK, so let's say you have another task just about to jump into
out_of_memory and ... end up in the same situation. This race is just
unavoidable.

> Strictly speaking, this patch is independent with OOM reaper.
> This patch increases possibility of succeeding get_page_from_freelist()
> without sending SIGKILL. Your patch is trying to drop it silently.
> 
> Serializing setting of MMF_OOM_SKIP with oom_lock is one approach,
> and ignoring MMF_OOM_SKIP once without oom_lock is another approach.

Or simply making sure that we only set the flag _after_ the address
space is gone, which is what I am proposing.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Tetsuo Handa]

Michal Hocko wrote:
> > If we ignore MMF_OOM_SKIP once, we can avoid sequence above.
> 
> But we set MMF_OOM_SKIP _after_ the process lost its address space (well
> after the patch which allows to race oom reaper with the exit_mmap).
> 
> > 
> >     Process-1              Process-2
> > 
> >     Takes oom_lock.
> >     Fails get_page_from_freelist().
> >     Enters out_of_memory().
> >     Get SIGKILL.
> >     Get TIF_MEMDIE.
> >     Leaves out_of_memory().
> >     Releases oom_lock.
> >     Enters do_exit().
> >     Calls __mmput().
> >                            Takes oom_lock.
> >                            Fails get_page_from_freelist().
> >     Releases some memory.
> >     Sets MMF_OOM_SKIP.
> >                            Enters out_of_memory().
> >                            Ignores MMF_OOM_SKIP mm once.
> >                            Leaves out_of_memory().
> >                            Releases oom_lock.
> >                            Succeeds get_page_from_freelist().
> 
> OK, so let's say you have another task just about to jump into
> out_of_memory and ... end up in the same situation.

Right.

> 
>                                                     This race is just
> unavoidable.

There is no perfect way (always timing dependent). But

> 
> > Strictly speaking, this patch is independent with OOM reaper.
> > This patch increases possibility of succeeding get_page_from_freelist()
> > without sending SIGKILL. Your patch is trying to drop it silently.

we can try to reduce possibility of ending up in the same situation by
this proposal, and your proposal is irrelevant with reducing possibility of
ending up in the same situation because

> > 
> > Serializing setting of MMF_OOM_SKIP with oom_lock is one approach,
> > and ignoring MMF_OOM_SKIP once without oom_lock is another approach.
> 
> Or simply making sure that we only set the flag _after_ the address
> space is gone, which is what I am proposing.

the address space being gone does not guarantee that get_page_from_freelist()
shall be called before entering into out_of_memory() (e.g. preempted for seconds
between "Fails get_page_from_freelist()." and "Enters out_of_memory().").

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Michal Hocko]

On Sat 22-07-17 00:18:48, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > > If we ignore MMF_OOM_SKIP once, we can avoid sequence above.
> > 
> > But we set MMF_OOM_SKIP _after_ the process lost its address space (well
> > after the patch which allows to race oom reaper with the exit_mmap).
> > 
> > > 
> > >     Process-1              Process-2
> > > 
> > >     Takes oom_lock.
> > >     Fails get_page_from_freelist().
> > >     Enters out_of_memory().
> > >     Get SIGKILL.
> > >     Get TIF_MEMDIE.
> > >     Leaves out_of_memory().
> > >     Releases oom_lock.
> > >     Enters do_exit().
> > >     Calls __mmput().
> > >                            Takes oom_lock.
> > >                            Fails get_page_from_freelist().
> > >     Releases some memory.
> > >     Sets MMF_OOM_SKIP.
> > >                            Enters out_of_memory().
> > >                            Ignores MMF_OOM_SKIP mm once.
> > >                            Leaves out_of_memory().
> > >                            Releases oom_lock.
> > >                            Succeeds get_page_from_freelist().
> > 
> > OK, so let's say you have another task just about to jump into
> > out_of_memory and ... end up in the same situation.
> 
> Right.
> 
> > 
> >                                                     This race is just
> > unavoidable.
> 
> There is no perfect way (always timing dependent). But

I would rather not add a code which _pretends_ it solves something. If
we see the above race a real problem in out there then we should think
about how to fix it. I definitely do not want to add more hack into an
already complicated code base.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Tetsuo Handa]

Michal Hocko wrote:
> On Sat 22-07-17 00:18:48, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > OK, so let's say you have another task just about to jump into
> > > out_of_memory and ... end up in the same situation.
> > 
> > Right.
> > 
> > > 
> > >                                                     This race is just
> > > unavoidable.
> > 
> > There is no perfect way (always timing dependent). But
> 
> I would rather not add a code which _pretends_ it solves something. If
> we see the above race a real problem in out there then we should think
> about how to fix it. I definitely do not want to add more hack into an
> already complicated code base.

So, how can we verify the above race a real problem? I consider that
it is impossible. The " free:%lukB" field by show_free_areas() is too
random/inaccurate/racy/outdated for evaluating this race window.

Only actually calling alloc_page_from_freelist() immediately after
MMF_OOM_SKIP test (like Patch1 shown below) can evaluate this race window,
but I know that you won't allow me to add such code to the OOM killer layer.

Your "[RFC PATCH] mm, oom: allow oom reaper to race with exit_mmap" patch
is shown below as Patch2.

My "ignore MMF_OOM_SKIP once" patch is shown below as Patch3.

My "wait for oom_lock" patch is shown below as Patch4.

Patch1:
----------------------------------------
 include/linux/oom.h |  4 ++++
 mm/internal.h       |  4 ++++
 mm/oom_kill.c       | 28 +++++++++++++++++++++++++++-
 mm/page_alloc.c     | 10 +++++++---
 4 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/include/linux/oom.h b/include/linux/oom.h
index 8a266e2..1b0bbb6 100644
--- a/include/linux/oom.h
+++ b/include/linux/oom.h
@@ -11,6 +11,7 @@
 struct notifier_block;
 struct mem_cgroup;
 struct task_struct;
+struct alloc_context;
 
 /*
  * Details of the page allocation that triggered the oom killer that are used to
@@ -39,6 +40,9 @@ struct oom_control {
 	unsigned long totalpages;
 	struct task_struct *chosen;
 	unsigned long chosen_points;
+
+	const struct alloc_context *alloc_context;
+	unsigned int alloc_flags;
 };
 
 extern struct mutex oom_lock;
diff --git a/mm/internal.h b/mm/internal.h
index 24d88f0..95a08b5 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -522,4 +522,8 @@ static inline bool is_migrate_highatomic_page(struct page *page)
 	return get_pageblock_migratetype(page) == MIGRATE_HIGHATOMIC;
 }
 
+struct page *get_page_from_freelist(gfp_t gfp_mask, unsigned int order,
+				    int alloc_flags,
+				    const struct alloc_context *ac);
+
 #endif	/* __MM_INTERNAL_H */
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 9e8b4f0..fb7b2c8 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -288,6 +288,9 @@ static enum oom_constraint constrained_alloc(struct oom_control *oc)
 	return CONSTRAINT_NONE;
 }
 
+static unsigned int mmf_oom_skip_raced;
+static unsigned int mmf_oom_skip_not_raced;
+
 static int oom_evaluate_task(struct task_struct *task, void *arg)
 {
 	struct oom_control *oc = arg;
@@ -303,8 +306,21 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
 	 * any memory is quite low.
 	 */
 	if (!is_sysrq_oom(oc) && tsk_is_oom_victim(task)) {
-		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags))
+		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags)) {
+			const struct alloc_context *ac = oc->alloc_context;
+
+			if (ac) {
+				struct page *page = get_page_from_freelist
+					(oc->gfp_mask, oc->order,
+					 oc->alloc_flags, ac);
+				if (page) {
+					__free_pages(page, oc->order);
+					mmf_oom_skip_raced++;
+				} else
+					mmf_oom_skip_not_raced++;
+			}
 			goto next;
+		}
 		goto abort;
 	}
 
@@ -1059,6 +1075,16 @@ bool out_of_memory(struct oom_control *oc)
 		 */
 		schedule_timeout_killable(1);
 	}
+	{
+		static unsigned long last;
+		unsigned long now = jiffies;
+
+		if (!last || time_after(now, last + 5 * HZ)) {
+			last = now;
+			pr_info("MMF_OOM_SKIP: raced=%u not_raced=%u\n",
+				mmf_oom_skip_raced, mmf_oom_skip_not_raced);
+		}
+	}
 	return !!oc->chosen;
 }
 
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 80e4adb..4cf2861 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3054,7 +3054,7 @@ static bool zone_allows_reclaim(struct zone *local_zone, struct zone *zone)
  * get_page_from_freelist goes through the zonelist trying to allocate
  * a page.
  */
-static struct page *
+struct page *
 get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
 						const struct alloc_context *ac)
 {
@@ -3245,7 +3245,8 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
 
 static inline struct page *
 __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
-	const struct alloc_context *ac, unsigned long *did_some_progress)
+		      unsigned int alloc_flags, const struct alloc_context *ac,
+		      unsigned long *did_some_progress)
 {
 	struct oom_control oc = {
 		.zonelist = ac->zonelist,
@@ -3253,6 +3254,8 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
 		.memcg = NULL,
 		.gfp_mask = gfp_mask,
 		.order = order,
+		.alloc_context = ac,
+		.alloc_flags = alloc_flags,
 	};
 	struct page *page;
 
@@ -3955,7 +3958,8 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
 		goto retry_cpuset;
 
 	/* Reclaim has failed us, start killing things */
-	page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
+	page = __alloc_pages_may_oom(gfp_mask, order, alloc_flags, ac,
+				     &did_some_progress);
 	if (page)
 		goto got_pg;
 
----------------------------------------

Patch2:
----------------------------------------
 mm/mmap.c     |  7 +++++++
 mm/oom_kill.c | 35 +++++------------------------------
 2 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index f19efcf..669f07d 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2993,6 +2993,11 @@ void exit_mmap(struct mm_struct *mm)
 	/* Use -1 here to ensure all VMAs in the mm are unmapped */
 	unmap_vmas(&tlb, vma, 0, -1);
 
+	/*
+	 * oom reaper might race with exit_mmap so make sure we won't free
+	 * page tables or unmap VMAs under its feet
+	 */
+	down_write(&mm->mmap_sem);
 	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
 	tlb_finish_mmu(&tlb, 0, -1);
 
@@ -3005,7 +3010,9 @@ void exit_mmap(struct mm_struct *mm)
 			nr_accounted += vma_pages(vma);
 		vma = remove_vma(vma);
 	}
+	mm->mmap = NULL;
 	vm_unacct_memory(nr_accounted);
+	up_write(&mm->mmap_sem);
 }
 
 /* Insert vm structure into process list sorted by address
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index fb7b2c8..3ef14f0 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -486,39 +486,16 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 {
 	struct mmu_gather tlb;
 	struct vm_area_struct *vma;
-	bool ret = true;
-
-	/*
-	 * We have to make sure to not race with the victim exit path
-	 * and cause premature new oom victim selection:
-	 * __oom_reap_task_mm		exit_mm
-	 *   mmget_not_zero
-	 *				  mmput
-	 *				    atomic_dec_and_test
-	 *				  exit_oom_victim
-	 *				[...]
-	 *				out_of_memory
-	 *				  select_bad_process
-	 *				    # no TIF_MEMDIE task selects new victim
-	 *  unmap_page_range # frees some memory
-	 */
-	mutex_lock(&oom_lock);
 
 	if (!down_read_trylock(&mm->mmap_sem)) {
-		ret = false;
 		trace_skip_task_reaping(tsk->pid);
-		goto unlock_oom;
+		return false;
 	}
 
-	/*
-	 * increase mm_users only after we know we will reap something so
-	 * that the mmput_async is called only when we have reaped something
-	 * and delayed __mmput doesn't matter that much
-	 */
-	if (!mmget_not_zero(mm)) {
+	/* There is nothing to reap so bail out without signs in the log */
+	if (!mm->mmap) {
 		up_read(&mm->mmap_sem);
-		trace_skip_task_reaping(tsk->pid);
-		goto unlock_oom;
+		return true;
 	}
 
 	trace_start_task_reaping(tsk->pid);
@@ -565,9 +542,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 	 */
 	mmput_async(mm);
 	trace_finish_task_reaping(tsk->pid);
-unlock_oom:
-	mutex_unlock(&oom_lock);
-	return ret;
+	return true;
 }
 
 #define MAX_OOM_REAP_RETRIES 10
----------------------------------------

Patch3:
----------------------------------------
 mm/oom_kill.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 3ef14f0..9cc6634 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -306,7 +306,7 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
 	 * any memory is quite low.
 	 */
 	if (!is_sysrq_oom(oc) && tsk_is_oom_victim(task)) {
-		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags)) {
+		if (task->signal->oom_mm->async_put_work.func) {
 			const struct alloc_context *ac = oc->alloc_context;
 
 			if (ac) {
@@ -321,6 +321,8 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
 			}
 			goto next;
 		}
+		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags))
+			task->signal->oom_mm->async_put_work.func = (void *) 1;
 		goto abort;
 	}
 
@@ -652,8 +654,10 @@ static void mark_oom_victim(struct task_struct *tsk)
 		return;
 
 	/* oom_mm is bound to the signal struct life time. */
-	if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm))
+	if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm)) {
 		mmgrab(tsk->signal->oom_mm);
+		tsk->signal->oom_mm->async_put_work.func = NULL;
+	}
 
 	/*
 	 * Make sure that the task is woken up from uninterruptible sleep
----------------------------------------

Patch4:
----------------------------------------
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 4cf2861..3e0e7da 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3265,7 +3265,7 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
 	 * Acquire the oom lock.  If that fails, somebody else is
 	 * making progress for us.
 	 */
-	if (!mutex_trylock(&oom_lock)) {
+	if (mutex_lock_killable(&oom_lock)) {
 		*did_some_progress = 1;
 		schedule_timeout_uninterruptible(1);
 		return NULL;
----------------------------------------

Memory stressor is shown below.
----------------------------------------
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <poll.h>

int main(int argc, char *argv[])
{
        static char buffer[4096] = { };
        char *buf = NULL;
        unsigned long size;
        unsigned long i;
        for (i = 0; i < 1024; i++) {
                if (fork() == 0) {
                        int fd = open("/proc/self/oom_score_adj", O_WRONLY);
                        write(fd, "1000", 4);
                        close(fd);
                        sleep(1);
                        if (!i)
                                pause();
                        snprintf(buffer, sizeof(buffer), "/tmp/file.%u", getpid());
                        fd = open(buffer, O_WRONLY | O_CREAT | O_APPEND, 0600);
                        while (write(fd, buffer, sizeof(buffer)) == sizeof(buffer)) {
                                poll(NULL, 0, 10);
                                fsync(fd);
                        }
                        _exit(0);
                }
        }
        for (size = 1048576; size < 512UL * (1 << 30); size <<= 1) {
                char *cp = realloc(buf, size);
                if (!cp) {
                        size >>= 1;
                        break;
                }
                buf = cp;
        }
        sleep(2);
        /* Will cause OOM due to overcommit */
        for (i = 0; i < size; i += 4096)
                buf[i] = 0;
        pause();
        return 0;
}
----------------------------------------

Log is at http://I-love.SAKURA.ne.jp/tmp/serial-20170722.txt.xz .

# grep MMF_OOM_SKIP serial-20170722.txt | sed -e 's/=/ /g' | awk ' { if ($5 + $7) printf("%10u %10u %10f\n", $5, $7, ($5*100/($5+$7))); else printf("-----\n"); }'
----------------------------------------
----- # Patch1
         0         10   0.000000
         0         25   0.000000
        16        178   8.247423
        16        591   2.635914
        51       1476   3.339882
        51       1517   3.252551
        51       1559   3.167702
        51       1602   3.085299
        51       1646   3.005303
        51       1832   2.708444
        51       1931   2.573158
        51       2141   2.326642
       172       2950   5.509289
       172       4890   3.397866
       471       7916   5.615834
       471       8255   5.397662
       471       8717   5.126252
       471       8954   4.997347
       471       9435   4.754694
       471      10060   4.472510
       471      10840   4.164088
       471      10973   4.115694
       471      12475   3.638189
       471      14318   3.184800
       471      14762   3.091971
       471      16122   2.838546
       471      16433   2.786323
       471      16748   2.735350
       471      17067   2.685597
       471      18507   2.481821
       471      19173   2.397679
       471      22002   2.095848
       471      22173   2.080021
       471      22867   2.018168
       655      26574   2.405524
       655      30397   2.109365
       655      31030   2.067224
       655      32971   1.947897
       655      33414   1.922569
       655      33637   1.910066
       682      34285   1.950410
       682      34740   1.925357
       936      34740   2.623613
       936      34740   2.623613
       936      34777   2.620894
       936      34846   2.615840
       936      35104   2.597114
       968      35377   2.663365
      1046      36776   2.765586
      1099      38417   2.781152
      1176      41715   2.741834
      1176      42957   2.664673
      1286      55200   2.276670
      1640      67105   2.385628
      2138     186214   1.135109
      2138     188287   1.122752
      2138     188288   1.122746
      2164     188724   1.133649
      2164     189131   1.131237
      2164     189432   1.129460
      2164     190152   1.125231
      2164     190323   1.124232
      2164     190890   1.120930
      2164     193030   1.108641
      2164     197603   1.083262
      2283     199866   1.129365
      2283     202543   1.114605
      2283     203293   1.110538
      2437     204552   1.177357
----- # Patch1 + Patch2
         2        151   1.307190
         2        188   1.052632
         2        208   0.952381
         2        208   0.952381
         2        223   0.888889
         8        355   2.203857
        62        640   8.831909
        96       1681   5.402364
        96       3381   2.761001
       190       5403   3.397104
       344      14944   2.250131
       589      31461   1.837754
       589      65517   0.890993
       589      99284   0.589749
       750     204676   0.365095
      1157     283736   0.406117
      1157     286966   0.401565
      1647     368642   0.444788
      4870     494913   0.974423
      8615     646051   1.315938
      9266     743860   1.230339
----- # Patch1 + Patch2 + Patch3
         0         39   0.000000
         0        109   0.000000
         0        189   0.000000
         0        922   0.000000
        31       1101   2.738516
        31       1130   2.670112
        31       1175   2.570481
        31       1214   2.489960
        31       1230   2.458366
      2204      16429  11.828476
      9855      78544  11.148316
     17286     165828   9.440021
     29345     276217   9.603616
     41258     413082   9.080865
     63125     597249   9.558977
     73859     799400   8.457857
    100960     965601   9.465938
    100960     965806   9.464119
    100960     967986   9.444818
    101025     969145   9.440089
    101040     976753   9.374713
    101040     982309   9.326634
    101040     982469   9.325257
    101100     983224   9.323781
    101227     990001   9.276430
    101715    1045386   8.867136
    101968    1063231   8.751123
    103042    1090044   8.636595
    104288    1154220   8.286638
    105186    1230825   7.873139
----- # Patch1 + Patch2 + Patch3 + Patch4
      5400        297  94.786730
      5941       1843  76.323227
      7750       4445  63.550636
      9443       8928  51.401666
     11596      29502  28.215485
     11596     417423   2.702911
     11596     525783   2.157881
     14241     529736   2.617942
     21111     550020   3.696350
     45408     610006   6.928140
     82501     654515  11.193923
     98495     676552  12.708262
    111349     709904  13.558428
    133540     742574  15.242309
    203589     854338  19.244144
    249020    1049335  19.179654
----------------------------------------

The result shows that this race is highly timing dependent, but it
at least shows that it is not rare case that get_page_from_freelist()
can succeed after we checked that victim's mm already has MMF_OOM_SKIP.

So, how can we check the above race a real problem? I consider that
it is impossible.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Tetsuo Handa]

Tetsuo Handa wrote:
> Log is at http://I-love.SAKURA.ne.jp/tmp/serial-20170722.txt.xz .

Oops, I forgot to remove mmput_async() in Patch2. Below is updated result.
Though, situation (i.e. we can't tell without Patch1 whether we raced with
OOM_MMF_SKIP) is same.

Patch1:
----------------------------------------
 include/linux/oom.h |  4 ++++
 mm/internal.h       |  4 ++++
 mm/oom_kill.c       | 28 +++++++++++++++++++++++++++-
 mm/page_alloc.c     | 10 +++++++---
 4 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/include/linux/oom.h b/include/linux/oom.h
index 8a266e2..1b0bbb6 100644
--- a/include/linux/oom.h
+++ b/include/linux/oom.h
@@ -11,6 +11,7 @@
 struct notifier_block;
 struct mem_cgroup;
 struct task_struct;
+struct alloc_context;
 
 /*
  * Details of the page allocation that triggered the oom killer that are used to
@@ -39,6 +40,9 @@ struct oom_control {
 	unsigned long totalpages;
 	struct task_struct *chosen;
 	unsigned long chosen_points;
+
+	const struct alloc_context *alloc_context;
+	unsigned int alloc_flags;
 };
 
 extern struct mutex oom_lock;
diff --git a/mm/internal.h b/mm/internal.h
index 24d88f0..95a08b5 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -522,4 +522,8 @@ static inline bool is_migrate_highatomic_page(struct page *page)
 	return get_pageblock_migratetype(page) == MIGRATE_HIGHATOMIC;
 }
 
+struct page *get_page_from_freelist(gfp_t gfp_mask, unsigned int order,
+				    int alloc_flags,
+				    const struct alloc_context *ac);
+
 #endif	/* __MM_INTERNAL_H */
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 9e8b4f0..fb7b2c8 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -288,6 +288,9 @@ static enum oom_constraint constrained_alloc(struct oom_control *oc)
 	return CONSTRAINT_NONE;
 }
 
+static unsigned int mmf_oom_skip_raced;
+static unsigned int mmf_oom_skip_not_raced;
+
 static int oom_evaluate_task(struct task_struct *task, void *arg)
 {
 	struct oom_control *oc = arg;
@@ -303,8 +306,21 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
 	 * any memory is quite low.
 	 */
 	if (!is_sysrq_oom(oc) && tsk_is_oom_victim(task)) {
-		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags))
+		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags)) {
+			const struct alloc_context *ac = oc->alloc_context;
+
+			if (ac) {
+				struct page *page = get_page_from_freelist
+					(oc->gfp_mask, oc->order,
+					 oc->alloc_flags, ac);
+				if (page) {
+					__free_pages(page, oc->order);
+					mmf_oom_skip_raced++;
+				} else
+					mmf_oom_skip_not_raced++;
+			}
 			goto next;
+		}
 		goto abort;
 	}
 
@@ -1059,6 +1075,16 @@ bool out_of_memory(struct oom_control *oc)
 		 */
 		schedule_timeout_killable(1);
 	}
+	{
+		static unsigned long last;
+		unsigned long now = jiffies;
+
+		if (!last || time_after(now, last + 5 * HZ)) {
+			last = now;
+			pr_info("MMF_OOM_SKIP: raced=%u not_raced=%u\n",
+				mmf_oom_skip_raced, mmf_oom_skip_not_raced);
+		}
+	}
 	return !!oc->chosen;
 }
 
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 80e4adb..4cf2861 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3054,7 +3054,7 @@ static bool zone_allows_reclaim(struct zone *local_zone, struct zone *zone)
  * get_page_from_freelist goes through the zonelist trying to allocate
  * a page.
  */
-static struct page *
+struct page *
 get_page_from_freelist(gfp_t gfp_mask, unsigned int order, int alloc_flags,
 						const struct alloc_context *ac)
 {
@@ -3245,7 +3245,8 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
 
 static inline struct page *
 __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
-	const struct alloc_context *ac, unsigned long *did_some_progress)
+		      unsigned int alloc_flags, const struct alloc_context *ac,
+		      unsigned long *did_some_progress)
 {
 	struct oom_control oc = {
 		.zonelist = ac->zonelist,
@@ -3253,6 +3254,8 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
 		.memcg = NULL,
 		.gfp_mask = gfp_mask,
 		.order = order,
+		.alloc_context = ac,
+		.alloc_flags = alloc_flags,
 	};
 	struct page *page;
 
@@ -3955,7 +3958,8 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
 		goto retry_cpuset;
 
 	/* Reclaim has failed us, start killing things */
-	page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
+	page = __alloc_pages_may_oom(gfp_mask, order, alloc_flags, ac,
+				     &did_some_progress);
 	if (page)
 		goto got_pg;
 
----------------------------------------

Patch2:
----------------------------------------
 mm/mmap.c     |  7 +++++++
 mm/oom_kill.c | 41 +++++------------------------------------
 2 files changed, 12 insertions(+), 36 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index f19efcf..669f07d 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2993,6 +2993,11 @@ void exit_mmap(struct mm_struct *mm)
 	/* Use -1 here to ensure all VMAs in the mm are unmapped */
 	unmap_vmas(&tlb, vma, 0, -1);
 
+	/*
+	 * oom reaper might race with exit_mmap so make sure we won't free
+	 * page tables or unmap VMAs under its feet
+	 */
+	down_write(&mm->mmap_sem);
 	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
 	tlb_finish_mmu(&tlb, 0, -1);
 
@@ -3005,7 +3010,9 @@ void exit_mmap(struct mm_struct *mm)
 			nr_accounted += vma_pages(vma);
 		vma = remove_vma(vma);
 	}
+	mm->mmap = NULL;
 	vm_unacct_memory(nr_accounted);
+	up_write(&mm->mmap_sem);
 }
 
 /* Insert vm structure into process list sorted by address
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index fb7b2c8..ed88355 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -486,39 +486,16 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 {
 	struct mmu_gather tlb;
 	struct vm_area_struct *vma;
-	bool ret = true;
-
-	/*
-	 * We have to make sure to not race with the victim exit path
-	 * and cause premature new oom victim selection:
-	 * __oom_reap_task_mm		exit_mm
-	 *   mmget_not_zero
-	 *				  mmput
-	 *				    atomic_dec_and_test
-	 *				  exit_oom_victim
-	 *				[...]
-	 *				out_of_memory
-	 *				  select_bad_process
-	 *				    # no TIF_MEMDIE task selects new victim
-	 *  unmap_page_range # frees some memory
-	 */
-	mutex_lock(&oom_lock);
 
 	if (!down_read_trylock(&mm->mmap_sem)) {
-		ret = false;
 		trace_skip_task_reaping(tsk->pid);
-		goto unlock_oom;
+		return false;
 	}
 
-	/*
-	 * increase mm_users only after we know we will reap something so
-	 * that the mmput_async is called only when we have reaped something
-	 * and delayed __mmput doesn't matter that much
-	 */
-	if (!mmget_not_zero(mm)) {
+	/* There is nothing to reap so bail out without signs in the log */
+	if (!mm->mmap) {
 		up_read(&mm->mmap_sem);
-		trace_skip_task_reaping(tsk->pid);
-		goto unlock_oom;
+		return true;
 	}
 
 	trace_start_task_reaping(tsk->pid);
@@ -558,16 +535,8 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 			K(get_mm_counter(mm, MM_SHMEMPAGES)));
 	up_read(&mm->mmap_sem);
 
-	/*
-	 * Drop our reference but make sure the mmput slow path is called from a
-	 * different context because we shouldn't risk we get stuck there and
-	 * put the oom_reaper out of the way.
-	 */
-	mmput_async(mm);
 	trace_finish_task_reaping(tsk->pid);
-unlock_oom:
-	mutex_unlock(&oom_lock);
-	return ret;
+	return true;
 }
 
 #define MAX_OOM_REAP_RETRIES 10
----------------------------------------

Patch3:
----------------------------------------
 mm/oom_kill.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index ed88355..59737bf 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -306,7 +306,7 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
 	 * any memory is quite low.
 	 */
 	if (!is_sysrq_oom(oc) && tsk_is_oom_victim(task)) {
-		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags)) {
+		if (task->signal->oom_mm->async_put_work.func) {
 			const struct alloc_context *ac = oc->alloc_context;
 
 			if (ac) {
@@ -321,6 +321,8 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
 			}
 			goto next;
 		}
+		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags))
+			task->signal->oom_mm->async_put_work.func = (void *) 1;
 		goto abort;
 	}
 
@@ -646,8 +648,10 @@ static void mark_oom_victim(struct task_struct *tsk)
 		return;
 
 	/* oom_mm is bound to the signal struct life time. */
-	if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm))
+	if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm)) {
 		mmgrab(tsk->signal->oom_mm);
+		tsk->signal->oom_mm->async_put_work.func = NULL;
+	}
 
 	/*
 	 * Make sure that the task is woken up from uninterruptible sleep
----------------------------------------

Patch4:
----------------------------------------
 mm/page_alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 4cf2861..3e0e7da 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3265,7 +3265,7 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
 	 * Acquire the oom lock.  If that fails, somebody else is
 	 * making progress for us.
 	 */
-	if (!mutex_trylock(&oom_lock)) {
+	if (mutex_lock_killable(&oom_lock)) {
 		*did_some_progress = 1;
 		schedule_timeout_uninterruptible(1);
 		return NULL;
----------------------------------------

Log is at http://I-love.SAKURA.ne.jp/tmp/serial-20170723.txt.xz .

# grep MMF_OOM_SKIP serial-20170723.txt | sed -e 's/=/ /g' | awk ' { if ($5 + $7) printf("%10u %10u %10f\n", $5, $7, ($5*100/($5+$7))); else printf("-----\n"); }'

----------------------------------------
----- # Patch1
        42      72416   0.057965
       684     100569   0.675536
      1169     103432   1.117580
      1169     103843   1.113206
      1169     254979   0.456377
      1169     260675   0.446449
      1449     268899   0.535976
      1449     268905   0.535964
      1449     268927   0.535920
      1449     268965   0.535845
      1449     268990   0.535796
      1449     269089   0.535599
      1469     269307   0.542515
      1469     270651   0.539835
      1545     272860   0.563036
      1738     275991   0.625790
      1738     276321   0.625047
      1738     277121   0.623254
      1861     282203   0.655134
      2214     289569   0.758783
      2590     302229   0.849685
      3036     315279   0.953772
----- # Patch1 + Patch2
         0         21   0.000000
         0         45   0.000000
        12         79  13.186813
        12        159   7.017544
        12       2270   0.525855
        12       4750   0.251995
       178      15222   1.155844
       178      16997   1.036390
       178      19847   0.888889
       178      20645   0.854824
       178      23135   0.763522
       178      30479   0.580618
       178      32475   0.545126
       178      35060   0.505137
       178      36122   0.490358
       178      44854   0.395274
       178      49726   0.356685
       178      51619   0.343649
       178      57369   0.309312
       506      61344   0.818108
       506      63039   0.796286
       506      69691   0.720829
       506      83565   0.601872
       506      86330   0.582708
      1358     102218   1.311115
      1358     106653   1.257279
      1358     108003   1.241759
      1358     113901   1.178216
      1358     115739   1.159722
      1358     115739   1.159722
      1358     225671   0.598161
      1680     253286   0.658911
      9368     760763   1.216416
      9368     760852   1.216276
      9368     761841   1.214716
      9368     765167   1.209500
      9381     770368   1.203079
      9381     773975   1.197540
      9816     786044   1.233383
      9875     808291   1.206968
      9875     840890   1.160720
     10770     854555   1.244619
     10794     857956   1.242475
     10794     866148   1.230868
     11161     869111   1.267904
     11226     941179   1.178700
     11697     945889   1.221509
     12222     980317   1.231387
     12948    1038330   1.231644
     13157    1054693   1.232102
     14412    1077659   1.319694
     14953    1097134   1.344589
     15466    1252732   1.219526
----- # Patch1 + Patch2 + Patch3
         0          2   0.000000
         2         75   2.597403
        46        995   4.418828
       175       5416   3.130030
       358      15725   2.225953
       736      28838   2.488672
       736      36445   1.979506
      1008      63860   1.553925
      1008      75472   1.317992
      1008      78268   1.271507
      1408      95598   1.451457
      2142     141059   1.495800
      2537     215187   1.165237
      3123     222191   1.386066
      3478     318033   1.081767
      3618     505315   0.710899
      4768     615277   0.768976
      5939     825753   0.714086
      5939     926402   0.636999
      6969    1088325   0.636268
      7852    1361918   0.573235
----- # Patch1 + Patch2 + Patch3 + Patch4
         0         25   0.000000
         0        959   0.000000
        55       3868   1.401988
      3514      10387  25.278757
      5532      38260  12.632444
      7325      44891  14.028267
      7325      45320  13.913952
      7325      45320  13.913952
      7327      45322  13.916694
      8202      48418  14.486047
     11548      71310  13.937097
     14330      96425  12.938468
     14793     126763  10.450281
     14793     152881   8.822477
     14793     177491   7.693308
     19953     191976   9.414946
     19953     192330   9.399245
     19953     192684   9.383597
     19953     193750   9.336790
     19953     194106   9.321262
     50961     226093  18.393887
     54075     254175  17.542579
     54075     255039  17.493546
     54224     258917  17.316161
     54224     262745  17.107036
     55053     267306  17.078164
     56026     276647  16.841162
     56026     284621  16.446938
     58931     308741  16.028145
     64579     353502  15.446528
     81552     416345  16.379291
    102796     585118  14.943147
    125723     837199  13.056405
    153081    1010078  13.160797
    182049    1067762  14.566122
    184647    1111130  14.249906
----------------------------------------

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Michal Hocko]

On Sun 23-07-17 09:41:50, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Sat 22-07-17 00:18:48, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > OK, so let's say you have another task just about to jump into
> > > > out_of_memory and ... end up in the same situation.
> > > 
> > > Right.
> > > 
> > > > 
> > > >                                                     This race is just
> > > > unavoidable.
> > > 
> > > There is no perfect way (always timing dependent). But
> > 
> > I would rather not add a code which _pretends_ it solves something. If
> > we see the above race a real problem in out there then we should think
> > about how to fix it. I definitely do not want to add more hack into an
> > already complicated code base.
> 
> So, how can we verify the above race a real problem?

Try to simulate a _real_ workload and see whether we kill more tasks
than necessary. 

> I consider that
> it is impossible. The " free:%lukB" field by show_free_areas() is too
> random/inaccurate/racy/outdated for evaluating this race window.
> 
> Only actually calling alloc_page_from_freelist() immediately after
> MMF_OOM_SKIP test (like Patch1 shown below) can evaluate this race window,
> but I know that you won't allow me to add such code to the OOM killer layer.

Sigh. It is not about _me_ allowing you something or not. It is about
what makes sense and under which circumstances and usual cost benefit
evaluation. In other words, any patch has to be _justified_. I am really
tired of repeating this simple thing over and over again.

Anyway, the change you are proposing is wrong for two reasons. First,
you are in non-preemptible context in oom_evaluate_task so you cannot
call into get_page_from_freelist (node_reclaim) and secondly it is a
very specific hack while there is a whole category of possible races
where someone frees memory (e.g. and exiting task which smells like what
you see in your testing) while we are selecting an oom victim which
can be quite an expensive operation. Such races are unfortunate but
unavoidable unless we synchronize oom kill with any memory freeing which
smells like a no-go to me. We can try a last allocation attempt right
before we go and kill something (which still wouldn't be race free) but
that might cause other issues - e.g. prolonged trashing without ever
killing something - but I haven't evaluated those to be honest.

[...]

> The result shows that this race is highly timing dependent, but it
> at least shows that it is not rare case that get_page_from_freelist()
> can succeed after we checked that victim's mm already has MMF_OOM_SKIP.

It might be not rare for the extreme test case you are using. Do not
forget you spawn many tasks and them exiting might race with the oom
selection. I am really skeptical this reflects a real usecase.

> So, how can we check the above race a real problem? I consider that
> it is impossible.

And so I would be rather reluctant to add more hacks^Wheuristics...

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Tetsuo Handa]

Michal Hocko wrote:
> On Sun 23-07-17 09:41:50, Tetsuo Handa wrote:
> > So, how can we verify the above race a real problem?
> 
> Try to simulate a _real_ workload and see whether we kill more tasks
> than necessary. 

Whether it is a _real_ workload or not cannot become an answer.

If somebody is trying to allocate hundreds/thousands of pages after memory of
an OOM victim was reaped, avoiding this race window makes no sense; next OOM
victim will be selected anyway. But if somebody is trying to allocate only one
page and then is planning to release a lot of memory, avoiding this race window
can save somebody from being OOM-killed needlessly. This race window depends on
what the threads are about to do, not whether the workload is natural or
artificial.

My question is, how can users know it if somebody was OOM-killed needlessly
by allowing MMF_OOM_SKIP to race.

> Anyway, the change you are proposing is wrong for two reasons. First,
> you are in non-preemptible context in oom_evaluate_task so you cannot
> call into get_page_from_freelist (node_reclaim) and secondly it is a
> very specific hack while there is a whole category of possible races
> where someone frees memory (e.g. and exiting task which smells like what
> you see in your testing) while we are selecting an oom victim which
> can be quite an expensive operation.

Oh, I didn't know that get_page_from_freelist() might sleep.
I was assuming that get_page_from_freelist() never sleeps because it is
called from !can_direct_reclaim context. But looking into that function,
it is gfpflags_allow_blocking() from node_reclaim() from
get_page_from_freelist() that prevents !can_direct_reclaim context from
sleeping.

OK. I have to either mask __GFP_DIRECT_RECLAIM or postpone till
oom_kill_process(). Well, I came to worry about get_page_from_freelist()
at __alloc_pages_may_oom() which is called after oom_lock is taken.

Is it guaranteed that __node_reclaim() never (even indirectly) waits for
__GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory allocation? If it is not
guaranteed, calling __alloc_pages_may_oom(__GFP_DIRECT_RECLAIM) with oom_lock
taken can prevent __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory allocation from
completing (because did_some_progress will be forever set to 1 due to oom_lock
already taken). A possible location of OOM lockup unless it is guaranteed.

>                                      Such races are unfortunate but
> unavoidable unless we synchronize oom kill with any memory freeing which
> smells like a no-go to me. We can try a last allocation attempt right
> before we go and kill something (which still wouldn't be race free) but
> that might cause other issues - e.g. prolonged trashing without ever
> killing something - but I haven't evaluated those to be honest.

Yes, postpone last get_page_from_freelist() attempt till oom_kill_process()
will be what we would afford at best.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Michal Hocko]

On Wed 26-07-17 20:33:21, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Sun 23-07-17 09:41:50, Tetsuo Handa wrote:
> > > So, how can we verify the above race a real problem?
> > 
> > Try to simulate a _real_ workload and see whether we kill more tasks
> > than necessary. 
> 
> Whether it is a _real_ workload or not cannot become an answer.
> 
> If somebody is trying to allocate hundreds/thousands of pages after memory of
> an OOM victim was reaped, avoiding this race window makes no sense; next OOM
> victim will be selected anyway. But if somebody is trying to allocate only one
> page and then is planning to release a lot of memory, avoiding this race window
> can save somebody from being OOM-killed needlessly. This race window depends on
> what the threads are about to do, not whether the workload is natural or
> artificial.

And with a desparate lack of crystal ball we cannot do much about that
really.

> My question is, how can users know it if somebody was OOM-killed needlessly
> by allowing MMF_OOM_SKIP to race.

Is it really important to know that the race is due to MMF_OOM_SKIP?
Isn't it sufficient to see that we kill too many tasks and then debug it
further once something hits that?

[...]
> Is it guaranteed that __node_reclaim() never (even indirectly) waits for
> __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory allocation?

this is a direct reclaim which can go down to slab shrinkers with all
the usual fun...

> >                                      Such races are unfortunate but
> > unavoidable unless we synchronize oom kill with any memory freeing which
> > smells like a no-go to me. We can try a last allocation attempt right
> > before we go and kill something (which still wouldn't be race free) but
> > that might cause other issues - e.g. prolonged trashing without ever
> > killing something - but I haven't evaluated those to be honest.
> 
> Yes, postpone last get_page_from_freelist() attempt till oom_kill_process()
> will be what we would afford at best.

as I've said this would have to be evaluated very carefully and a strong
usecase would have to be shown.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Tetsuo Handa]

Michal Hocko wrote:
> On Wed 26-07-17 20:33:21, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Sun 23-07-17 09:41:50, Tetsuo Handa wrote:
> > > > So, how can we verify the above race a real problem?
> > > 
> > > Try to simulate a _real_ workload and see whether we kill more tasks
> > > than necessary. 
> > 
> > Whether it is a _real_ workload or not cannot become an answer.
> > 
> > If somebody is trying to allocate hundreds/thousands of pages after memory of
> > an OOM victim was reaped, avoiding this race window makes no sense; next OOM
> > victim will be selected anyway. But if somebody is trying to allocate only one
> > page and then is planning to release a lot of memory, avoiding this race window
> > can save somebody from being OOM-killed needlessly. This race window depends on
> > what the threads are about to do, not whether the workload is natural or
> > artificial.
> 
> And with a desparate lack of crystal ball we cannot do much about that
> really.
> 
> > My question is, how can users know it if somebody was OOM-killed needlessly
> > by allowing MMF_OOM_SKIP to race.
> 
> Is it really important to know that the race is due to MMF_OOM_SKIP?

Yes, it is really important. Needlessly selecting even one OOM victim is
a pain which is difficult to explain to and persuade some of customers.

> Isn't it sufficient to see that we kill too many tasks and then debug it
> further once something hits that?

It is not sufficient.

> 
> [...]
> > Is it guaranteed that __node_reclaim() never (even indirectly) waits for
> > __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory allocation?
> 
> this is a direct reclaim which can go down to slab shrinkers with all
> the usual fun...

Excuse me, but does that mean "Yes, it is" ?

As far as I checked, most shrinkers use non-scheduling operations other than
cond_resched(). But some shrinkers use lock_page()/down_write() etc. I worry
that such shrinkers might wait for __GFP_DIRECT_RECLAIM && !__GFP_NORETRY
memory allocation (i.e. "No, it isn't").

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Michal Hocko]

On Sat 05-08-17 10:02:55, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Wed 26-07-17 20:33:21, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > On Sun 23-07-17 09:41:50, Tetsuo Handa wrote:
> > > > > So, how can we verify the above race a real problem?
> > > > 
> > > > Try to simulate a _real_ workload and see whether we kill more tasks
> > > > than necessary. 
> > > 
> > > Whether it is a _real_ workload or not cannot become an answer.
> > > 
> > > If somebody is trying to allocate hundreds/thousands of pages after memory of
> > > an OOM victim was reaped, avoiding this race window makes no sense; next OOM
> > > victim will be selected anyway. But if somebody is trying to allocate only one
> > > page and then is planning to release a lot of memory, avoiding this race window
> > > can save somebody from being OOM-killed needlessly. This race window depends on
> > > what the threads are about to do, not whether the workload is natural or
> > > artificial.
> > 
> > And with a desparate lack of crystal ball we cannot do much about that
> > really.
> > 
> > > My question is, how can users know it if somebody was OOM-killed needlessly
> > > by allowing MMF_OOM_SKIP to race.
> > 
> > Is it really important to know that the race is due to MMF_OOM_SKIP?
> 
> Yes, it is really important. Needlessly selecting even one OOM victim is
> a pain which is difficult to explain to and persuade some of customers.

How is this any different from a race with a task exiting an releasing
some memory after we have crossed the point of no return and will kill
something?

> > Isn't it sufficient to see that we kill too many tasks and then debug it
> > further once something hits that?
> 
> It is not sufficient.
> 
> > 
> > [...]
> > > Is it guaranteed that __node_reclaim() never (even indirectly) waits for
> > > __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory allocation?
> > 
> > this is a direct reclaim which can go down to slab shrinkers with all
> > the usual fun...
> 
> Excuse me, but does that mean "Yes, it is" ?
> 
> As far as I checked, most shrinkers use non-scheduling operations other than
> cond_resched(). But some shrinkers use lock_page()/down_write() etc. I worry
> that such shrinkers might wait for __GFP_DIRECT_RECLAIM && !__GFP_NORETRY
> memory allocation (i.e. "No, it isn't").

Yes that is possible. Once you are in the shrinker land then you have to
count with everything. And if you want to imply that
get_page_from_freelist inside __alloc_pages_may_oom may lockup while
holding the oom_lock then you might be right but I haven't checked that
too deeply. It might be very well possible that the node reclaim bails
out early when we are under OOM.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: [PATCH] oom_reaper: close race without using oom_lock

[penguin-kernel]

Michal Hocko wrote:
> On Sat 05-08-17 10:02:55, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Wed 26-07-17 20:33:21, Tetsuo Handa wrote:
> > > > Michal Hocko wrote:
> > > > > On Sun 23-07-17 09:41:50, Tetsuo Handa wrote:
> > > > > > So, how can we verify the above race a real problem?
> > > > > 
> > > > > Try to simulate a _real_ workload and see whether we kill more tasks
> > > > > than necessary. 
> > > > 
> > > > Whether it is a _real_ workload or not cannot become an answer.
> > > > 
> > > > If somebody is trying to allocate hundreds/thousands of pages after memory of
> > > > an OOM victim was reaped, avoiding this race window makes no sense; next OOM
> > > > victim will be selected anyway. But if somebody is trying to allocate only one
> > > > page and then is planning to release a lot of memory, avoiding this race window
> > > > can save somebody from being OOM-killed needlessly. This race window depends on
> > > > what the threads are about to do, not whether the workload is natural or
> > > > artificial.
> > > 
> > > And with a desparate lack of crystal ball we cannot do much about that
> > > really.
> > > 
> > > > My question is, how can users know it if somebody was OOM-killed needlessly
> > > > by allowing MMF_OOM_SKIP to race.
> > > 
> > > Is it really important to know that the race is due to MMF_OOM_SKIP?
> > 
> > Yes, it is really important. Needlessly selecting even one OOM victim is
> > a pain which is difficult to explain to and persuade some of customers.
> 
> How is this any different from a race with a task exiting an releasing
> some memory after we have crossed the point of no return and will kill
> something?

I'm not complaining about an exiting task releasing some memory after we have
crossed the point of no return.

What I'm saying is that we can postpone "the point of no return" if we ignore
MMF_OOM_SKIP for once (both this "oom_reaper: close race without using oom_lock"
thread and "mm, oom: task_will_free_mem(current) should ignore MMF_OOM_SKIP for
once." thread). These are race conditions we can avoid without crystal ball.

I don't like leaving MMF_OOM_SKIP race window open which we can reduce to "an
exiting task releasing some memory after we have crossed the point of no return."
if we ignore MMF_OOM_SKIP for once.

> 
> > > Isn't it sufficient to see that we kill too many tasks and then debug it
> > > further once something hits that?
> > 
> > It is not sufficient.
> > 
> > > 
> > > [...]
> > > > Is it guaranteed that __node_reclaim() never (even indirectly) waits for
> > > > __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory allocation?
> > > 
> > > this is a direct reclaim which can go down to slab shrinkers with all
> > > the usual fun...
> > 
> > Excuse me, but does that mean "Yes, it is" ?
> > 
> > As far as I checked, most shrinkers use non-scheduling operations other than
> > cond_resched(). But some shrinkers use lock_page()/down_write() etc. I worry
> > that such shrinkers might wait for __GFP_DIRECT_RECLAIM && !__GFP_NORETRY
> > memory allocation (i.e. "No, it isn't").
> 
> Yes that is possible. Once you are in the shrinker land then you have to
> count with everything. And if you want to imply that
> get_page_from_freelist inside __alloc_pages_may_oom may lockup while
> holding the oom_lock then you might be right but I haven't checked that
> too deeply. It might be very well possible that the node reclaim bails
> out early when we are under OOM.

Yes, I worry that get_page_from_freelist() with oom_lock held might lockup.

If we are about to invoke the OOM killer for the first time, it is likely that
__node_reclaim() finds nothing to reclaim and will bail out immediately. But if
we are about to invoke the OOM killer again, it is possible that small amount of
memory was reclaimed by the OOM killer/reaper, and all reclaimed memory was assigned
to things which __node_reclaim() will find and try to reclaim, and any thread which
took oom_lock will call __node_reclaim() and __node_reclaim() find something
reclaimable if __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory allocation is involved.

We should consider such situation volatile (i.e. should not make assumption that
get_page_from_freelist() with oom_lock held shall bail out immediately) if shrinkers
which (directly or indirectly) involve __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory
allocation are permitted.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: [PATCH] oom_reaper: close race without using oom_lock

[Michal Hocko]

On Tue 08-08-17 11:14:50, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Sat 05-08-17 10:02:55, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > On Wed 26-07-17 20:33:21, Tetsuo Handa wrote:
> > > > > Michal Hocko wrote:
> > > > > > On Sun 23-07-17 09:41:50, Tetsuo Handa wrote:
> > > > > > > So, how can we verify the above race a real problem?
> > > > > > 
> > > > > > Try to simulate a _real_ workload and see whether we kill more tasks
> > > > > > than necessary. 
> > > > > 
> > > > > Whether it is a _real_ workload or not cannot become an answer.
> > > > > 
> > > > > If somebody is trying to allocate hundreds/thousands of pages after memory of
> > > > > an OOM victim was reaped, avoiding this race window makes no sense; next OOM
> > > > > victim will be selected anyway. But if somebody is trying to allocate only one
> > > > > page and then is planning to release a lot of memory, avoiding this race window
> > > > > can save somebody from being OOM-killed needlessly. This race window depends on
> > > > > what the threads are about to do, not whether the workload is natural or
> > > > > artificial.
> > > > 
> > > > And with a desparate lack of crystal ball we cannot do much about that
> > > > really.
> > > > 
> > > > > My question is, how can users know it if somebody was OOM-killed needlessly
> > > > > by allowing MMF_OOM_SKIP to race.
> > > > 
> > > > Is it really important to know that the race is due to MMF_OOM_SKIP?
> > > 
> > > Yes, it is really important. Needlessly selecting even one OOM victim is
> > > a pain which is difficult to explain to and persuade some of customers.
> > 
> > How is this any different from a race with a task exiting an releasing
> > some memory after we have crossed the point of no return and will kill
> > something?
> 
> I'm not complaining about an exiting task releasing some memory after we have
> crossed the point of no return.
> 
> What I'm saying is that we can postpone "the point of no return" if we ignore
> MMF_OOM_SKIP for once (both this "oom_reaper: close race without using oom_lock"
> thread and "mm, oom: task_will_free_mem(current) should ignore MMF_OOM_SKIP for
> once." thread). These are race conditions we can avoid without crystal ball.

If those races are really that common than we can handle them even
without "try once more" tricks. Really this is just an ugly hack. If you
really care then make sure that we always try to allocate from memory
reserves before going down the oom path. In other words, try to find a
robust solution rather than tweaks around a problem.

[...]
> > Yes that is possible. Once you are in the shrinker land then you have to
> > count with everything. And if you want to imply that
> > get_page_from_freelist inside __alloc_pages_may_oom may lockup while
> > holding the oom_lock then you might be right but I haven't checked that
> > too deeply. It might be very well possible that the node reclaim bails
> > out early when we are under OOM.
> 
> Yes, I worry that get_page_from_freelist() with oom_lock held might lockup.
> 
> If we are about to invoke the OOM killer for the first time, it is likely that
> __node_reclaim() finds nothing to reclaim and will bail out immediately. But if
> we are about to invoke the OOM killer again, it is possible that small amount of
> memory was reclaimed by the OOM killer/reaper, and all reclaimed memory was assigned
> to things which __node_reclaim() will find and try to reclaim, and any thread which
> took oom_lock will call __node_reclaim() and __node_reclaim() find something
> reclaimable if __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory allocation is involved.
> 
> We should consider such situation volatile (i.e. should not make assumption that
> get_page_from_freelist() with oom_lock held shall bail out immediately) if shrinkers
> which (directly or indirectly) involve __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory
> allocation are permitted.

Well, I think you are so focused on details that you most probably miss
a large picture here. Just think about the purpose of the node reclaim.
It is there to _prefer_ local allocations than go to a distant NUMA
node. So rather than speculating about details maybe it makes sense to
consider whether it actually makes any sense to even try to node reclaim
when we are OOM. In other words why to do an additional reclaim when we
just found out that all reclaim attempts have failed...

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: [PATCH] oom_reaper: close race without using oom_lock

[Tetsuo Handa]

Michal Hocko wrote:
> On Tue 08-08-17 11:14:50, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Sat 05-08-17 10:02:55, Tetsuo Handa wrote:
> > > > Michal Hocko wrote:
> > > > > On Wed 26-07-17 20:33:21, Tetsuo Handa wrote:
> > > > > > My question is, how can users know it if somebody was OOM-killed needlessly
> > > > > > by allowing MMF_OOM_SKIP to race.
> > > > > 
> > > > > Is it really important to know that the race is due to MMF_OOM_SKIP?
> > > > 
> > > > Yes, it is really important. Needlessly selecting even one OOM victim is
> > > > a pain which is difficult to explain to and persuade some of customers.
> > > 
> > > How is this any different from a race with a task exiting an releasing
> > > some memory after we have crossed the point of no return and will kill
> > > something?
> > 
> > I'm not complaining about an exiting task releasing some memory after we have
> > crossed the point of no return.
> > 
> > What I'm saying is that we can postpone "the point of no return" if we ignore
> > MMF_OOM_SKIP for once (both this "oom_reaper: close race without using oom_lock"
> > thread and "mm, oom: task_will_free_mem(current) should ignore MMF_OOM_SKIP for
> > once." thread). These are race conditions we can avoid without crystal ball.
> 
> If those races are really that common than we can handle them even
> without "try once more" tricks. Really this is just an ugly hack. If you
> really care then make sure that we always try to allocate from memory
> reserves before going down the oom path. In other words, try to find a
> robust solution rather than tweaks around a problem.

Since your "mm, oom: allow oom reaper to race with exit_mmap" patch removes
oom_lock serialization from the OOM reaper, possibility of calling out_of_memory()
due to successful mutex_trylock(&oom_lock) would increase when the OOM reaper set
MMF_OOM_SKIP quickly.

What if task_is_oom_victim(current) became true and MMF_OOM_SKIP was set
on current->mm between after __gfp_pfmemalloc_flags() returned 0 and before
out_of_memory() is called (due to successful mutex_trylock(&oom_lock)) ?

Excuse me? Are you suggesting to try memory reserves before
task_is_oom_victim(current) becomes true?

> 
> [...]
> > > Yes that is possible. Once you are in the shrinker land then you have to
> > > count with everything. And if you want to imply that
> > > get_page_from_freelist inside __alloc_pages_may_oom may lockup while
> > > holding the oom_lock then you might be right but I haven't checked that
> > > too deeply. It might be very well possible that the node reclaim bails
> > > out early when we are under OOM.
> > 
> > Yes, I worry that get_page_from_freelist() with oom_lock held might lockup.
> > 
> > If we are about to invoke the OOM killer for the first time, it is likely that
> > __node_reclaim() finds nothing to reclaim and will bail out immediately. But if
> > we are about to invoke the OOM killer again, it is possible that small amount of
> > memory was reclaimed by the OOM killer/reaper, and all reclaimed memory was assigned
> > to things which __node_reclaim() will find and try to reclaim, and any thread which
> > took oom_lock will call __node_reclaim() and __node_reclaim() find something
> > reclaimable if __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory allocation is involved.
> > 
> > We should consider such situation volatile (i.e. should not make assumption that
> > get_page_from_freelist() with oom_lock held shall bail out immediately) if shrinkers
> > which (directly or indirectly) involve __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory
> > allocation are permitted.
> 
> Well, I think you are so focused on details that you most probably miss
> a large picture here. Just think about the purpose of the node reclaim.
> It is there to _prefer_ local allocations than go to a distant NUMA
> node. So rather than speculating about details maybe it makes sense to
> consider whether it actually makes any sense to even try to node reclaim
> when we are OOM. In other words why to do an additional reclaim when we
> just found out that all reclaim attempts have failed...

Below is what I will propose if there is possibility of lockup.

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index be5bd60..718b2e7 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3271,9 +3271,11 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
 	/*
 	 * Go through the zonelist yet one more time, keep very high watermark
 	 * here, this is only to catch a parallel oom killing, we must fail if
-	 * we're still under heavy pressure.
+	 * we're still under heavy pressure. But make sure that this reclaim
+	 * attempt shall not involve __GFP_DIRECT_RECLAIM && !__GFP_NORETRY
+	 * allocation which will never fail due to oom_lock already held.
 	 */
-	page = get_page_from_freelist(gfp_mask | __GFP_HARDWALL, order,
+	page = get_page_from_freelist((gfp_mask | __GFP_HARDWALL) & ~__GFP_DIRECT_RECLAIM, order,
 					ALLOC_WMARK_HIGH|ALLOC_CPUSET, ac);
 	if (page)
 		goto out;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: [PATCH] oom_reaper: close race without using oom_lock

[Michal Hocko]

On Thu 10-08-17 21:10:30, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Tue 08-08-17 11:14:50, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > On Sat 05-08-17 10:02:55, Tetsuo Handa wrote:
> > > > > Michal Hocko wrote:
> > > > > > On Wed 26-07-17 20:33:21, Tetsuo Handa wrote:
> > > > > > > My question is, how can users know it if somebody was OOM-killed needlessly
> > > > > > > by allowing MMF_OOM_SKIP to race.
> > > > > > 
> > > > > > Is it really important to know that the race is due to MMF_OOM_SKIP?
> > > > > 
> > > > > Yes, it is really important. Needlessly selecting even one OOM victim is
> > > > > a pain which is difficult to explain to and persuade some of customers.
> > > > 
> > > > How is this any different from a race with a task exiting an releasing
> > > > some memory after we have crossed the point of no return and will kill
> > > > something?
> > > 
> > > I'm not complaining about an exiting task releasing some memory after we have
> > > crossed the point of no return.
> > > 
> > > What I'm saying is that we can postpone "the point of no return" if we ignore
> > > MMF_OOM_SKIP for once (both this "oom_reaper: close race without using oom_lock"
> > > thread and "mm, oom: task_will_free_mem(current) should ignore MMF_OOM_SKIP for
> > > once." thread). These are race conditions we can avoid without crystal ball.
> > 
> > If those races are really that common than we can handle them even
> > without "try once more" tricks. Really this is just an ugly hack. If you
> > really care then make sure that we always try to allocate from memory
> > reserves before going down the oom path. In other words, try to find a
> > robust solution rather than tweaks around a problem.
> 
> Since your "mm, oom: allow oom reaper to race with exit_mmap" patch removes
> oom_lock serialization from the OOM reaper, possibility of calling out_of_memory()
> due to successful mutex_trylock(&oom_lock) would increase when the OOM reaper set
> MMF_OOM_SKIP quickly.
> 
> What if task_is_oom_victim(current) became true and MMF_OOM_SKIP was set
> on current->mm between after __gfp_pfmemalloc_flags() returned 0 and before
> out_of_memory() is called (due to successful mutex_trylock(&oom_lock)) ?
> 
> Excuse me? Are you suggesting to try memory reserves before
> task_is_oom_victim(current) becomes true?

No what I've tried to say is that if this really is a real problem,
which I am not sure about, then the proper way to handle that is to
attempt to allocate from memory reserves for an oom victim. I would be
even willing to take the oom_lock back into the oom reaper path if the
former turnes out to be awkward to implement. But all this assumes this
is a _real_ problem.

> > [...]
> > > > Yes that is possible. Once you are in the shrinker land then you have to
> > > > count with everything. And if you want to imply that
> > > > get_page_from_freelist inside __alloc_pages_may_oom may lockup while
> > > > holding the oom_lock then you might be right but I haven't checked that
> > > > too deeply. It might be very well possible that the node reclaim bails
> > > > out early when we are under OOM.
> > > 
> > > Yes, I worry that get_page_from_freelist() with oom_lock held might lockup.
> > > 
> > > If we are about to invoke the OOM killer for the first time, it is likely that
> > > __node_reclaim() finds nothing to reclaim and will bail out immediately. But if
> > > we are about to invoke the OOM killer again, it is possible that small amount of
> > > memory was reclaimed by the OOM killer/reaper, and all reclaimed memory was assigned
> > > to things which __node_reclaim() will find and try to reclaim, and any thread which
> > > took oom_lock will call __node_reclaim() and __node_reclaim() find something
> > > reclaimable if __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory allocation is involved.
> > > 
> > > We should consider such situation volatile (i.e. should not make assumption that
> > > get_page_from_freelist() with oom_lock held shall bail out immediately) if shrinkers
> > > which (directly or indirectly) involve __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory
> > > allocation are permitted.
> > 
> > Well, I think you are so focused on details that you most probably miss
> > a large picture here. Just think about the purpose of the node reclaim.
> > It is there to _prefer_ local allocations than go to a distant NUMA
> > node. So rather than speculating about details maybe it makes sense to
> > consider whether it actually makes any sense to even try to node reclaim
> > when we are OOM. In other words why to do an additional reclaim when we
> > just found out that all reclaim attempts have failed...
> 
> Below is what I will propose if there is possibility of lockup.
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index be5bd60..718b2e7 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3271,9 +3271,11 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
>  	/*
>  	 * Go through the zonelist yet one more time, keep very high watermark
>  	 * here, this is only to catch a parallel oom killing, we must fail if
> -	 * we're still under heavy pressure.
> +	 * we're still under heavy pressure. But make sure that this reclaim
> +	 * attempt shall not involve __GFP_DIRECT_RECLAIM && !__GFP_NORETRY
> +	 * allocation which will never fail due to oom_lock already held.
>  	 */
> -	page = get_page_from_freelist(gfp_mask | __GFP_HARDWALL, order,
> +	page = get_page_from_freelist((gfp_mask | __GFP_HARDWALL) & ~__GFP_DIRECT_RECLAIM, order,
>  					ALLOC_WMARK_HIGH|ALLOC_CPUSET, ac);
>  	if (page)
>  		goto out;

get_page_from_freelist doesn't check __GFP_DIRECT_RECLAIM. I was think
something like ALLOC_OOM which would skip node reclaim.

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Re: [PATCH] oom_reaper: close race without using oom_lock

[Tetsuo Handa]

Michal Hocko wrote:
> On Thu 10-08-17 21:10:30, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Tue 08-08-17 11:14:50, Tetsuo Handa wrote:
> > > > Michal Hocko wrote:
> > > > > On Sat 05-08-17 10:02:55, Tetsuo Handa wrote:
> > > > > > Michal Hocko wrote:
> > > > > > > On Wed 26-07-17 20:33:21, Tetsuo Handa wrote:
> > > > > > > > My question is, how can users know it if somebody was OOM-killed needlessly
> > > > > > > > by allowing MMF_OOM_SKIP to race.
> > > > > > > 
> > > > > > > Is it really important to know that the race is due to MMF_OOM_SKIP?
> > > > > > 
> > > > > > Yes, it is really important. Needlessly selecting even one OOM victim is
> > > > > > a pain which is difficult to explain to and persuade some of customers.
> > > > > 
> > > > > How is this any different from a race with a task exiting an releasing
> > > > > some memory after we have crossed the point of no return and will kill
> > > > > something?
> > > > 
> > > > I'm not complaining about an exiting task releasing some memory after we have
> > > > crossed the point of no return.
> > > > 
> > > > What I'm saying is that we can postpone "the point of no return" if we ignore
> > > > MMF_OOM_SKIP for once (both this "oom_reaper: close race without using oom_lock"
> > > > thread and "mm, oom: task_will_free_mem(current) should ignore MMF_OOM_SKIP for
> > > > once." thread). These are race conditions we can avoid without crystal ball.
> > > 
> > > If those races are really that common than we can handle them even
> > > without "try once more" tricks. Really this is just an ugly hack. If you
> > > really care then make sure that we always try to allocate from memory
> > > reserves before going down the oom path. In other words, try to find a
> > > robust solution rather than tweaks around a problem.
> > 
> > Since your "mm, oom: allow oom reaper to race with exit_mmap" patch removes
> > oom_lock serialization from the OOM reaper, possibility of calling out_of_memory()
> > due to successful mutex_trylock(&oom_lock) would increase when the OOM reaper set
> > MMF_OOM_SKIP quickly.
> > 
> > What if task_is_oom_victim(current) became true and MMF_OOM_SKIP was set
> > on current->mm between after __gfp_pfmemalloc_flags() returned 0 and before
> > out_of_memory() is called (due to successful mutex_trylock(&oom_lock)) ?
> > 
> > Excuse me? Are you suggesting to try memory reserves before
> > task_is_oom_victim(current) becomes true?
> 
> No what I've tried to say is that if this really is a real problem,
> which I am not sure about, then the proper way to handle that is to
> attempt to allocate from memory reserves for an oom victim. I would be
> even willing to take the oom_lock back into the oom reaper path if the
> former turnes out to be awkward to implement. But all this assumes this
> is a _real_ problem.

Aren't we back to square one? My question is, how can users know it if
somebody was OOM-killed needlessly by allowing MMF_OOM_SKIP to race.

You don't want to call get_page_from_freelist() from out_of_memory(), do you?
But without passing a flag "whether get_page_from_freelist() with memory reserves
was already attempted if current thread is an OOM victim" to task_will_free_mem()
in out_of_memory() and a flag "whether get_page_from_freelist() without memory
reserves was already attempted if current thread is not an OOM victim" to
test_bit(MMF_OOM_SKIP) in oom_evaluate_task(), we won't be able to know
if somebody was OOM-killed needlessly by allowing MMF_OOM_SKIP to race.

Will you accept passing such flags (something like incomplete patch shown below) ?

--- a/include/linux/oom.h
+++ b/include/linux/oom.h
@@ -35,6 +35,8 @@ struct oom_control {
 	 */
 	const int order;
 
+	const bool reserves_tried;
+
 	/* Used by oom implementation, do not set */
 	unsigned long totalpages;
 	struct task_struct *chosen;
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -303,8 +303,10 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
 	 * any memory is quite low.
 	 */
 	if (!is_sysrq_oom(oc) && tsk_is_oom_victim(task)) {
-		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags))
+		if (test_bit(MMF_OOM_SKIP, &task->signal->oom_mm->flags)) {
+			WARN_ON(!oc->reserves_tried); // can't represent correctly
 			goto next;
+		}
 		goto abort;
 	}
 
@@ -762,7 +764,7 @@ static inline bool __task_will_free_mem(struct task_struct *task)
  * Caller has to make sure that task->mm is stable (hold task_lock or
  * it operates on the current).
  */
-static bool task_will_free_mem(struct task_struct *task)
+static bool task_will_free_mem(struct task_struct *task, const bool reserves_tried)
 {
 	struct mm_struct *mm = task->mm;
 	struct task_struct *p;
@@ -783,8 +785,10 @@ static bool task_will_free_mem(struct task_struct *task)
 	 * This task has already been drained by the oom reaper so there are
 	 * only small chances it will free some more
 	 */
-	if (test_bit(MMF_OOM_SKIP, &mm->flags))
+	if (test_bit(MMF_OOM_SKIP, &mm->flags)) {
+		WARN_ON(task == current && !reserves_tried);
 		return false;
+	}
 
 	if (atomic_read(&mm->mm_users) <= 1)
 		return true;
@@ -827,7 +831,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	 * its children or threads, just set TIF_MEMDIE so it can die quickly
 	 */
 	task_lock(p);
-	if (task_will_free_mem(p)) {
+	if (task_will_free_mem(p, oc->reserves_tried)) {
 		mark_oom_victim(p);
 		wake_oom_reaper(p);
 		task_unlock(p);
@@ -1011,7 +1015,7 @@ bool out_of_memory(struct oom_control *oc)
 	 * select it.  The goal is to allow it to allocate so that it may
 	 * quickly exit and free its memory.
 	 */
-	if (task_will_free_mem(current)) {
+	if (task_will_free_mem(current, oc->reserves_tried)) {
 		mark_oom_victim(current);
 		wake_oom_reaper(current);
 		return true;
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3244,7 +3244,7 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
 }
 
 static inline struct page *
-__alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
+__alloc_pages_may_oom(gfp_t gfp_mask, bool reserves_tried, unsigned int order,
 	const struct alloc_context *ac, unsigned long *did_some_progress)
 {
 	struct oom_control oc = {
@@ -3253,6 +3253,7 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...)
 		.memcg = NULL,
 		.gfp_mask = gfp_mask,
 		.order = order,
+		.reserves_tried = reserves_tried,
 	};
 	struct page *page;
 
@@ -3955,7 +3956,8 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
 		goto retry_cpuset;
 
 	/* Reclaim has failed us, start killing things */
-	page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
+	page = __alloc_pages_may_oom(gfp_mask, alloc_flags == ALLOC_OOM,
+				     order, ac, &did_some_progress);
 	if (page)
 		goto got_pg;
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] oom_reaper: close race without using oom_lock

[Johannes Weiner]

On Tue, Jul 18, 2017 at 11:06:50PM +0900, Tetsuo Handa wrote:
> Commit e2fe14564d3316d1 ("oom_reaper: close race with exiting task")
> guarded whole OOM reaping operations using oom_lock. But there was no
> need to guard whole operations. We needed to guard only setting of
> MMF_OOM_REAPED flag because get_page_from_freelist() in
> __alloc_pages_may_oom() is called with oom_lock held.
>
> If we change to guard only setting of MMF_OOM_SKIP flag, the OOM reaper
> can start reaping operations as soon as wake_oom_reaper() is called.
> But since setting of MMF_OOM_SKIP flag at __mmput() is not guarded with
> oom_lock, guarding only the OOM reaper side is not sufficient.
> 
> If we change the OOM killer side to ignore MMF_OOM_SKIP flag once,
> there is no need to guard setting of MMF_OOM_SKIP flag, and we can
> guarantee a chance to call get_page_from_freelist() in
> __alloc_pages_may_oom() without depending on oom_lock serialization.
> 
> This patch makes MMF_OOM_SKIP act as if MMF_OOM_REAPED, and adds a new
> flag which acts as if MMF_OOM_SKIP, in order to close both race window
> (the OOM reaper side and __mmput() side) without using oom_lock.

I have no idea what this is about - a race window fix? A performance
optimization? A code simplification?

Users and vendors are later going to read through these changelogs and
have to decide whether they want this patch or upgrade to a kernel
containing it. Please keep these people in mind when writing the
subject and first paragraph of the changelogs.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>