[PATCH 1/4] mm: drop zap_details::ignore_dirty

[PATCH 1/4] mm: drop zap_details::ignore_dirty

[Kirill A. Shutemov]

The only user of ignore_dirty is oom-reaper. But it doesn't really use
it.

ignore_dirty only has effect on file pages mapped with dirty pte.
But oom-repear skips shared VMAs, so there's no way we can dirty file
pte in them.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 include/linux/mm.h | 1 -
 mm/memory.c        | 6 ------
 mm/oom_kill.c      | 3 +--
 3 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 4424784ac374..7b8e425ac41c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1148,7 +1148,6 @@ struct zap_details {
 	struct address_space *check_mapping;	/* Check page->mapping if set */
 	pgoff_t	first_index;			/* Lowest page->index to unmap */
 	pgoff_t last_index;			/* Highest page->index to unmap */
-	bool ignore_dirty;			/* Ignore dirty pages */
 	bool check_swap_entries;		/* Check also swap entries */
 };
 
diff --git a/mm/memory.c b/mm/memory.c
index 455c3e628d52..6ac8fa56080f 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1155,12 +1155,6 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
 
 			if (!PageAnon(page)) {
 				if (pte_dirty(ptent)) {
-					/*
-					 * oom_reaper cannot tear down dirty
-					 * pages
-					 */
-					if (unlikely(details && details->ignore_dirty))
-						continue;
 					force_flush = 1;
 					set_page_dirty(page);
 				}
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index ec9f11d4f094..f101db68e760 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -465,8 +465,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 {
 	struct mmu_gather tlb;
 	struct vm_area_struct *vma;
-	struct zap_details details = {.check_swap_entries = true,
-				      .ignore_dirty = true};
+	struct zap_details details = {.check_swap_entries = true};
 	bool ret = true;
 
 	/*
-- 
2.10.2

[PATCH 2/4] mm: drop zap_details::check_swap_entries

[Kirill A. Shutemov]

detail == NULL would give the same functionality as
.check_swap_entries==true.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 include/linux/mm.h | 1 -
 mm/memory.c        | 4 ++--
 mm/oom_kill.c      | 3 +--
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7b8e425ac41c..5f6bea4c9d41 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1148,7 +1148,6 @@ struct zap_details {
 	struct address_space *check_mapping;	/* Check page->mapping if set */
 	pgoff_t	first_index;			/* Lowest page->index to unmap */
 	pgoff_t last_index;			/* Highest page->index to unmap */
-	bool check_swap_entries;		/* Check also swap entries */
 };
 
 struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
diff --git a/mm/memory.c b/mm/memory.c
index 6ac8fa56080f..c03b18f13619 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1173,8 +1173,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
 			}
 			continue;
 		}
-		/* only check swap_entries if explicitly asked for in details */
-		if (unlikely(details && !details->check_swap_entries))
+		/* If details->check_mapping, we leave swap entries. */
+		if (unlikely(details))
 			continue;
 
 		entry = pte_to_swp_entry(ptent);
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index f101db68e760..96a53ab0c9eb 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -465,7 +465,6 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 {
 	struct mmu_gather tlb;
 	struct vm_area_struct *vma;
-	struct zap_details details = {.check_swap_entries = true};
 	bool ret = true;
 
 	/*
@@ -531,7 +530,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 		 */
 		if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED))
 			unmap_page_range(&tlb, vma, vma->vm_start, vma->vm_end,
-					 &details);
+					 NULL);
 	}
 	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

[PATCH 3/4] mm: drop unused argument of zap_page_range()

[Kirill A. Shutemov]

There's no users of zap_page_range() who wants non-NULL 'details'.
Let's drop it.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/s390/mm/gmap.c               | 2 +-
 arch/x86/mm/mpx.c                 | 2 +-
 drivers/android/binder.c          | 2 +-
 drivers/staging/android/ion/ion.c | 3 +--
 include/linux/mm.h                | 2 +-
 mm/madvise.c                      | 2 +-
 mm/memory.c                       | 4 ++--
 7 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index ec1f0dedb948..59ac93714fa4 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -687,7 +687,7 @@ void gmap_discard(struct gmap *gmap, unsigned long from, unsigned long to)
 		/* Find vma in the parent mm */
 		vma = find_vma(gmap->mm, vmaddr);
 		size = min(to - gaddr, PMD_SIZE - (gaddr & ~PMD_MASK));
