Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1543480 > unrolled thread

[PATCH 4/4] oom-reaper: use madvise_dontneed() instead of unmap_page_range()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/4] oom-reaper: use madvise_dontneed() instead of unmap_page_range() "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2016-12-16 15:20 +0100
    Re: [PATCH 4/4] oom-reaper: use madvise_dontneed() instead of  unmap_page_range() kbuild test robot <lkp@intel.com> - 2016-12-16 17:50 +0100
    Re: [PATCH 4/4] oom-reaper: use madvise_dontneed() instead of  unmap_page_range() Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-12-19 12:50 +0100
      Re: [PATCH 4/4] oom-reaper: use madvise_dontneed() instead of  unmap_page_range() Michal Hocko <mhocko@kernel.org> - 2016-12-19 15:10 +0100
    Re: [PATCH 4/4] oom-reaper: use madvise_dontneed() instead of  unmap_page_range() Michal Hocko <mhocko@kernel.org> - 2016-12-19 15:40 +0100

#1543480 — [PATCH 4/4] oom-reaper: use madvise_dontneed() instead of unmap_page_range()

Logic on whether we can reap pages from the VMA should match what we
have in madvise_dontneed(). In particular, we should skip, VM_PFNMAP
VMAs, but we don't now.

Let's just call madvise_dontneed() from __oom_reap_task_mm(), so we
won't need to sync the logic in the future.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 mm/internal.h |  7 +++----
 mm/madvise.c  |  2 +-
 mm/memory.c   |  2 +-
 mm/oom_kill.c | 15 ++-------------
 4 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 44d68895a9b9..5c355855e4ad 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -41,10 +41,9 @@ int do_swap_page(struct vm_fault *vmf);
 void free_pgtables(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
 		unsigned long floor, unsigned long ceiling);
 
