Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1666318 > unrolled thread
| Started by | David Rientjes <rientjes@google.com> |
|---|---|
| First post | 2017-06-15 01:50 +0200 |
| Last post | 2017-06-17 07:20 +0200 |
| Articles | 9 on this page of 29 — 4 participants |
Back to article view | Back to linux.kernel
[patch] mm, oom: prevent additional oom kills before memory is freed David Rientjes <rientjes@google.com> - 2017-06-15 01:50 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Michal Hocko <mhocko@kernel.org> - 2017-06-15 12:50 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-15 13:00 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Michal Hocko <mhocko@kernel.org> - 2017-06-15 13:10 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-15 13:40 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Michal Hocko <mhocko@kernel.org> - 2017-06-15 14:10 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Michal Hocko <mhocko@kernel.org> - 2017-06-15 14:20 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-15 15:10 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Michal Hocko <mhocko@kernel.org> - 2017-06-15 15:30 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-15 23:50 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed David Rientjes <rientjes@google.com> - 2017-06-15 23:40 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Michal Hocko <mhocko@kernel.org> - 2017-06-15 14:30 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed David Rientjes <rientjes@google.com> - 2017-06-15 23:30 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Michal Hocko <mhocko@kernel.org> - 2017-06-15 23:50 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed David Rientjes <rientjes@google.com> - 2017-06-16 00:10 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Michal Hocko <mhocko@kernel.org> - 2017-06-16 00:20 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed David Rientjes <rientjes@google.com> - 2017-06-16 00:50 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Michal Hocko <mhocko@kernel.org> - 2017-06-16 10:10 +0200
Re: Re: [patch] mm, oom: prevent additional oom kills before memory is freed Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> - 2017-06-16 03:00 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> - 2017-06-16 06:10 +0200
Re: Re: [patch] mm, oom: prevent additional oom kills before memory is freed Michal Hocko <mhocko@kernel.org> - 2017-06-16 10:40 +0200
Re: Re: [patch] mm, oom: prevent additional oom kills before memory is freed Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-16 12:30 +0200
Re: Re: [patch] mm, oom: prevent additional oom kills before memory is freed Michal Hocko <mhocko@kernel.org> - 2017-06-16 13:10 +0200
Re: Re: [patch] mm, oom: prevent additional oom kills before memoryis freed Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-16 16:30 +0200
Re: Re: [patch] mm, oom: prevent additional oom kills before memoryis freed Michal Hocko <mhocko@kernel.org> - 2017-06-16 16:50 +0200
Re: Re: [patch] mm, oom: prevent additional oom kills before memory is freed Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-17 15:40 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-16 14:30 +0200
Re: [patch] mm, oom: prevent additional oom kills before memory is freed Michal Hocko <mhocko@kernel.org> - 2017-06-16 16:20 +0200
[PATCH] mm,oom_kill: Close race window of needlessly selecting new victims. Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2017-06-17 07:20 +0200
Page 2 of 2 — ← Prev page 1 [2]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-06-16 10:40 +0200 |
| Subject | Re: Re: [patch] mm, oom: prevent additional oom kills before memory is freed |
| Message-ID | <tSUXo-63M-11@gated-at.bofh.it> |
| In reply to | #1667364 |
On Fri 16-06-17 09:54:34, Tetsuo Handa wrote:
[...]
> And the patch you proposed is broken.
Thanks for your testing!
> ----------
> [ 161.846202] Out of memory: Kill process 6331 (a.out) score 999 or sacrifice child
> [ 161.850327] Killed process 6331 (a.out) total-vm:4172kB, anon-rss:84kB, file-rss:0kB, shmem-rss:0kB
> [ 161.858503] ------------[ cut here ]------------
> [ 161.861512] kernel BUG at mm/memory.c:1381!
BUG_ON(addr >= end) suggests our vma has trimmed. I guess I see what is
going on here.
__oom_reap_task_mm exit_mmap
free_pgtables
up_write(mm->mmap_sem)
down_read_trylock(&mm->mmap_sem)
remove_vma
unmap_page_range
So we need to extend the mmap_sem coverage. See the updated diff (not
the full proper patch yet).
> Please carefully consider the reason why there is VM_BUG_ON() in __mmput(),
> and clarify in your patch that what are possible side effects of racing
> uprobe_clear_state()/exit_aio()/ksm_exit()/exit_mmap() etc. with
> __oom_reap_task_mm()
Yes that definitely needs to be checked. We basically rely on the racing
part of the __mmput to not modify the address space. oom_reaper doesn't
touch any vma state except it unmaps pages which can run in parallel.
exit_aio->kill_ioctx seemingly does vm_munmap but it a) uses the
mmap_sem for write and b) it doesn't actually unmap because exit_aio
does ctx->mmap_size = 0. {ksm,khugepaged}_exit just do some houskeeping
which is not modifying the address space. I hope I will find some more
time to work on this next week. Additional test would be highly
appreciated of course.
---
diff --git a/mm/mmap.c b/mm/mmap.c
index 3bd5ecd20d4d..ca58f8a2a217 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2962,6 +2962,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);
@@ -2975,6 +2980,7 @@ void exit_mmap(struct mm_struct *mm)
vma = remove_vma(vma);
}
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 0e2c925e7826..3df464f0f48b 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -494,16 +494,6 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
}
/*
- * 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)) {
- up_read(&mm->mmap_sem);
- goto unlock_oom;
- }
-
- /*
* Tell all users of get_user/copy_from_user etc... that the content
* is no longer stable. No barriers really needed because unmapping
* should imply barriers already and the reader would hit a page fault
@@ -537,13 +527,6 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
K(get_mm_counter(mm, MM_FILEPAGES)),
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);
unlock_oom:
mutex_unlock(&oom_lock);
return ret;
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2017-06-16 12:30 +0200 |
| Subject | Re: Re: [patch] mm, oom: prevent additional oom kills before memory is freed |
| Message-ID | <tSWFQ-7co-13@gated-at.bofh.it> |
| In reply to | #1667528 |
Michal Hocko wrote:
> On Fri 16-06-17 09:54:34, Tetsuo Handa wrote:
> [...]
> > And the patch you proposed is broken.
>
> Thanks for your testing!
>
> > ----------
> > [ 161.846202] Out of memory: Kill process 6331 (a.out) score 999 or sacrifice child
> > [ 161.850327] Killed process 6331 (a.out) total-vm:4172kB, anon-rss:84kB, file-rss:0kB, shmem-rss:0kB
> > [ 161.858503] ------------[ cut here ]------------
> > [ 161.861512] kernel BUG at mm/memory.c:1381!
>
> BUG_ON(addr >= end) suggests our vma has trimmed. I guess I see what is
> going on here.
> __oom_reap_task_mm exit_mmap
> free_pgtables
> up_write(mm->mmap_sem)
> down_read_trylock(&mm->mmap_sem)
> remove_vma
> unmap_page_range
>
> So we need to extend the mmap_sem coverage. See the updated diff (not
> the full proper patch yet).
That diff is still wrong. We need to prevent __oom_reap_task_mm() from calling
unmap_page_range() when __mmput() already called exit_mm(), by setting/checking
MMF_OOM_SKIP like shown below.
diff --git a/kernel/fork.c b/kernel/fork.c
index e53770d..5ef715c 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -902,6 +902,11 @@ static inline void __mmput(struct mm_struct *mm)
exit_aio(mm);
ksm_exit(mm);
khugepaged_exit(mm); /* must run before exit_mmap */
+ /*
+ * oom reaper might race with exit_mmap so make sure we won't free
+ * page tables under its feet
+ */
+ down_write(&mm->mmap_sem);
exit_mmap(mm);
mm_put_huge_zero_page(mm);
set_mm_exe_file(mm, NULL);
@@ -913,6 +918,7 @@ static inline void __mmput(struct mm_struct *mm)
if (mm->binfmt)
module_put(mm->binfmt->module);
set_bit(MMF_OOM_SKIP, &mm->flags);
+ up_write(&mm->mmap_sem);
mmdrop(mm);
}
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 04c9143..98cca19 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -493,12 +493,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
goto unlock_oom;
}
- /*
- * 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)) {
+ if (test_bit(MMF_OOM_SKIP, &mm->flags)) {
up_read(&mm->mmap_sem);
goto unlock_oom;
}
@@ -537,13 +532,6 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
K(get_mm_counter(mm, MM_FILEPAGES)),
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);
unlock_oom:
mutex_unlock(&oom_lock);
return ret;
>
> > Please carefully consider the reason why there is VM_BUG_ON() in __mmput(),
> > and clarify in your patch that what are possible side effects of racing
> > uprobe_clear_state()/exit_aio()/ksm_exit()/exit_mmap() etc. with
> > __oom_reap_task_mm()
>
> Yes that definitely needs to be checked. We basically rely on the racing
> part of the __mmput to not modify the address space. oom_reaper doesn't
> touch any vma state except it unmaps pages which can run in parallel.
> exit_aio->kill_ioctx seemingly does vm_munmap but it a) uses the
> mmap_sem for write and b) it doesn't actually unmap because exit_aio
> does ctx->mmap_size = 0. {ksm,khugepaged}_exit just do some houskeeping
> which is not modifying the address space. I hope I will find some more
> time to work on this next week. Additional test would be highly
> appreciated of course.
Since the OOM reaper does not reap hugepages, khugepaged_exit() part could be
safe. But ksm_exit() part might interfere. If it is guaranteed to be safe,
what will go wrong if we move uprobe_clear_state()/exit_aio()/ksm_exit() etc.
to just before mmdrop() (i.e. after setting MMF_OOM_SKIP) ?
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-06-16 13:10 +0200 |
| Subject | Re: Re: [patch] mm, oom: prevent additional oom kills before memory is freed |
| Message-ID | <tSXix-7EM-3@gated-at.bofh.it> |
| In reply to | #1667617 |
On Fri 16-06-17 19:27:19, Tetsuo Handa wrote: > Michal Hocko wrote: > > On Fri 16-06-17 09:54:34, Tetsuo Handa wrote: > > [...] > > > And the patch you proposed is broken. > > > > Thanks for your testing! > > > > > ---------- > > > [ 161.846202] Out of memory: Kill process 6331 (a.out) score 999 or sacrifice child > > > [ 161.850327] Killed process 6331 (a.out) total-vm:4172kB, anon-rss:84kB, file-rss:0kB, shmem-rss:0kB > > > [ 161.858503] ------------[ cut here ]------------ > > > [ 161.861512] kernel BUG at mm/memory.c:1381! > > > > BUG_ON(addr >= end) suggests our vma has trimmed. I guess I see what is > > going on here. > > __oom_reap_task_mm exit_mmap > > free_pgtables > > up_write(mm->mmap_sem) > > down_read_trylock(&mm->mmap_sem) > > remove_vma > > unmap_page_range > > > > So we need to extend the mmap_sem coverage. See the updated diff (not > > the full proper patch yet). > > That diff is still wrong. We need to prevent __oom_reap_task_mm() from calling > unmap_page_range() when __mmput() already called exit_mm(), by setting/checking > MMF_OOM_SKIP like shown below. Care to explain why? [...] > Since the OOM reaper does not reap hugepages, khugepaged_exit() part could be > safe. I think you are mixing hugetlb and THP pages here. khugepaged_exit is about later and we do unmap those. > But ksm_exit() part might interfere. How? > If it is guaranteed to be safe, > what will go wrong if we move uprobe_clear_state()/exit_aio()/ksm_exit() etc. > to just before mmdrop() (i.e. after setting MMF_OOM_SKIP) ? I do not see why those matter and why they should be any special. Unless I miss anything we really do only care about page table tear down and the address space modification. They do none of that. -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2017-06-16 16:30 +0200 |
| Subject | Re: Re: [patch] mm, oom: prevent additional oom kills before memoryis freed |
| Message-ID | <tT0q5-1dM-1@gated-at.bofh.it> |
| In reply to | #1667642 |
Michal Hocko wrote: > On Fri 16-06-17 19:27:19, Tetsuo Handa wrote: > > Michal Hocko wrote: > > > On Fri 16-06-17 09:54:34, Tetsuo Handa wrote: > > > [...] > > > > And the patch you proposed is broken. > > > > > > Thanks for your testing! > > > > > > > ---------- > > > > [ 161.846202] Out of memory: Kill process 6331 (a.out) score 999 or sacrifice child > > > > [ 161.850327] Killed process 6331 (a.out) total-vm:4172kB, anon-rss:84kB, file-rss:0kB, shmem-rss:0kB > > > > [ 161.858503] ------------[ cut here ]------------ > > > > [ 161.861512] kernel BUG at mm/memory.c:1381! > > > > > > BUG_ON(addr >= end) suggests our vma has trimmed. I guess I see what is > > > going on here. > > > __oom_reap_task_mm exit_mmap > > > free_pgtables > > > up_write(mm->mmap_sem) > > > down_read_trylock(&mm->mmap_sem) > > > remove_vma > > > unmap_page_range > > > > > > So we need to extend the mmap_sem coverage. See the updated diff (not > > > the full proper patch yet). > > > > That diff is still wrong. We need to prevent __oom_reap_task_mm() from calling > > unmap_page_range() when __mmput() already called exit_mm(), by setting/checking > > MMF_OOM_SKIP like shown below. > > Care to explain why? I don't know. Your updated diff is causing below oops. ---------- [ 90.621890] Out of memory: Kill process 2671 (a.out) score 999 or sacrifice child [ 90.624636] Killed process 2671 (a.out) total-vm:4172kB, anon-rss:84kB, file-rss:0kB, shmem-rss:0kB [ 90.861308] general protection fault: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC [ 90.863695] Modules linked in: coretemp pcspkr sg vmw_vmci shpchp i2c_piix4 sd_mod ata_generic pata_acpi serio_raw vmwgfx drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm mptspi scsi_transport_spi mptscsih ahci mptbase libahci drm e1000 ata_piix i2c_core libata ipv6 [ 90.870672] CPU: 2 PID: 47 Comm: oom_reaper Not tainted 4.12.0-rc5+ #128 [ 90.872929] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/02/2015 [ 90.875995] task: ffff88007b6cd2c0 task.stack: ffff88007b6d0000 [ 90.878290] RIP: 0010:__oom_reap_task_mm+0xa1/0x160 [ 90.880242] RSP: 0018:ffff88007b6d3df0 EFLAGS: 00010202 [ 90.882240] RAX: 6b6b6b6b6b6b6b6b RBX: ffff880077b8cd40 RCX: 0000000000000000 [ 90.884612] RDX: ffff88007b6d3e18 RSI: ffff880077b8cd40 RDI: ffff88007b6d3df0 [ 90.887001] RBP: ffff88007b6d3e98 R08: ffff88007b6cdb08 R09: ffff88007b6cdad0 [ 90.889702] R10: 0000000000000000 R11: 000000009213dd65 R12: ffff880077b8ce00 [ 90.892973] R13: ffff880076f48040 R14: 6b6b6b6b6b6b6b6b R15: ffff880077b8cd40 [ 90.895765] FS: 0000000000000000(0000) GS:ffff88007c600000(0000) knlGS:0000000000000000 [ 90.899015] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 90.901462] CR2: 00007feeae35ac80 CR3: 0000000076e21000 CR4: 00000000001406e0 [ 90.904019] Call Trace: [ 90.905518] ? process_timeout+0x1/0x10 [ 90.907280] oom_reaper+0xa2/0x1b0 [ 90.908946] ? wake_up_bit+0x30/0x30 [ 90.911391] kthread+0x10d/0x140 [ 90.913003] ? __oom_reap_task_mm+0x160/0x160 [ 90.914936] ? kthread_create_on_node+0x60/0x60 [ 90.916733] ret_from_fork+0x27/0x40 [ 90.918307] Code: c3 e8 54 82 f1 ff f0 80 8b 7a 04 00 00 40 48 8d bd 58 ff ff ff 48 83 c9 ff 31 d2 48 89 de e8 57 12 03 00 4c 8b 33 4d 85 f6 74 3b <49> 8b 46 50 a9 00 24 40 00 75 27 49 83 be 90 00 00 00 00 74 04 [ 90.923922] RIP: __oom_reap_task_mm+0xa1/0x160 RSP: ffff88007b6d3df0 [ 90.929583] ---[ end trace 20f6ec27ed25c461 ]--- ---------- It is you who should explain why. I found my patch via trial and error. > [...] > > > Since the OOM reaper does not reap hugepages, khugepaged_exit() part could be > > safe. > > I think you are mixing hugetlb and THP pages here. khugepaged_exit is > about later and we do unmap those. OK. > > > But ksm_exit() part might interfere. > > How? Why you think it does not interfere? Please explain it in your patch description because your patch is trying to do a tricky thing. I'm not a MM person. I just suspect what you think no problem. > > > If it is guaranteed to be safe, > > what will go wrong if we move uprobe_clear_state()/exit_aio()/ksm_exit() etc. > > to just before mmdrop() (i.e. after setting MMF_OOM_SKIP) ? > > I do not see why those matter and why they should be any special. Unless > I miss anything we really do only care about page table tear down and > the address space modification. They do none of that. I think the patch I posted at http://lkml.kernel.org/r/201706162122.ACE95321.tOFLOOVFFHMSJQ@I-love.SAKURA.ne.jp will be safer, and you agree that a solution which is fully contained inside the oom proper would be preferable. Thus, let's start checking that patch.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-06-16 16:50 +0200 |
| Subject | Re: Re: [patch] mm, oom: prevent additional oom kills before memoryis freed |
| Message-ID | <tT0Js-1ld-15@gated-at.bofh.it> |
| In reply to | #1667793 |
On Fri 16-06-17 23:26:20, Tetsuo Handa wrote: > Michal Hocko wrote: > > On Fri 16-06-17 19:27:19, Tetsuo Handa wrote: > > > Michal Hocko wrote: > > > > On Fri 16-06-17 09:54:34, Tetsuo Handa wrote: > > > > [...] > > > > > And the patch you proposed is broken. > > > > > > > > Thanks for your testing! > > > > > > > > > ---------- > > > > > [ 161.846202] Out of memory: Kill process 6331 (a.out) score 999 or sacrifice child > > > > > [ 161.850327] Killed process 6331 (a.out) total-vm:4172kB, anon-rss:84kB, file-rss:0kB, shmem-rss:0kB > > > > > [ 161.858503] ------------[ cut here ]------------ > > > > > [ 161.861512] kernel BUG at mm/memory.c:1381! > > > > > > > > BUG_ON(addr >= end) suggests our vma has trimmed. I guess I see what is > > > > going on here. > > > > __oom_reap_task_mm exit_mmap > > > > free_pgtables > > > > up_write(mm->mmap_sem) > > > > down_read_trylock(&mm->mmap_sem) > > > > remove_vma > > > > unmap_page_range > > > > > > > > So we need to extend the mmap_sem coverage. See the updated diff (not > > > > the full proper patch yet). > > > > > > That diff is still wrong. We need to prevent __oom_reap_task_mm() from calling > > > unmap_page_range() when __mmput() already called exit_mm(), by setting/checking > > > MMF_OOM_SKIP like shown below. > > > > Care to explain why? > > I don't know. Your updated diff is causing below oops. > > ---------- > [ 90.621890] Out of memory: Kill process 2671 (a.out) score 999 or sacrifice child > [ 90.624636] Killed process 2671 (a.out) total-vm:4172kB, anon-rss:84kB, file-rss:0kB, shmem-rss:0kB > [ 90.861308] general protection fault: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC > [ 90.863695] Modules linked in: coretemp pcspkr sg vmw_vmci shpchp i2c_piix4 sd_mod ata_generic pata_acpi serio_raw vmwgfx drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm mptspi scsi_transport_spi mptscsih ahci mptbase libahci drm e1000 ata_piix i2c_core libata ipv6 > [ 90.870672] CPU: 2 PID: 47 Comm: oom_reaper Not tainted 4.12.0-rc5+ #128 > [ 90.872929] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/02/2015 > [ 90.875995] task: ffff88007b6cd2c0 task.stack: ffff88007b6d0000 > [ 90.878290] RIP: 0010:__oom_reap_task_mm+0xa1/0x160 What does this dissassemble to on your kernel? Care to post addr2line? [...] > It is you who should explain why. I can definitely try but it was really impossible to deduce that you have seen an oops from your previous email... > I found my patch via trial and error. > > > [...] > > > > > Since the OOM reaper does not reap hugepages, khugepaged_exit() part could be > > > safe. > > > > I think you are mixing hugetlb and THP pages here. khugepaged_exit is > > about later and we do unmap those. > > OK. > > > > > > But ksm_exit() part might interfere. > > > > How? > > Why you think it does not interfere? Because it doesn't modify address space in any way. > Please explain it in your patch description because your patch is > trying to do a tricky thing. I'm not a MM person. I just suspect > what you think no problem. yeah, poking holes into a patch is a reasonable approach but if you make a statement that "ksm_exit() part might interfere." then you should back it by an argument. > > > If it is guaranteed to be safe, > > > what will go wrong if we move uprobe_clear_state()/exit_aio()/ksm_exit() etc. > > > to just before mmdrop() (i.e. after setting MMF_OOM_SKIP) ? > > > > I do not see why those matter and why they should be any special. Unless > > I miss anything we really do only care about page table tear down and > > the address space modification. They do none of that. > > I think the patch I posted at > http://lkml.kernel.org/r/201706162122.ACE95321.tOFLOOVFFHMSJQ@I-love.SAKURA.ne.jp > will be safer, and you agree that a solution which is fully contained inside > the oom proper would be preferable. Thus, let's start checking that patch. Yes I will keep thinking about your approach some more but it indeed seems easier and less tricky. -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2017-06-17 15:40 +0200 |
| Subject | Re: Re: [patch] mm, oom: prevent additional oom kills before memory is freed |
| Message-ID | <tTm7f-7MM-1@gated-at.bofh.it> |
| In reply to | #1667813 |
Michal Hocko wrote:
> On Fri 16-06-17 23:26:20, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Fri 16-06-17 19:27:19, Tetsuo Handa wrote:
> > > > Michal Hocko wrote:
> > > > > On Fri 16-06-17 09:54:34, Tetsuo Handa wrote:
> > > > > [...]
> > > > > > And the patch you proposed is broken.
> > > > >
> > > > > Thanks for your testing!
> > > > >
> > > > > > ----------
> > > > > > [ 161.846202] Out of memory: Kill process 6331 (a.out) score 999 or sacrifice child
> > > > > > [ 161.850327] Killed process 6331 (a.out) total-vm:4172kB, anon-rss:84kB, file-rss:0kB, shmem-rss:0kB
> > > > > > [ 161.858503] ------------[ cut here ]------------
> > > > > > [ 161.861512] kernel BUG at mm/memory.c:1381!
> > > > >
> > > > > BUG_ON(addr >= end) suggests our vma has trimmed. I guess I see what is
> > > > > going on here.
> > > > > __oom_reap_task_mm exit_mmap
> > > > > free_pgtables
> > > > > up_write(mm->mmap_sem)
> > > > > down_read_trylock(&mm->mmap_sem)
> > > > > remove_vma
> > > > > unmap_page_range
> > > > >
> > > > > So we need to extend the mmap_sem coverage. See the updated diff (not
> > > > > the full proper patch yet).
> > > >
> > > > That diff is still wrong. We need to prevent __oom_reap_task_mm() from calling
> > > > unmap_page_range() when __mmput() already called exit_mm(), by setting/checking
> > > > MMF_OOM_SKIP like shown below.
> > >
> > > Care to explain why?
> >
> > I don't know. Your updated diff is causing below oops.
> >
> > ----------
> > [ 90.621890] Out of memory: Kill process 2671 (a.out) score 999 or sacrifice child
> > [ 90.624636] Killed process 2671 (a.out) total-vm:4172kB, anon-rss:84kB, file-rss:0kB, shmem-rss:0kB
> > [ 90.861308] general protection fault: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
> > [ 90.863695] Modules linked in: coretemp pcspkr sg vmw_vmci shpchp i2c_piix4 sd_mod ata_generic pata_acpi serio_raw vmwgfx drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm mptspi scsi_transport_spi mptscsih ahci mptbase libahci drm e1000 ata_piix i2c_core libata ipv6
> > [ 90.870672] CPU: 2 PID: 47 Comm: oom_reaper Not tainted 4.12.0-rc5+ #128
> > [ 90.872929] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/02/2015
> > [ 90.875995] task: ffff88007b6cd2c0 task.stack: ffff88007b6d0000
> > [ 90.878290] RIP: 0010:__oom_reap_task_mm+0xa1/0x160
>
> What does this dissassemble to on your kernel? Care to post addr2line?
----------
[ 114.427451] Out of memory: Kill process 2876 (a.out) score 999 or sacrifice child
[ 114.430208] Killed process 2876 (a.out) total-vm:4172kB, anon-rss:84kB, file-rss:0kB, shmem-rss:0kB
[ 114.436753] general protection fault: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[ 114.439129] Modules linked in: pcspkr coretemp sg vmw_vmci i2c_piix4 shpchp sd_mod ata_generic pata_acpi serio_raw vmwgfx drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm ahci e1000 libahci mptspi scsi_transport_spi drm mptscsih mptbase i2c_core ata_piix libata ipv6
[ 114.446220] CPU: 0 PID: 47 Comm: oom_reaper Not tainted 4.12.0-rc5+ #133
[ 114.448705] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/02/2015
[ 114.451695] task: ffff88007b6cd2c0 task.stack: ffff88007b6d0000
[ 114.453703] RIP: 0010:__oom_reap_task_mm+0xa1/0x160
[ 114.455422] RSP: 0000:ffff88007b6d3df0 EFLAGS: 00010202
[ 114.457527] RAX: 6b6b6b6b6b6b6b6b RBX: ffff8800670eaa40 RCX: 0000000000000000
[ 114.460002] RDX: ffff88007b6d3e18 RSI: ffff8800670eaa40 RDI: ffff88007b6d3df0
[ 114.462206] RBP: ffff88007b6d3e98 R08: ffff88007b6cdb08 R09: ffff88007b6cdad0
[ 114.464390] R10: 0000000000000000 R11: 0000000083f54a84 R12: ffff8800670eab00
[ 114.466659] R13: ffff880067211bc0 R14: 6b6b6b6b6b6b6b6b R15: ffff8800670eaa40
[ 114.469126] FS: 0000000000000000(0000) GS:ffff88007c200000(0000) knlGS:0000000000000000
[ 114.471496] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 114.473540] CR2: 00007f55d759d050 CR3: 0000000079ff4000 CR4: 00000000001406f0
[ 114.475773] Call Trace:
[ 114.477078] oom_reaper+0xa2/0x1b0 /* oom_reap_task at mm/oom_kill.c:542 (inlined by) oom_reaper at mm/oom_kill.c:580 */
[ 114.478569] ? wake_up_bit+0x30/0x30
[ 114.480058] kthread+0x10d/0x140
[ 114.481656] ? __oom_reap_task_mm+0x160/0x160
[ 114.483308] ? kthread_create_on_node+0x60/0x60
[ 114.485075] ret_from_fork+0x27/0x40
[ 114.486620] Code: c3 e8 54 82 f1 ff f0 80 8b 7a 04 00 00 40 48 8d bd 58 ff ff ff 48 83 c9 ff 31 d2 48 89 de e8 57 12 03 00 4c 8b 33 4d 85 f6 74 3b <49> 8b 46 50 a9 00 24 40 00 75 27 49 83 be 90 00 00 00 00 74 04
[ 114.491819] RIP: __oom_reap_task_mm+0xa1/0x160 RSP: ffff88007b6d3df0
[ 114.494520] ---[ end trace e254efa6cf6f5fe6 ]---
----------
The __oom_reap_task_mm+0xa1/0x160 is __oom_reap_task_mm at mm/oom_kill.c:472
which is "struct vm_area_struct *vma;" line in __oom_reap_task_mm().
The __oom_reap_task_mm+0xb1/0x160 is __oom_reap_task_mm at mm/oom_kill.c:519
which is "if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED))" line.
The <49> 8b 46 50 is "vma->vm_flags" in can_madv_dontneed_vma(vma) from __oom_reap_task_mm().
Is it safe for the OOM reaper to call tlb_gather_mmu()/unmap_page_range()/tlb_finish_mmu() sequence
after the OOM victim already completed tlb_gather_mmu()/unmap_vmas()/free_pgtables()/tlb_finish_mmu()/
remove_vma() sequence from exit_mmap() from __mmput() from mmput() from exit_mm() from do_exit() ?
I guess we need to prevent the OOM reaper from calling the sequence if the OOM victim already did
the sequence. And my patch did it via trial and error.
----------
unlock_oom:
mutex_unlock(&oom_lock);
26a: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
271: e8 00 00 00 00 callq 276 <__oom_reap_task_mm+0x56>
return ret;
}
276: 48 8b 55 d8 mov -0x28(%rbp),%rdx
27a: 65 48 33 14 25 28 00 xor %gs:0x28,%rdx
281: 00 00
283: 89 d8 mov %ebx,%eax
285: 75 10 jne 297 <__oom_reap_task_mm+0x77>
287: 48 81 c4 88 00 00 00 add $0x88,%rsp
28e: 5b pop %rbx
28f: 41 5c pop %r12
291: 41 5d pop %r13
293: 41 5e pop %r14
295: 5d pop %rbp
296: c3 retq
297: e8 00 00 00 00 callq 29c <__oom_reap_task_mm+0x7c>
*/
static __always_inline void
set_bit(long nr, volatile unsigned long *addr)
{
if (IS_IMMEDIATE(nr)) {
asm volatile(LOCK_PREFIX "orb %1,%0"
29c: f0 80 8b 7a 04 00 00 lock orb $0x40,0x47a(%rbx)
2a3: 40
* should imply barriers already and the reader would hit a page fault
* if it stumbled over a reaped memory.
*/
set_bit(MMF_UNSTABLE, &mm->flags);
tlb_gather_mmu(&tlb, mm, 0, -1);
2a4: 48 8d bd 58 ff ff ff lea -0xa8(%rbp),%rdi
2ab: 48 83 c9 ff or $0xffffffffffffffff,%rcx
2af: 31 d2 xor %edx,%edx
2b1: 48 89 de mov %rbx,%rsi
2b4: e8 00 00 00 00 callq 2b9 <__oom_reap_task_mm+0x99>
for (vma = mm->mmap ; vma; vma = vma->vm_next) {
2b9: 4c 8b 33 mov (%rbx),%r14
2bc: 4d 85 f6 test %r14,%r14
2bf: 74 3b je 2fc <__oom_reap_task_mm+0xdc>
static DEFINE_SPINLOCK(oom_reaper_lock);
static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
{
struct mmu_gather tlb;
struct vm_area_struct *vma;
2c1: 49 8b 46 50 mov 0x50(%r14),%rax
*/
set_bit(MMF_UNSTABLE, &mm->flags);
tlb_gather_mmu(&tlb, mm, 0, -1);
for (vma = mm->mmap ; vma; vma = vma->vm_next) {
if (!can_madv_dontneed_vma(vma))
2c5: a9 00 24 40 00 test $0x402400,%eax
2ca: 75 27 jne 2f3 <__oom_reap_task_mm+0xd3>
* We do not even care about fs backed pages because all
* which are reclaimable have already been reclaimed and
* we do not want to block exit_mmap by keeping mm ref
* count elevated without a good reason.
*/
if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED))
2cc: 49 83 be 90 00 00 00 cmpq $0x0,0x90(%r14)
2d3: 00
2d4: 74 04 je 2da <__oom_reap_task_mm+0xba>
2d6: a8 08 test $0x8,%al
2d8: 75 19 jne 2f3 <__oom_reap_task_mm+0xd3>
unmap_page_range(&tlb, vma, vma->vm_start, vma->vm_end,
2da: 49 8b 4e 08 mov 0x8(%r14),%rcx
2de: 49 8b 16 mov (%r14),%rdx
2e1: 48 8d bd 58 ff ff ff lea -0xa8(%rbp),%rdi
2e8: 45 31 c0 xor %r8d,%r8d
2eb: 4c 89 f6 mov %r14,%rsi
2ee: e8 00 00 00 00 callq 2f3 <__oom_reap_task_mm+0xd3>
* if it stumbled over a reaped memory.
----------
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2017-06-16 14:30 +0200 |
| Subject | Re: [patch] mm, oom: prevent additional oom kills before memory is freed |
| Message-ID | <tSYxY-8nq-25@gated-at.bofh.it> |
| In reply to | #1667294 |
Michal Hocko wrote:
> OK, could you play with the patch/idea suggested in
> http://lkml.kernel.org/r/20170615122031.GL1486@dhcp22.suse.cz?
I think we don't need to worry about mmap_sem dependency inside __mmput().
Since the OOM killer checks for !MMF_OOM_SKIP mm rather than TIF_MEMDIE thread,
we can keep the OOM killer disabled until we set MMF_OOM_SKIP to the victim's mm.
That is, elevating mm_users throughout the reaping procedure does not cause
premature victim selection, even after TIF_MEMDIE is cleared from the victim's
thread. Then, we don't need to use down_write()/up_write() for non OOM victim's mm
(nearly 100% of exit_mmap() calls), and can force partial reaping of OOM victim's mm
(nearly 0% of exit_mmap() calls) before __mmput() starts doing exit_aio() etc.
Patch is shown below. Only compile tested.
include/linux/sched/coredump.h | 1 +
mm/oom_kill.c | 80 ++++++++++++++++++++----------------------
2 files changed, 40 insertions(+), 41 deletions(-)
diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
index 98ae0d0..6b6237b 100644
--- a/include/linux/sched/coredump.h
+++ b/include/linux/sched/coredump.h
@@ -62,6 +62,7 @@ static inline int get_dumpable(struct mm_struct *mm)
* on NFS restore
*/
//#define MMF_EXE_FILE_CHANGED 18 /* see prctl_set_mm_exe_file() */
+#define MMF_OOM_REAPING 18 /* mm is supposed to be reaped */
#define MMF_HAS_UPROBES 19 /* has uprobes */
#define MMF_RECALC_UPROBES 20 /* MMF_HAS_UPROBES can be wrong */
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 0e2c925..bdcf658 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -470,38 +470,9 @@ 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;
- goto unlock_oom;
- }
-
- /*
- * 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)) {
- up_read(&mm->mmap_sem);
- goto unlock_oom;
- }
+ if (!down_read_trylock(&mm->mmap_sem))
+ return false;
/*
* Tell all users of get_user/copy_from_user etc... that the content
@@ -537,16 +508,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
K(get_mm_counter(mm, MM_FILEPAGES)),
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);
-unlock_oom:
- mutex_unlock(&oom_lock);
- return ret;
+ return true;
}
#define MAX_OOM_REAP_RETRIES 10
@@ -573,9 +535,32 @@ static void oom_reap_task(struct task_struct *tsk)
/*
* Hide this mm from OOM killer because it has been either reaped or
* somebody can't call up_write(mmap_sem).
+ *
+ * Serialize setting of MMF_OOM_SKIP using oom_lock in order to
+ * avoid race with select_bad_process() which causes premature
+ * new oom victim selection.
+ *
+ * The OOM reaper: An allocating task:
+ * Failed get_page_from_freelist().
+ * Enters into out_of_memory().
+ * Reaped memory enough to make get_page_from_freelist() succeed.
+ * Sets MMF_OOM_SKIP to mm.
+ * Enters into select_bad_process().
+ * # MMF_OOM_SKIP mm selects new victim.
*/
+ mutex_lock(&oom_lock);
set_bit(MMF_OOM_SKIP, &mm->flags);
+ mutex_unlock(&oom_lock);
+ /*
+ * 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.
+ */
+ if (test_bit(MMF_OOM_REAPING, &mm->flags)) {
+ clear_bit(MMF_OOM_REAPING, &mm->flags);
+ mmput_async(mm);
+ }
/* Drop a reference taken by wake_oom_reaper */
put_task_struct(tsk);
}
@@ -658,6 +643,13 @@ static void mark_oom_victim(struct task_struct *tsk)
if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm))
mmgrab(tsk->signal->oom_mm);
+#ifdef CONFIG_MMU
+ if (!test_bit(MMF_OOM_REAPING, &mm->flags)) {
+ set_bit(MMF_OOM_REAPING, &mm->flags);
+ mmget(mm);
+ }
+#endif
+
/*
* Make sure that the task is woken up from uninterruptible sleep
* if it is frozen because OOM killer wouldn't be able to free
@@ -913,6 +905,12 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
if (is_global_init(p)) {
can_oom_reap = false;
set_bit(MMF_OOM_SKIP, &mm->flags);
+#ifdef CONFIG_MMU
+ if (test_bit(MMF_OOM_REAPING, &mm->flags)) {
+ clear_bit(MMF_OOM_REAPING, &mm->flags);
+ mmput_async(mm);
+ }
+#endif
pr_info("oom killer %d (%s) has mm pinned by %d (%s)\n",
task_pid_nr(victim), victim->comm,
task_pid_nr(p), p->comm);
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2017-06-16 16:20 +0200 |
| Message-ID | <tT0gq-1a4-9@gated-at.bofh.it> |
| In reply to | #1667711 |
On Fri 16-06-17 21:22:20, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > OK, could you play with the patch/idea suggested in
> > http://lkml.kernel.org/r/20170615122031.GL1486@dhcp22.suse.cz?
>
> I think we don't need to worry about mmap_sem dependency inside __mmput().
> Since the OOM killer checks for !MMF_OOM_SKIP mm rather than TIF_MEMDIE thread,
> we can keep the OOM killer disabled until we set MMF_OOM_SKIP to the victim's mm.
> That is, elevating mm_users throughout the reaping procedure does not cause
> premature victim selection, even after TIF_MEMDIE is cleared from the victim's
> thread. Then, we don't need to use down_write()/up_write() for non OOM victim's mm
> (nearly 100% of exit_mmap() calls), and can force partial reaping of OOM victim's mm
> (nearly 0% of exit_mmap() calls) before __mmput() starts doing exit_aio() etc.
> Patch is shown below. Only compile tested.
Yes, that would be another approach.
> include/linux/sched/coredump.h | 1 +
> mm/oom_kill.c | 80 ++++++++++++++++++++----------------------
> 2 files changed, 40 insertions(+), 41 deletions(-)
>
> diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
> index 98ae0d0..6b6237b 100644
> --- a/include/linux/sched/coredump.h
> +++ b/include/linux/sched/coredump.h
> @@ -62,6 +62,7 @@ static inline int get_dumpable(struct mm_struct *mm)
> * on NFS restore
> */
> //#define MMF_EXE_FILE_CHANGED 18 /* see prctl_set_mm_exe_file() */
> +#define MMF_OOM_REAPING 18 /* mm is supposed to be reaped */
A new flag is not really needed. We can increase it for _each_ reapable
oom victim.
> @@ -658,6 +643,13 @@ static void mark_oom_victim(struct task_struct *tsk)
> if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm))
> mmgrab(tsk->signal->oom_mm);
>
> +#ifdef CONFIG_MMU
> + if (!test_bit(MMF_OOM_REAPING, &mm->flags)) {
> + set_bit(MMF_OOM_REAPING, &mm->flags);
> + mmget(mm);
> + }
> +#endif
This would really need a big fat warning explaining why we do not need
mmget_not_zero. We rely on exit_mm doing both mmput and tsk->mm = NULL
under the task_lock and mark_oom_victim is called under this lock as
well and task_will_free_mem resp. find_lock_task_mm makes sure we do not
even consider tasks wihout mm.
I agree that a solution which is fully contained inside the oom proper
would be preferable to touching __mmput path.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> |
|---|---|
| Date | 2017-06-17 07:20 +0200 |
| Subject | [PATCH] mm,oom_kill: Close race window of needlessly selecting new victims. |
| Message-ID | <tTejo-2oe-11@gated-at.bofh.it> |
| In reply to | #1667780 |
Michal Hocko wrote:
> On Fri 16-06-17 21:22:20, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > OK, could you play with the patch/idea suggested in
> > > http://lkml.kernel.org/r/20170615122031.GL1486@dhcp22.suse.cz?
> >
> > I think we don't need to worry about mmap_sem dependency inside __mmput().
> > Since the OOM killer checks for !MMF_OOM_SKIP mm rather than TIF_MEMDIE thread,
> > we can keep the OOM killer disabled until we set MMF_OOM_SKIP to the victim's mm.
> > That is, elevating mm_users throughout the reaping procedure does not cause
> > premature victim selection, even after TIF_MEMDIE is cleared from the victim's
> > thread. Then, we don't need to use down_write()/up_write() for non OOM victim's mm
> > (nearly 100% of exit_mmap() calls), and can force partial reaping of OOM victim's mm
> > (nearly 0% of exit_mmap() calls) before __mmput() starts doing exit_aio() etc.
> > Patch is shown below. Only compile tested.
>
> Yes, that would be another approach.
>
> > include/linux/sched/coredump.h | 1 +
> > mm/oom_kill.c | 80 ++++++++++++++++++++----------------------
> > 2 files changed, 40 insertions(+), 41 deletions(-)
> >
> > diff --git a/include/linux/sched/coredump.h b/include/linux/sched/coredump.h
> > index 98ae0d0..6b6237b 100644
> > --- a/include/linux/sched/coredump.h
> > +++ b/include/linux/sched/coredump.h
> > @@ -62,6 +62,7 @@ static inline int get_dumpable(struct mm_struct *mm)
> > * on NFS restore
> > */
> > //#define MMF_EXE_FILE_CHANGED 18 /* see prctl_set_mm_exe_file() */
> > +#define MMF_OOM_REAPING 18 /* mm is supposed to be reaped */
>
> A new flag is not really needed. We can increase it for _each_ reapable
> oom victim.
Yes if based on an assumption that number of mark_oom_victim() calls and
wake_oom_reaper() calls matches...
>
> > @@ -658,6 +643,13 @@ static void mark_oom_victim(struct task_struct *tsk)
> > if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm))
> > mmgrab(tsk->signal->oom_mm);
> >
> > +#ifdef CONFIG_MMU
> > + if (!test_bit(MMF_OOM_REAPING, &mm->flags)) {
> > + set_bit(MMF_OOM_REAPING, &mm->flags);
> > + mmget(mm);
> > + }
> > +#endif
>
> This would really need a big fat warning explaining why we do not need
> mmget_not_zero. We rely on exit_mm doing both mmput and tsk->mm = NULL
> under the task_lock and mark_oom_victim is called under this lock as
> well and task_will_free_mem resp. find_lock_task_mm makes sure we do not
> even consider tasks wihout mm.
>
> I agree that a solution which is fully contained inside the oom proper
> would be preferable to touching __mmput path.
OK. Updated patch shown below.
----------------------------------------
>From 5ed8922bd281456793408328c8b27899ebdd298b Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Sat, 17 Jun 2017 14:04:09 +0900
Subject: [PATCH] mm,oom_kill: Close race window of needlessly selecting new
victims.
David Rientjes has reported that the OOM killer can select next OOM victim
when existing OOM victims called __mmput() before the OOM reaper starts
trying to unmap pages. In his testing, 4.12-rc kernels are killing 1-4
processes unnecessarily for each OOM condition.
----------
One oom kill shows the system to be oom:
[22999.488705] Node 0 Normal free:90484kB min:90500kB ...
[22999.488711] Node 1 Normal free:91536kB min:91948kB ...
followed up by one or more unnecessary oom kills showing the oom killer
racing with memory freeing of the victim:
[22999.510329] Node 0 Normal free:229588kB min:90500kB ...
[22999.510334] Node 1 Normal free:600036kB min:91948kB ...
----------
This is because commit e5e3f4c4f0e95ecb ("mm, oom_reaper: make sure that
mmput_async is called only when memory was reaped") kept not to set
MMF_OOM_REAPED flag when the OOM reaper found that mm_users == 0 but then
commit 26db62f179d112d3 ("oom: keep mm of the killed task available") by
error changed to always set MMF_OOM_REAPED flag. As a result, MMF_OOM_SKIP
flag is immediately set without waiting for __mmput() because __mmput()
might get stuck before setting MMF_OOM_SKIP flag, and led to above report.
A workaround is to let the OOM reaper wait for a while and give up via
timeout as if the OOM reaper was unable to take mmap_sem for read. But
we want to avoid timeout based approach if possible. Therefore, this
patch takes a different approach.
This patch elevates mm_users of an OOM victim's mm, and prevents the OOM
victim from calling __mmput() before the OOM reaper starts trying to unmap
pages. In this way, we can force the OOM reaper to try to reclaim some
memory before setting MMF_OOM_SKIP flag.
Since commit 862e3073b3eed13f ("mm, oom: get rid of
signal_struct::oom_victims") changed to keep the OOM killer disabled
until MMF_OOM_SKIP is set on the victim's mm rather than until TIF_MEMDIE
is cleared from the victim's thread, we can keep the OOM killer disabled
until __oom_reap_task_mm() or __mmput() reclaims some memory and sets
MMF_OOM_SKIP flag. That is, elevating mm_users throughout the reaping
procedure does not cause premature victim selection.
This patch also reduces the range of oom_lock protection in
__oom_reap_task_mm() introduced by commit e2fe14564d3316d1 ("oom_reaper:
close race with exiting task"). Since the OOM killer is kept disabled until
MMF_OOM_SKIP flag is set, we don't need to serialize throughout the reaping
procedure; serializing only setting MMF_OOM_SKIP flag is enough.
This allows the OOM reaper to start reclaiming memory as soon as
wake_oom_reaper() is called by the OOM killer in order to compensate for
delaying exit_mmap() call caused by elevating mm_users of the OOM victim's
mm when the OOM victim can exit smoothly, for currently the OOM killer
calls schedule_timeout_killable(1) at out_of_memory() with oom_lock held.
This might also allow direct OOM reaping (i.e. let the OOM killer call
__oom_reap_task_mm() because we would have 16KB kernel stack with stack
overflow detection) and replace the OOM reaper kernel thread with a
workqueue (i.e. save one kernel thread).
Reported-by: David Rientjes <rientjes@google.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Fixes: 26db62f179d112d3 ("oom: keep mm of the killed task available")
Cc: Michal Hocko <mhocko@suse.com>
---
mm/oom_kill.c | 94 ++++++++++++++++++++++++++++++++---------------------------
1 file changed, 51 insertions(+), 43 deletions(-)
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 04c9143..cf1d331 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -470,38 +470,9 @@ 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;
- goto unlock_oom;
- }
- /*
- * 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)) {
- up_read(&mm->mmap_sem);
- goto unlock_oom;
- }
+ if (!down_read_trylock(&mm->mmap_sem))
+ return false;
/*
* Tell all users of get_user/copy_from_user etc... that the content
@@ -537,16 +508,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
K(get_mm_counter(mm, MM_FILEPAGES)),
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);
-unlock_oom:
- mutex_unlock(&oom_lock);
- return ret;
+ return true;
}
#define MAX_OOM_REAP_RETRIES 10
@@ -569,12 +531,31 @@ static void oom_reap_task(struct task_struct *tsk)
done:
tsk->oom_reaper_list = NULL;
+ /*
+ * Drop a mm_users reference taken by mark_oom_victim().
+ * A mm_count reference taken by mark_oom_victim() remains.
+ */
+ mmput_async(mm);
/*
* Hide this mm from OOM killer because it has been either reaped or
* somebody can't call up_write(mmap_sem).
+ *
+ * Serialize setting of MMF_OOM_SKIP using oom_lock in order to
+ * avoid race with select_bad_process() which causes premature
+ * new oom victim selection.
+ *
+ * The OOM reaper: An allocating task:
+ * Failed get_page_from_freelist().
+ * Enters into out_of_memory().
+ * Reaped memory enough to make get_page_from_freelist() succeed.
+ * Sets MMF_OOM_SKIP to mm.
+ * Enters into select_bad_process().
+ * # MMF_OOM_SKIP mm selects new victim.
*/
+ mutex_lock(&oom_lock);
set_bit(MMF_OOM_SKIP, &mm->flags);
+ mutex_unlock(&oom_lock);
/* Drop a reference taken by wake_oom_reaper */
put_task_struct(tsk);
@@ -602,12 +583,16 @@ static int oom_reaper(void *unused)
static void wake_oom_reaper(struct task_struct *tsk)
{
- if (!oom_reaper_th)
+ if (!oom_reaper_th) {
+ mmput_async(tsk->signal->oom_mm);
return;
+ }
/* tsk is already queued? */
- if (tsk == oom_reaper_list || tsk->oom_reaper_list)
+ if (tsk == oom_reaper_list || tsk->oom_reaper_list) {
+ mmput_async(tsk->signal->oom_mm);
return;
+ }
get_task_struct(tsk);
@@ -650,12 +635,32 @@ static void mark_oom_victim(struct task_struct *tsk)
struct mm_struct *mm = tsk->mm;
WARN_ON(oom_killer_disabled);
+#ifdef CONFIG_MMU
+ /*
+ * Take a mm_users reference so that __oom_reap_task_mm() can unmap
+ * pages without risking a race condition where final mmput() from
+ * exit_mm() from do_exit() triggered __mmput() and gets stuck there
+ * (but __oom_reap_task_mm() cannot unmap pages due to mm_users == 0).
+ *
+ * Since all callers guarantee that this mm is stable (hold task_lock
+ * or task is current), we can safely use mmget() here.
+ *
+ * When dropping this reference, mmput_async() has to be used because
+ * __mmput() can get stuck which in turn keeps the OOM killer/reaper
+ * disabled forever.
+ */
+ mmget(mm);
+#endif
/* OOM killer might race with memcg OOM */
if (test_and_set_tsk_thread_flag(tsk, TIF_MEMDIE))
return;
/* oom_mm is bound to the signal struct life time. */
if (!cmpxchg(&tsk->signal->oom_mm, NULL, mm))
+ /*
+ * Take a mm_count reference so that we can examine flags value
+ * when tsk_is_oom_victim() is true.
+ */
mmgrab(tsk->signal->oom_mm);
/*
@@ -908,6 +913,9 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
if (is_global_init(p)) {
can_oom_reap = false;
set_bit(MMF_OOM_SKIP, &mm->flags);
+#ifdef CONFIG_MMU
+ mmput_async(mm);
+#endif
pr_info("oom killer %d (%s) has mm pinned by %d (%s)\n",
task_pid_nr(victim), victim->comm,
task_pid_nr(p), p->comm);
--
1.8.3.1
[toc] | [prev] | [standalone]
Page 2 of 2 — ← Prev page 1 [2]
Back to top | Article view | linux.kernel
csiph-web