-		zap_page_range(vma, vmaddr, size, NULL);
+		zap_page_range(vma, vmaddr, size);
 	}
 	up_read(&gmap->mm->mmap_sem);
 }
diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
index e4f800999b32..4bfb31e79d5d 100644
--- a/arch/x86/mm/mpx.c
+++ b/arch/x86/mm/mpx.c
@@ -796,7 +796,7 @@ static noinline int zap_bt_entries_mapping(struct mm_struct *mm,
 			return -EINVAL;
 
 		len = min(vma->vm_end, end) - addr;
-		zap_page_range(vma, addr, len, NULL);
+		zap_page_range(vma, addr, len);
 		trace_mpx_unmap_zap(addr, addr+len);
 
 		vma = vma->vm_next;
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 3c71b982bf2a..d97f6725cf8c 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -629,7 +629,7 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
 		page = &proc->pages[(page_addr - proc->buffer) / PAGE_SIZE];
 		if (vma)
 			zap_page_range(vma, (uintptr_t)page_addr +
-				proc->user_buffer_offset, PAGE_SIZE, NULL);
+				proc->user_buffer_offset, PAGE_SIZE);
 err_vm_insert_page_failed:
 		unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE);
 err_map_kernel_failed:
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index b653451843c8..0fb0e28ace70 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -865,8 +865,7 @@ static void ion_buffer_sync_for_device(struct ion_buffer *buffer,
 	list_for_each_entry(vma_list, &buffer->vmas, list) {
 		struct vm_area_struct *vma = vma_list->vma;
 
-		zap_page_range(vma, vma->vm_start, vma->vm_end - vma->vm_start,
-			       NULL);
+		zap_page_range(vma, vma->vm_start, vma->vm_end - vma->vm_start);
 	}
 	mutex_unlock(&buffer->lock);
 }
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5f6bea4c9d41..92dcada8caaf 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1158,7 +1158,7 @@ struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr,
 int zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
 		unsigned long size);
 void zap_page_range(struct vm_area_struct *vma, unsigned long address,
-		unsigned long size, struct zap_details *);
+		unsigned long size);
 void unmap_vmas(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
 		unsigned long start, unsigned long end);
 
diff --git a/mm/madvise.c b/mm/madvise.c
index 0e3828eae9f8..aa4c502caecb 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -476,7 +476,7 @@ static long madvise_dontneed(struct vm_area_struct *vma,
 	if (vma->vm_flags & (VM_LOCKED|VM_HUGETLB|VM_PFNMAP))
 		return -EINVAL;
 
-	zap_page_range(vma, start, end - start, NULL);
+	zap_page_range(vma, start, end - start);
 	return 0;
 }
 
diff --git a/mm/memory.c b/mm/memory.c
index c03b18f13619..eed102070dcb 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1375,7 +1375,7 @@ void unmap_vmas(struct mmu_gather *tlb,
  * Caller must protect the VMA list
  */
 void zap_page_range(struct vm_area_struct *vma, unsigned long start,
-		unsigned long size, struct zap_details *details)
+		unsigned long size)
 {
 	struct mm_struct *mm = vma->vm_mm;
 	struct mmu_gather tlb;
@@ -1386,7 +1386,7 @@ void zap_page_range(struct vm_area_struct *vma, unsigned long start,
 	update_hiwater_rss(mm);
 	mmu_notifier_invalidate_range_start(mm, start, end);
 	for ( ; vma && vma->vm_start < end; vma = vma->vm_next)
-		unmap_single_vma(&tlb, vma, start, end, details);
+		unmap_single_vma(&tlb, vma, start, end, NULL);
 	mmu_notifier_invalidate_range_end(mm, start, end);
 	tlb_finish_mmu(&tlb, start, end);
 }
-- 
2.10.2

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

[Kirill A. Shutemov]

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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

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: i386-randconfig-s0-201650 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

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

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28732 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 4737 bytes --]

Re: [PATCH 3/4] mm: drop unused argument of zap_page_range()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4269 bytes --]

Hi Kirill,