-void unmap_page_range(struct mmu_gather *tlb,
-			     struct vm_area_struct *vma,
-			     unsigned long addr, unsigned long end,
-			     struct zap_details *details);
+long madvise_dontneed(struct vm_area_struct *vma,
+			     struct vm_area_struct **prev,
+			     unsigned long start, unsigned long end);
 
 extern int __do_page_cache_readahead(struct address_space *mapping,
 		struct file *filp, pgoff_t offset, unsigned long nr_to_read,
diff --git a/mm/madvise.c b/mm/madvise.c
index aa4c502caecb..8c9f19b62b4a 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -468,7 +468,7 @@ static long madvise_free(struct vm_area_struct *vma,
  * An interface that causes the system to free clean pages and flush
  * dirty pages is already available as msync(MS_INVALIDATE).
  */
-static long madvise_dontneed(struct vm_area_struct *vma,
+long madvise_dontneed(struct vm_area_struct *vma,
 			     struct vm_area_struct **prev,
 			     unsigned long start, unsigned long end)
 {
diff --git a/mm/memory.c b/mm/memory.c
index eed102070dcb..f8836232a492 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1271,7 +1271,7 @@ static inline unsigned long zap_pud_range(struct mmu_gather *tlb,
 	return addr;
 }
 
-void unmap_page_range(struct mmu_gather *tlb,
+static void unmap_page_range(struct mmu_gather *tlb,
 			     struct vm_area_struct *vma,
 			     unsigned long addr, unsigned long end,
 			     struct zap_details *details)
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 96a53ab0c9eb..59a00b1c3145 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -479,7 +479,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 	 *				out_of_memory
 	 *				  select_bad_process
 	 *				    # no TIF_MEMDIE task selects new victim
-	 *  unmap_page_range # frees some memory
+	 *  madvise_dontneed # frees some memory
 	 */
 	mutex_lock(&oom_lock);
 
@@ -508,16 +508,6 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 
 	tlb_gather_mmu(&tlb, mm, 0, -1);
 	for (vma = mm->mmap ; vma; vma = vma->vm_next) {
-		if (is_vm_hugetlb_page(vma))
-			continue;
-
-		/*
-		 * mlocked VMAs require explicit munlocking before unmap.
-		 * Let's keep it simple here and skip such VMAs.
-		 */
-		if (vma->vm_flags & VM_LOCKED)
-			continue;
-
 		/*
 		 * Only anonymous pages have a good chance to be dropped
 		 * without additional steps which we cannot afford as we
@@ -529,8 +519,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 		 * count elevated without a good reason.
 		 */
 		if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED))
-			unmap_page_range(&tlb, vma, vma->vm_start, vma->vm_end,
-					 NULL);
+			madvise_dontneed(vma, &vma, vma->vm_start, vma->vm_end);
 	}
 	tlb_finish_mmu(&tlb, 0, -1);
 	pr_info("oom_reaper: reaped process %d (%s), now anon-rss:%lukB, file-rss:%lukB, shmem-rss:%lukB\n",
-- 
2.10.2

[toc] | [next] | [standalone]

#1543586 — Re: [PATCH 4/4] oom-reaper: use madvise_dontneed() instead of unmap_page_range()

[Multipart message — attachments visible in raw view] — view raw

Hi Kirill,

[auto build test ERROR on mmotm/master]
[also build test ERROR on v4.9 next-20161216]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kirill-A-Shutemov/mm-drop-zap_details-ignore_dirty/20161216-231509
base:   git://git.cmpxchg.org/linux-mmotm.git master
config: parisc-allnoconfig (attached as .config)
compiler: hppa-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=parisc 

All errors (new ones prefixed by >>):

   mm/built-in.o: In function `oom_reaper':
>> mm/oom_kill.o:(.text.oom_reaper+0x114): undefined reference to `madvise_dontneed'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1544478 — Re: [PATCH 4/4] oom-reaper: use madvise_dontneed() instead of unmap_page_range()

On 2016/12/16 23:15, Kirill A. Shutemov wrote:
> Logic on whether we can reap pages from the VMA should match what we
> have in madvise_dontneed(). In particular, we should skip, VM_PFNMAP
> VMAs, but we don't now.
> 
> Let's just call madvise_dontneed() from __oom_reap_task_mm(), so we
> won't need to sync the logic in the future.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> ---
>  mm/internal.h |  7 +++----
>  mm/madvise.c  |  2 +-
>  mm/memory.c   |  2 +-
>  mm/oom_kill.c | 15 ++-------------
>  4 files changed, 7 insertions(+), 19 deletions(-)

madvise_dontneed() calls zap_page_range().
zap_page_range() calls mmu_notifier_invalidate_range_start().
mmu_notifier_invalidate_range_start() calls __mmu_notifier_invalidate_range_start().
__mmu_notifier_invalidate_range_start() calls srcu_read_lock()/srcu_read_unlock().
This means that madvise_dontneed() might sleep.

I don't know what individual notifier will do, but for example

  static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
          .invalidate_range_start = i915_gem_userptr_mn_invalidate_range_start,
  };

i915_gem_userptr_mn_invalidate_range_start() calls flush_workqueue()
which means that we can OOM livelock if work item involves memory allocation.
Some of other notifiers call mutex_lock()/mutex_unlock().

Even if none of currently in-tree notifier users are blocked on memory
allocation, I think it is not guaranteed that future changes/users won't be
blocked on memory allocation.

[toc] | [prev] | [next] | [standalone]

#1544549 — Re: [PATCH 4/4] oom-reaper: use madvise_dontneed() instead of unmap_page_range()

On Mon 19-12-16 20:39:24, Tetsuo Handa wrote:
> On 2016/12/16 23:15, Kirill A. Shutemov wrote:
> > Logic on whether we can reap pages from the VMA should match what we
> > have in madvise_dontneed(). In particular, we should skip, VM_PFNMAP
> > VMAs, but we don't now.
> > 
> > Let's just call madvise_dontneed() from __oom_reap_task_mm(), so we
> > won't need to sync the logic in the future.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > ---
> >  mm/internal.h |  7 +++----
> >  mm/madvise.c  |  2 +-
> >  mm/memory.c   |  2 +-
> >  mm/oom_kill.c | 15 ++-------------
> >  4 files changed, 7 insertions(+), 19 deletions(-)
> 
> madvise_dontneed() calls zap_page_range().
> zap_page_range() calls mmu_notifier_invalidate_range_start().
> mmu_notifier_invalidate_range_start() calls __mmu_notifier_invalidate_range_start().
> __mmu_notifier_invalidate_range_start() calls srcu_read_lock()/srcu_read_unlock().
> This means that madvise_dontneed() might sleep.
> 
> I don't know what individual notifier will do, but for example
> 
>   static const struct mmu_notifier_ops i915_gem_userptr_notifier = {
>           .invalidate_range_start = i915_gem_userptr_mn_invalidate_range_start,
>   };
> 
> i915_gem_userptr_mn_invalidate_range_start() calls flush_workqueue()
> which means that we can OOM livelock if work item involves memory allocation.
> Some of other notifiers call mutex_lock()/mutex_unlock().
> 
> Even if none of currently in-tree notifier users are blocked on memory
> allocation, I think it is not guaranteed that future changes/users won't be
> blocked on memory allocation.

Yes I agree. The reason I didn't go with zap_page_range was that I
didn't want to rely on any external code path. Moreover I believe that
we even do not have to care about mmu notifiers. The task is dead and
nobody should be watching its address space. If somebody still does then
it would get SEGV anyway. Or am I missing something?

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1544575 — Re: [PATCH 4/4] oom-reaper: use madvise_dontneed() instead of unmap_page_range()

On Fri 16-12-16 17:15:56, Kirill A. Shutemov wrote:
> Logic on whether we can reap pages from the VMA should match what we
> have in madvise_dontneed(). In particular, we should skip, VM_PFNMAP
> VMAs, but we don't now.
> 
> Let's just call madvise_dontneed() from __oom_reap_task_mm(), so we
> won't need to sync the logic in the future.

I would rather extract those check into can_madv_dontneed_vma() and use
it in the oom reaper. I am really woried about notifier API which can
sleep or rely on locks or do whatever else.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web