Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1690286 > unrolled thread
| Started by | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| First post | 2017-07-18 16:10 +0200 |
| Last post | 2017-07-18 16:20 +0200 |
| Articles | 14 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] oom_reaper: close race without using oom_lock Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-07-18 16:10 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Michal Hocko <mhocko@kernel.org> - 2017-07-18 16:20 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-07-18 23:00 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Michal Hocko <mhocko@kernel.org> - 2017-07-20 16:20 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-07-20 23:50 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Michal Hocko <mhocko@kernel.org> - 2017-07-21 17:10 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-07-21 17:20 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Michal Hocko <mhocko@kernel.org> - 2017-07-21 17:40 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-07-23 02:50 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-07-23 05:10 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Michal Hocko <mhocko@kernel.org> - 2017-07-24 08:40 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-07-26 13:40 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Michal Hocko <mhocko@kernel.org> - 2017-07-26 13:50 +0200
Re: [PATCH] oom_reaper: close race without using oom_lock Johannes Weiner <hannes@cmpxchg.org> - 2017-07-18 16:20 +0200
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2017-07-18 16:10 +0200 |
| Subject | [PATCH] oom_reaper: close race without using oom_lock |
| Message-ID | <u4Bmh-3oI-11@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-07-18 16:20 +0200 |
| Message-ID | <u4BvX-3ta-17@gated-at.bofh.it> |
| In reply to | #1690286 |
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
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2017-07-18 23:00 +0200 |
| Message-ID | <u4HL4-7dn-19@gated-at.bofh.it> |
| In reply to | #1690293 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-07-20 16:20 +0200 |
| Message-ID | <u5kt4-bb-11@gated-at.bofh.it> |
| In reply to | #1690645 |
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
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2017-07-20 23:50 +0200 |
| Message-ID | <u5ruy-4sb-11@gated-at.bofh.it> |
| In reply to | #1692971 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-07-21 17:10 +0200 |
| Message-ID | <u5HIZ-6p6-11@gated-at.bofh.it> |
| In reply to | #1693248 |
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
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2017-07-21 17:20 +0200 |
| Message-ID | <u5HSG-6sw-7@gated-at.bofh.it> |
| In reply to | #1693777 |
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().").
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-07-21 17:40 +0200 |
| Message-ID | <u5Ic2-6B7-31@gated-at.bofh.it> |
| In reply to | #1693781 |
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
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2017-07-23 02:50 +0200 |
| Message-ID | <u6dfQ-sX-13@gated-at.bofh.it> |
| In reply to | #1693802 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2017-07-23 05:10 +0200 |
| Message-ID | <u6frj-245-3@gated-at.bofh.it> |
| In reply to | #1694236 |
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
----------------------------------------
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-07-24 08:40 +0200 |
| Message-ID | <u6Fc6-1cC-19@gated-at.bofh.it> |
| In reply to | #1694236 |
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
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2017-07-26 13:40 +0200 |
| Message-ID | <u7sPv-89V-1@gated-at.bofh.it> |
| In reply to | #1694474 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-07-26 13:50 +0200 |
| Message-ID | <u7sZc-8do-3@gated-at.bofh.it> |
| In reply to | #1697001 |
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
[toc] | [prev] | [next] | [standalone]
| From | Johannes Weiner <hannes@cmpxchg.org> |
|---|---|
| Date | 2017-07-18 16:20 +0200 |
| Message-ID | <u4BvX-3ta-15@gated-at.bofh.it> |
| In reply to | #1690286 |
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.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web