[auto build test WARNING on mmotm/master]
[also build test WARNING 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
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   lib/crc32.c:148: warning: No description found for parameter 'tab)[256]'
   lib/crc32.c:148: warning: Excess function parameter 'tab' description in 'crc32_le_generic'
   lib/crc32.c:293: warning: No description found for parameter 'tab)[256]'
   lib/crc32.c:293: warning: Excess function parameter 'tab' description in 'crc32_be_generic'
   lib/crc32.c:1: warning: no structured comments found
   lib/idr.c:223: warning: No description found for parameter 'start'
   lib/idr.c:223: warning: No description found for parameter 'id'
   lib/idr.c:223: warning: Excess function parameter 'starting_id' description in 'ida_get_new_above'
   lib/idr.c:223: warning: Excess function parameter 'p_id' description in 'ida_get_new_above'
   lib/idr.c:1: warning: no structured comments found
       Was looking for 'IDA description'.
   lib/idr.c:223: warning: No description found for parameter 'start'
   lib/idr.c:223: warning: No description found for parameter 'id'
   lib/idr.c:223: warning: Excess function parameter 'starting_id' description in 'ida_get_new_above'
   lib/idr.c:223: warning: Excess function parameter 'p_id' description in 'ida_get_new_above'
>> mm/memory.c:1379: warning: Excess function parameter 'details' description in 'zap_page_range'
   drivers/pci/msi.c:623: warning: No description found for parameter 'affd'
   drivers/pci/msi.c:623: warning: Excess function parameter 'affinity' description in 'msi_capability_init'

vim +1379 mm/memory.c

f5cc4eef9 Al Viro            2012-03-05  1363  	for ( ; vma && vma->vm_start < end_addr; vma = vma->vm_next)
4f74d2c8e Linus Torvalds     2012-05-06  1364  		unmap_single_vma(tlb, vma, start_addr, end_addr, NULL);
cddb8a5c1 Andrea Arcangeli   2008-07-28  1365  	mmu_notifier_invalidate_range_end(mm, start_addr, end_addr);
^1da177e4 Linus Torvalds     2005-04-16  1366  }
^1da177e4 Linus Torvalds     2005-04-16  1367  
^1da177e4 Linus Torvalds     2005-04-16  1368  /**
^1da177e4 Linus Torvalds     2005-04-16  1369   * zap_page_range - remove user pages in a given range
^1da177e4 Linus Torvalds     2005-04-16  1370   * @vma: vm_area_struct holding the applicable pages
eb4546bbb Randy Dunlap       2012-06-20  1371   * @start: starting address of pages to zap
^1da177e4 Linus Torvalds     2005-04-16  1372   * @size: number of bytes to zap
8a5f14a23 Kirill A. Shutemov 2015-02-10  1373   * @details: details of shared cache invalidation
f5cc4eef9 Al Viro            2012-03-05  1374   *
f5cc4eef9 Al Viro            2012-03-05  1375   * Caller must protect the VMA list
^1da177e4 Linus Torvalds     2005-04-16  1376   */
7e027b14d Linus Torvalds     2012-05-06  1377  void zap_page_range(struct vm_area_struct *vma, unsigned long start,
1ddef4086 Kirill A. Shutemov 2016-12-16  1378  		unsigned long size)
^1da177e4 Linus Torvalds     2005-04-16 @1379  {
^1da177e4 Linus Torvalds     2005-04-16  1380  	struct mm_struct *mm = vma->vm_mm;
d16dfc550 Peter Zijlstra     2011-05-24  1381  	struct mmu_gather tlb;
7e027b14d Linus Torvalds     2012-05-06  1382  	unsigned long end = start + size;
^1da177e4 Linus Torvalds     2005-04-16  1383  
^1da177e4 Linus Torvalds     2005-04-16  1384  	lru_add_drain();
2b047252d Linus Torvalds     2013-08-15  1385  	tlb_gather_mmu(&tlb, mm, start, end);
365e9c87a Hugh Dickins       2005-10-29  1386  	update_hiwater_rss(mm);
7e027b14d Linus Torvalds     2012-05-06  1387  	mmu_notifier_invalidate_range_start(mm, start, end);

:::::: The code at line 1379 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6474 bytes --]

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

[Tetsuo Handa]

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.

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

[Michal Hocko]

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

Re: [PATCH 1/4] mm: drop zap_details::ignore_dirty

[Michal Hocko]

On Fri 16-12-16 17:15:53, Kirill A. Shutemov wrote:
> The only user of ignore_dirty is oom-reaper. But it doesn't really use
> it.
> 
> ignore_dirty only has effect on file pages mapped with dirty pte.
> But oom-repear skips shared VMAs, so there's no way we can dirty file
> pte in them.

Hmm, I am trying to rememeber why I've done that and it seems I was just
too paranoid and wanted to make sure that we never touch dirty mapped
pages. As you say this is not possible with the current implementation
so the patch should be OK.

> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  include/linux/mm.h | 1 -
>  mm/memory.c        | 6 ------
>  mm/oom_kill.c      | 3 +--
>  3 files changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 4424784ac374..7b8e425ac41c 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1148,7 +1148,6 @@ struct zap_details {
>  	struct address_space *check_mapping;	/* Check page->mapping if set */
>  	pgoff_t	first_index;			/* Lowest page->index to unmap */
>  	pgoff_t last_index;			/* Highest page->index to unmap */
> -	bool ignore_dirty;			/* Ignore dirty pages */
>  	bool check_swap_entries;		/* Check also swap entries */
>  };
>  
> diff --git a/mm/memory.c b/mm/memory.c
> index 455c3e628d52..6ac8fa56080f 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1155,12 +1155,6 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
>  
>  			if (!PageAnon(page)) {
>  				if (pte_dirty(ptent)) {
> -					/*
> -					 * oom_reaper cannot tear down dirty
> -					 * pages
> -					 */
> -					if (unlikely(details && details->ignore_dirty))
> -						continue;
>  					force_flush = 1;
>  					set_page_dirty(page);
>  				}
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index ec9f11d4f094..f101db68e760 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -465,8 +465,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
>  {
>  	struct mmu_gather tlb;
>  	struct vm_area_struct *vma;
> -	struct zap_details details = {.check_swap_entries = true,
> -				      .ignore_dirty = true};
> +	struct zap_details details = {.check_swap_entries = true};
>  	bool ret = true;
>  
>  	/*
> -- 
> 2.10.2
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/4] mm: drop zap_details::check_swap_entries

[Michal Hocko]

On Fri 16-12-16 17:15:54, Kirill A. Shutemov wrote:
> detail == NULL would give the same functionality as
> .check_swap_entries==true.

Yes, now that check_swap_entries is the only used flag from there we can
safely rely on detail == NULL check.

> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  include/linux/mm.h | 1 -
>  mm/memory.c        | 4 ++--
>  mm/oom_kill.c      | 3 +--
>  3 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7b8e425ac41c..5f6bea4c9d41 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1148,7 +1148,6 @@ struct zap_details {
>  	struct address_space *check_mapping;	/* Check page->mapping if set */
>  	pgoff_t	first_index;			/* Lowest page->index to unmap */
>  	pgoff_t last_index;			/* Highest page->index to unmap */
> -	bool check_swap_entries;		/* Check also swap entries */
>  };
>  
>  struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
> diff --git a/mm/memory.c b/mm/memory.c
> index 6ac8fa56080f..c03b18f13619 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1173,8 +1173,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
>  			}
>  			continue;
>  		}
> -		/* only check swap_entries if explicitly asked for in details */
> -		if (unlikely(details && !details->check_swap_entries))
> +		/* If details->check_mapping, we leave swap entries. */
> +		if (unlikely(details))
>  			continue;
>  
>  		entry = pte_to_swp_entry(ptent);
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index f101db68e760..96a53ab0c9eb 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -465,7 +465,6 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
>  {
>  	struct mmu_gather tlb;
>  	struct vm_area_struct *vma;
> -	struct zap_details details = {.check_swap_entries = true};
>  	bool ret = true;
>  
>  	/*
> @@ -531,7 +530,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
>  		 */
>  		if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED))
>  			unmap_page_range(&tlb, vma, vma->vm_start, vma->vm_end,
> -					 &details);
> +					 NULL);
>  	}
>  	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
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 3/4] mm: drop unused argument of zap_page_range()

[Michal Hocko]

On Fri 16-12-16 17:15:55, Kirill A. Shutemov wrote:
> There's no users of zap_page_range() who wants non-NULL 'details'.
> Let's drop it.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

kbuild robot has noticed that you should remove doc part as well
 * @details: details of shared cache invalidation

then you can add
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  arch/s390/mm/gmap.c               | 2 +-
>  arch/x86/mm/mpx.c                 | 2 +-
>  drivers/android/binder.c          | 2 +-
>  drivers/staging/android/ion/ion.c | 3 +--
>  include/linux/mm.h                | 2 +-
>  mm/madvise.c                      | 2 +-
>  mm/memory.c                       | 4 ++--
>  7 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index ec1f0dedb948..59ac93714fa4 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -687,7 +687,7 @@ void gmap_discard(struct gmap *gmap, unsigned long from, unsigned long to)
>  		/* Find vma in the parent mm */
>  		vma = find_vma(gmap->mm, vmaddr);
>  		size = min(to - gaddr, PMD_SIZE - (gaddr & ~PMD_MASK));
> -		zap_page_range(vma, vmaddr, size, NULL);
> +		zap_page_range(vma, vmaddr, size);
>  	}
>  	up_read(&gmap->mm->mmap_sem);
>  }
> diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
> index e4f800999b32..4bfb31e79d5d 100644
> --- a/arch/x86/mm/mpx.c
> +++ b/arch/x86/mm/mpx.c
> @@ -796,7 +796,7 @@ static noinline int zap_bt_entries_mapping(struct mm_struct *mm,
>  			return -EINVAL;
>  
>  		len = min(vma->vm_end, end) - addr;
> -		zap_page_range(vma, addr, len, NULL);
> +		zap_page_range(vma, addr, len);
>  		trace_mpx_unmap_zap(addr, addr+len);
>  
>  		vma = vma->vm_next;
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 3c71b982bf2a..d97f6725cf8c 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -629,7 +629,7 @@ static int binder_update_page_range(struct binder_proc *proc, int allocate,
>  		page = &proc->pages[(page_addr - proc->buffer) / PAGE_SIZE];
>  		if (vma)
>  			zap_page_range(vma, (uintptr_t)page_addr +
> -				proc->user_buffer_offset, PAGE_SIZE, NULL);
> +				proc->user_buffer_offset, PAGE_SIZE);
>  err_vm_insert_page_failed:
>  		unmap_kernel_range((unsigned long)page_addr, PAGE_SIZE);
>  err_map_kernel_failed:
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index b653451843c8..0fb0e28ace70 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -865,8 +865,7 @@ static void ion_buffer_sync_for_device(struct ion_buffer *buffer,
>  	list_for_each_entry(vma_list, &buffer->vmas, list) {
>  		struct vm_area_struct *vma = vma_list->vma;
>  
> -		zap_page_range(vma, vma->vm_start, vma->vm_end - vma->vm_start,
> -			       NULL);
> +		zap_page_range(vma, vma->vm_start, vma->vm_end - vma->vm_start);
>  	}
>  	mutex_unlock(&buffer->lock);
>  }
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 5f6bea4c9d41..92dcada8caaf 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1158,7 +1158,7 @@ struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr,
>  int zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
>  		unsigned long size);
>  void zap_page_range(struct vm_area_struct *vma, unsigned long address,
> -		unsigned long size, struct zap_details *);
> +		unsigned long size);
>  void unmap_vmas(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
>  		unsigned long start, unsigned long end);
>  
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 0e3828eae9f8..aa4c502caecb 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -476,7 +476,7 @@ static long madvise_dontneed(struct vm_area_struct *vma,
>  	if (vma->vm_flags & (VM_LOCKED|VM_HUGETLB|VM_PFNMAP))
>  		return -EINVAL;
>  
> -	zap_page_range(vma, start, end - start, NULL);
> +	zap_page_range(vma, start, end - start);
>  	return 0;
>  }
>  
> diff --git a/mm/memory.c b/mm/memory.c
> index c03b18f13619..eed102070dcb 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1375,7 +1375,7 @@ void unmap_vmas(struct mmu_gather *tlb,
>   * Caller must protect the VMA list
>   */
>  void zap_page_range(struct vm_area_struct *vma, unsigned long start,
> -		unsigned long size, struct zap_details *details)
> +		unsigned long size)
>  {
>  	struct mm_struct *mm = vma->vm_mm;
>  	struct mmu_gather tlb;
> @@ -1386,7 +1386,7 @@ void zap_page_range(struct vm_area_struct *vma, unsigned long start,
>  	update_hiwater_rss(mm);
>  	mmu_notifier_invalidate_range_start(mm, start, end);
>  	for ( ; vma && vma->vm_start < end; vma = vma->vm_next)
> -		unmap_single_vma(&tlb, vma, start, end, details);
> +		unmap_single_vma(&tlb, vma, start, end, NULL);
>  	mmu_notifier_invalidate_range_end(mm, start, end);
>  	tlb_finish_mmu(&tlb, start, end);
>  }
> -- 
> 2.10.2
> 

-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

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