[RFC v6 PATCH 0/2] mm: zap pages with read mmap_sem in munmap for large mapping

[RFC v6 PATCH 0/2] mm: zap pages with read mmap_sem in munmap for large mapping

[Yang Shi]

Background:
Recently, when we ran some vm scalability tests on machines with large memory,
we ran into a couple of mmap_sem scalability issues when unmapping large memory
space, please refer to https://lkml.org/lkml/2017/12/14/733 and
https://lkml.org/lkml/2018/2/20/576.


History:
Then akpm suggested to unmap large mapping section by section and drop mmap_sem
at a time to mitigate it (see https://lkml.org/lkml/2018/3/6/784).

V1 patch series was submitted to the mailing list per Andrew's suggestion
(see https://lkml.org/lkml/2018/3/20/786). Then I received a lot great feedback
and suggestions.

Then this topic was discussed on LSFMM summit 2018. In the summit, Michal Hocko
suggested (also in the v1 patches review) to try "two phases" approach. Zapping
pages with read mmap_sem, then doing via cleanup with write mmap_sem (for
discussion detail, see https://lwn.net/Articles/753269/)


Approach:
Zapping pages is the most time consuming part, according to the suggestion from
Michal Hocko [1], zapping pages can be done with holding read mmap_sem, like
what MADV_DONTNEED does. Then re-acquire write mmap_sem to cleanup vmas.

But, we can't call MADV_DONTNEED directly, since there are two major drawbacks:
  * The unexpected state from PF if it wins the race in the middle of munmap.
    It may return zero page, instead of the content or SIGSEGV.
  * Can’t handle VM_LOCKED | VM_HUGETLB | VM_PFNMAP and uprobe mappings, which
    is a showstopper from akpm

But, some part may need write mmap_sem, for example, vma splitting. So,
the design is as follows:
        acquire write mmap_sem
        lookup vmas (find and split vmas)
        detach vmas
        deal with special mappings
        downgrade_write

        zap pages
        free page tables
        release mmap_sem

The vm events with read mmap_sem may come in during page zapping, but
since vmas have been detached before, they, i.e. page fault, gup, etc,
will not be able to find valid vma, then just return SIGSEGV or -EFAULT
as expected.

If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
considered as special mappings. They will be dealt with before zapping
pages with write mmap_sem held. Basically, just update vm_flags.

And, since they are also manipulated by unmap_single_vma() which is
called by unmap_vma() with read mmap_sem held in this case, to
prevent from updating vm_flags in read critical section, a new
parameter, called "skip_flags" is added to unmap_region(), unmap_vmas()
and unmap_single_vma(). If it is true, then just skip unmap those
special mappings. Currently, the only place which pass true to this
parameter is us.

With this approach we don't have to re-acquire mmap_sem again to clean
up vmas to avoid race window which might get the address space changed.

And, since the lock acquire/release cost is managed to the minimum and
almost as same as before, the optimization could be extended to any size
of mapping without incuring significant penalty to small mappings.

For the time being, just do this in munmap syscall path. Other vm_munmap() or
do_munmap() call sites (i.e mmap, mremap, etc) remain intact for stability
reason.

Changelog:
v5 -> v6:
* Fixed the comments from Kirill and Laurent
* Added Laurent's reviewed-by to patch 1/2. Thanks.

v4 -> v5:
* Detach vmas before zapping pages so that we don't have to use VM_DEAD to mark
  a being unmapping vma since they have been detached from rbtree when zapping
  pages. Per Kirill
* Eliminate VM_DEAD stuff
* With this change we don't have to re-acquire write mmap_sem to do cleanup.
  So, we could eliminate a potential race window
* Eliminate PUD_SIZE check, and extend this optimization to all size

v3 -> v4:
* Extend check_stable_address_space to check VM_DEAD as Michal suggested
* Deal with vm_flags update of VM_LOCKED | VM_HUGETLB | VM_PFNMAP and uprobe
  mappings with exclusive lock held. The actual unmapping is still done with read
  mmap_sem to solve akpm's concern
* Clean up vmas with calling do_munmap to prevent from race condition by not
  carrying vmas as Kirill suggested
* Extracted more common code
* Solved some code cleanup comments from akpm
* Dropped uprobe and arch specific code, now all the changes are mm only
* Still keep PUD_SIZE threshold, if everyone thinks it is better to extend to all
  sizes or smaller size, will remove it
* Make this optimization 64 bit only explicitly per akpm's suggestion

v2 -> v3:
* Refactor do_munmap code to extract the common part per Peter's sugestion
* Introduced VM_DEAD flag per Michal's suggestion. Just handled VM_DEAD in
  x86's page fault handler for the time being. Other architectures will be covered
  once the patch series is reviewed
* Now lookup vma (find and split) and set VM_DEAD flag with write mmap_sem, then
  zap mapping with read mmap_sem, then clean up pgtables and vmas with write
  mmap_sem per Peter's suggestion

v1 -> v2:
* Re-implemented the code per the discussion on LSFMM summit


Regression and performance data:
Did the below regression test with setting thresh to 4K manually in the code:
  * Full LTP
  * Trinity (munmap/all vm syscalls)
  * Stress-ng: mmap/mmapfork/mmapfixed/mmapaddr/mmapmany/vm
  * mm-tests: kernbench, phpbench, sysbench-mariadb, will-it-scale
  * vm-scalability

With the patches, exclusive mmap_sem hold time when munmap a 80GB address
space on a machine with 32 cores of E5-2680 @ 2.70GHz dropped to us level
from second.

munmap_test-15002 [008]   594.380138: funcgraph_entry: |  vm_munmap_zap_rlock() {
munmap_test-15002 [008]   594.380146: funcgraph_entry:      !2485684 us |    unmap_region();
munmap_test-15002 [008]   596.865836: funcgraph_exit:       !2485692 us |  }

Here the excution time of unmap_region() is used to evaluate the time of
holding read mmap_sem, then the remaining time is used with holding
exclusive lock.

Yang Shi (2):
      mm: refactor do_munmap() to extract the common part
      mm: mmap: zap pages with read mmap_sem in munmap

 include/linux/mm.h |   2 +-
 mm/memory.c        |  41 ++++++++++----
 mm/mmap.c          | 219 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------
 3 files changed, 205 insertions(+), 57 deletions(-)

[RFC v6 PATCH 1/2] mm: refactor do_munmap() to extract the common part

[Yang Shi]

Introduces three new helper functions:
  * munmap_addr_sanity()
  * munmap_lookup_vma()
  * munmap_mlock_vma()

They will be used by do_munmap() and the new do_munmap with zapping
large mapping early in the later patch.

There is no functional change, just code refactor.

Reviewed-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
 mm/mmap.c | 120 ++++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 82 insertions(+), 38 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index d1eb87e..2504094 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2686,34 +2686,44 @@ int split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
 	return __split_vma(mm, vma, addr, new_below);
 }
 
-/* Munmap is split into 2 main parts -- this part which finds
- * what needs doing, and the areas themselves, which do the
- * work.  This now handles partial unmappings.
- * Jeremy Fitzhardinge <jeremy@goop.org>
- */
-int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
-	      struct list_head *uf)
+static inline bool munmap_addr_sanity(unsigned long start, size_t len)
 {
-	unsigned long end;
-	struct vm_area_struct *vma, *prev, *last;
-
 	if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start)
-		return -EINVAL;
+		return false;
 
-	len = PAGE_ALIGN(len);
-	if (len == 0)
-		return -EINVAL;
+	if (PAGE_ALIGN(len) == 0)
+		return false;
+
+	return true;
+}
+
+/*
+ * munmap_lookup_vma: find the first overlap vma and split overlap vmas.
+ * @mm: mm_struct
+ * @vma: the first overlapping vma
+ * @prev: vma's prev
+ * @start: start address
+ * @end: end address
+ *
+ * returns 1 if successful, 0 or errno otherwise
+ */
+static int munmap_lookup_vma(struct mm_struct *mm, struct vm_area_struct **vma,
+			     struct vm_area_struct **prev, unsigned long start,
+			     unsigned long end)
+{
+	struct vm_area_struct *tmp, *last;
 
 	/* Find the first overlapping VMA */
-	vma = find_vma(mm, start);
-	if (!vma)
+	tmp = find_vma(mm, start);
+	if (!tmp)
 		return 0;
-	prev = vma->vm_prev;
-	/* we have  start < vma->vm_end  */
+
+	*prev = tmp->vm_prev;
+
+	/* we have start < vma->vm_end  */
 
 	/* if it doesn't overlap, we have nothing.. */
-	end = start + len;
-	if (vma->vm_start >= end)
+	if (tmp->vm_start >= end)
 		return 0;
 
 	/*
@@ -2723,7 +2733,7 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
 	 * unmapped vm_area_struct will remain in use: so lower split_vma
 	 * places tmp vma above, and higher split_vma places tmp vma below.
 	 */
-	if (start > vma->vm_start) {
+	if (start > tmp->vm_start) {
 		int error;
 
 		/*
@@ -2731,13 +2741,14 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
 		 * not exceed its limit; but let map_count go just above
 		 * its limit temporarily, to help free resources as expected.
 		 */
-		if (end < vma->vm_end && mm->map_count >= sysctl_max_map_count)
+		if (end < tmp->vm_end &&
+		    mm->map_count > sysctl_max_map_count)
 			return -ENOMEM;
 
-		error = __split_vma(mm, vma, start, 0);
+		error = __split_vma(mm, tmp, start, 0);
 		if (error)
 			return error;
-		prev = vma;
+		*prev = tmp;
 	}
 
 	/* Does it split the last one? */
@@ -2747,7 +2758,48 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
 		if (error)
 			return error;
 	}
-	vma = prev ? prev->vm_next : mm->mmap;
+
+	*vma = *prev ? (*prev)->vm_next : mm->mmap;
+
+	return 1;
+}
+
+static inline void munmap_mlock_vma(struct vm_area_struct *vma,
+				    unsigned long end)
+{
+	struct vm_area_struct *tmp = vma;
+
+	while (tmp && tmp->vm_start < end) {
+		if (tmp->vm_flags & VM_LOCKED) {
+			vma->vm_mm->locked_vm -= vma_pages(tmp);
+			munlock_vma_pages_all(tmp);
+		}
+		tmp = tmp->vm_next;
+	}
+}
+
+/* Munmap is split into 2 main parts -- this part which finds
+ * what needs doing, and the areas themselves, which do the
+ * work.  This now handles partial unmappings.
+ * Jeremy Fitzhardinge <jeremy@goop.org>
+ */
+int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
+	      struct list_head *uf)
+{
+	unsigned long end;
+	struct vm_area_struct *vma = NULL, *prev;
+	int ret = 0;
+
+	if (!munmap_addr_sanity(start, len))
+		return -EINVAL;
+
+	len = PAGE_ALIGN(len);
+
+	end = start + len;
+
+	ret = munmap_lookup_vma(mm, &vma, &prev, start, end);
+	if (ret != 1)
+		return ret;
 
 	if (unlikely(uf)) {
 		/*
@@ -2759,24 +2811,16 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
 		 * split, despite we could. This is unlikely enough
 		 * failure that it's not worth optimizing it for.
 		 */
-		int error = userfaultfd_unmap_prep(vma, start, end, uf);
-		if (error)
-			return error;
+		ret = userfaultfd_unmap_prep(vma, start, end, uf);
+		if (ret)
+			return ret;
 	}
 
 	/*
 	 * unlock any mlock()ed ranges before detaching vmas
 	 */
-	if (mm->locked_vm) {
-		struct vm_area_struct *tmp = vma;
-		while (tmp && tmp->vm_start < end) {
-			if (tmp->vm_flags & VM_LOCKED) {
-				mm->locked_vm -= vma_pages(tmp);
-				munlock_vma_pages_all(tmp);
-			}
-			tmp = tmp->vm_next;
-		}
-	}
+	if (mm->locked_vm)
+		munmap_mlock_vma(vma, end);
 
 	/*
 	 * Remove the vma's, and unmap the actual pages
-- 
1.8.3.1

[RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Yang Shi]

When running some mmap/munmap scalability tests with large memory (i.e.
> 300GB), the below hung task issue may happen occasionally.

INFO: task ps:14018 blocked for more than 120 seconds.
       Tainted: G            E 4.9.79-009.ali3000.alios7.x86_64 #1
 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
message.
 ps              D    0 14018      1 0x00000004
  ffff885582f84000 ffff885e8682f000 ffff880972943000 ffff885ebf499bc0
  ffff8828ee120000 ffffc900349bfca8 ffffffff817154d0 0000000000000040
  00ffffff812f872a ffff885ebf499bc0 024000d000948300 ffff880972943000
 Call Trace:
  [<ffffffff817154d0>] ? __schedule+0x250/0x730
  [<ffffffff817159e6>] schedule+0x36/0x80
  [<ffffffff81718560>] rwsem_down_read_failed+0xf0/0x150
  [<ffffffff81390a28>] call_rwsem_down_read_failed+0x18/0x30
  [<ffffffff81717db0>] down_read+0x20/0x40
  [<ffffffff812b9439>] proc_pid_cmdline_read+0xd9/0x4e0
  [<ffffffff81253c95>] ? do_filp_open+0xa5/0x100
  [<ffffffff81241d87>] __vfs_read+0x37/0x150
  [<ffffffff812f824b>] ? security_file_permission+0x9b/0xc0
  [<ffffffff81242266>] vfs_read+0x96/0x130
  [<ffffffff812437b5>] SyS_read+0x55/0xc0
  [<ffffffff8171a6da>] entry_SYSCALL_64_fastpath+0x1a/0xc5

It is because munmap holds mmap_sem exclusively from very beginning to
all the way down to the end, and doesn't release it in the middle. When
unmapping large mapping, it may take long time (take ~18 seconds to
unmap 320GB mapping with every single page mapped on an idle machine).

Zapping pages is the most time consuming part, according to the
suggestion from Michal Hocko [1], zapping pages can be done with holding
read mmap_sem, like what MADV_DONTNEED does. Then re-acquire write
mmap_sem to cleanup vmas.

But, some part may need write mmap_sem, for example, vma splitting. So,
the design is as follows:
        acquire write mmap_sem
        lookup vmas (find and split vmas)
	detach vmas
        deal with special mappings
        downgrade_write

        zap pages
	free page tables
        release mmap_sem

The vm events with read mmap_sem may come in during page zapping, but
since vmas have been detached before, they, i.e. page fault, gup, etc,
will not be able to find valid vma, then just return SIGSEGV or -EFAULT
as expected.

If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
considered as special mappings. They will be dealt with before zapping
pages with write mmap_sem held. Basically, just update vm_flags.

And, since they are also manipulated by unmap_single_vma() which is
called by unmap_vma() with read mmap_sem held in this case, to
prevent from updating vm_flags in read critical section, a new
parameter, called "skip_flags" is added to unmap_region(), unmap_vmas()
and unmap_single_vma(). If it is true, then just skip unmap those
special mappings. Currently, the only place which pass true to this
parameter is us.

With this approach we don't have to re-acquire mmap_sem again to clean
up vmas to avoid race window which might get the address space changed.

And, since the lock acquire/release cost is managed to the minimum and
almost as same as before, the optimization could be extended to any size
of mapping without incurring significant penalty to small mappings.

For the time being, just do this in munmap syscall path. Other
vm_munmap() or do_munmap() call sites (i.e mmap, mremap, etc) remain
intact for stability reason.

With the patches, exclusive mmap_sem hold time when munmap a 80GB
address space on a machine with 32 cores of E5-2680 @ 2.70GHz dropped to
us level from second.

munmap_test-15002 [008]   594.380138: funcgraph_entry: |  vm_munmap_zap_rlock() {
munmap_test-15002 [008]   594.380146: funcgraph_entry:      !2485684 us |    unmap_region();
munmap_test-15002 [008]   596.865836: funcgraph_exit:       !2485692 us |  }

Here the excution time of unmap_region() is used to evaluate the time of
holding read mmap_sem, then the remaining time is used with holding
exclusive lock.

[1] https://lwn.net/Articles/753269/

Suggested-by: Michal Hocko <mhocko@kernel.org>
Suggested-by: Kirill A. Shutemov <kirill@shutemov.name>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Laurent Dufour <ldufour@linux.vnet.ibm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
 include/linux/mm.h |  2 +-
 mm/memory.c        | 41 ++++++++++++++++------
 mm/mmap.c          | 99 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 123 insertions(+), 19 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index a0fbb9f..e4480d8 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1321,7 +1321,7 @@ void zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
 void zap_page_range(struct vm_area_struct *vma, unsigned long address,
 		    unsigned long size);
 void unmap_vmas(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
-		unsigned long start, unsigned long end);
+		unsigned long start, unsigned long end, bool skip_vm_flags);
 
 /**
  * mm_walk - callbacks for walk_page_range
diff --git a/mm/memory.c b/mm/memory.c
index 7206a63..6a772bd 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1514,7 +1514,7 @@ void unmap_page_range(struct mmu_gather *tlb,
 static void unmap_single_vma(struct mmu_gather *tlb,
 		struct vm_area_struct *vma, unsigned long start_addr,
 		unsigned long end_addr,
-		struct zap_details *details)
+		struct zap_details *details, bool skip_vm_flags)
 {
 	unsigned long start = max(vma->vm_start, start_addr);
 	unsigned long end;
@@ -1525,11 +1525,19 @@ static void unmap_single_vma(struct mmu_gather *tlb,
 	if (end <= vma->vm_start)
 		return;
 
-	if (vma->vm_file)
-		uprobe_munmap(vma, start, end);
+	/*
+	 * Since unmap_single_vma might be called with read mmap_sem held
+	 * in munmap optimization, so vm_flags can't be updated in this case.
+	 * They have been updated before this call with write mmap_sem held.
+	 * Here if skip_vm_flags is true, just skip the update.
+	 */
+	if (!skip_vm_flags) {
+		if (vma->vm_file)
+			uprobe_munmap(vma, start, end);
 
-	if (unlikely(vma->vm_flags & VM_PFNMAP))
-		untrack_pfn(vma, 0, 0);
+		if (unlikely(vma->vm_flags & VM_PFNMAP))
+			untrack_pfn(vma, 0, 0);
+	}
 
 	if (start != end) {
 		if (unlikely(is_vm_hugetlb_page(vma))) {
@@ -1546,7 +1554,19 @@ static void unmap_single_vma(struct mmu_gather *tlb,
 			 */
 			if (vma->vm_file) {
 				i_mmap_lock_write(vma->vm_file->f_mapping);
-				__unmap_hugepage_range_final(tlb, vma, start, end, NULL);
+				if (!skip_vm_flags) {
+					/*
+					 * The vma is being unmapped with read
+					 * mmap_sem.
+					 * Can't update vm_flags here, it has
+					 * been updated before this call with
+					 * write mmap_sem held.
+					 */
+					__unmap_hugepage_range(tlb, vma, start,
+							end, NULL);
+				} else
+					__unmap_hugepage_range_final(tlb, vma,
+							start, end, NULL);
 				i_mmap_unlock_write(vma->vm_file->f_mapping);
 			}
 		} else
@@ -1574,13 +1594,14 @@ static void unmap_single_vma(struct mmu_gather *tlb,
  */
 void unmap_vmas(struct mmu_gather *tlb,
 		struct vm_area_struct *vma, unsigned long start_addr,
-		unsigned long end_addr)
+		unsigned long end_addr, bool skip_vm_flags)
 {
 	struct mm_struct *mm = vma->vm_mm;
 
 	mmu_notifier_invalidate_range_start(mm, start_addr, end_addr);
 	for ( ; vma && vma->vm_start < end_addr; vma = vma->vm_next)
-		unmap_single_vma(tlb, vma, start_addr, end_addr, NULL);
+		unmap_single_vma(tlb, vma, start_addr, end_addr, NULL,
+				 skip_vm_flags);
 	mmu_notifier_invalidate_range_end(mm, start_addr, end_addr);
 }
 
@@ -1604,7 +1625,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, NULL);
+		unmap_single_vma(&tlb, vma, start, end, NULL, false);
 
 		/*
 		 * zap_page_range does not specify whether mmap_sem should be
@@ -1641,7 +1662,7 @@ static void zap_page_range_single(struct vm_area_struct *vma, unsigned long addr
 	tlb_gather_mmu(&tlb, mm, address, end);
 	update_hiwater_rss(mm);
 	mmu_notifier_invalidate_range_start(mm, address, end);
-	unmap_single_vma(&tlb, vma, address, end, details);
+	unmap_single_vma(&tlb, vma, address, end, details, false);
 	mmu_notifier_invalidate_range_end(mm, address, end);
 	tlb_finish_mmu(&tlb, address, end);
 }
diff --git a/mm/mmap.c b/mm/mmap.c
index 2504094..663a0c5 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -73,7 +73,7 @@
 
 static void unmap_region(struct mm_struct *mm,
 		struct vm_area_struct *vma, struct vm_area_struct *prev,
-		unsigned long start, unsigned long end);
+		unsigned long start, unsigned long end, bool skip_flags);
 
 /* description of effects of mapping type and prot in current implementation.
  * this is due to the limited x86 page protection hardware.  The expected
@@ -1824,7 +1824,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
 	fput(file);
 
 	/* Undo any partial mapping done by a device driver. */
-	unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
+	unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end, false);
 	charged = 0;
 	if (vm_flags & VM_SHARED)
 		mapping_unmap_writable(file->f_mapping);
@@ -2559,7 +2559,7 @@ static void remove_vma_list(struct mm_struct *mm, struct vm_area_struct *vma)
  */
 static void unmap_region(struct mm_struct *mm,
 		struct vm_area_struct *vma, struct vm_area_struct *prev,
-		unsigned long start, unsigned long end)
+		unsigned long start, unsigned long end, bool skip_flags)
 {
 	struct vm_area_struct *next = prev ? prev->vm_next : mm->mmap;
 	struct mmu_gather tlb;
@@ -2567,7 +2567,7 @@ static void unmap_region(struct mm_struct *mm,
 	lru_add_drain();
 	tlb_gather_mmu(&tlb, mm, start, end);
 	update_hiwater_rss(mm);
-	unmap_vmas(&tlb, vma, start, end);
+	unmap_vmas(&tlb, vma, start, end, skip_flags);
 	free_pgtables(&tlb, vma, prev ? prev->vm_end : FIRST_USER_ADDRESS,
 				 next ? next->vm_start : USER_PGTABLES_CEILING);
 	tlb_finish_mmu(&tlb, start, end);
@@ -2778,6 +2778,79 @@ static inline void munmap_mlock_vma(struct vm_area_struct *vma,
 	}
 }
 
+/*
+ * Zap pages with read mmap_sem held
+ *
+ * uf is the list for userfaultfd
+ */
+static int do_munmap_zap_rlock(struct mm_struct *mm, unsigned long start,
+			       size_t len, struct list_head *uf)
+{
+	unsigned long end = 0;
+	struct vm_area_struct *start_vma = NULL, *prev, *vma;
+	int ret = 0;
+
+	if (!munmap_addr_sanity(start, len))
+		return -EINVAL;
+
+	len = PAGE_ALIGN(len);
+
+	end = start + len;
+
+	/*
+	 * Need write mmap_sem to split vmas and detach vmas
+	 * splitting vma up-front to save PITA to clean if it is failed
+	 */
+	if (down_write_killable(&mm->mmap_sem))
+		return -EINTR;
+
+	ret = munmap_lookup_vma(mm, &start_vma, &prev, start, end);
+	if (ret != 1)
+		goto out;
+
+	if (unlikely(uf)) {
+		ret = userfaultfd_unmap_prep(start_vma, start, end, uf);
+		if (ret)
+			goto out;
+	}
+
+	/* Handle mlocked vmas */
+	if (mm->locked_vm)
+		munmap_mlock_vma(start_vma, end);
+
+	/* Detach vmas from rbtree */
+	detach_vmas_to_be_unmapped(mm, start_vma, prev, end);
+
+	/*
+	 * Clear uprobe, VM_PFNMAP and hugetlb mapping in advance since they
+	 * need update vm_flags with write mmap_sem
+	 */
+	vma = start_vma;
+	for ( ; vma && vma->vm_start < end; vma = vma->vm_next) {
+		if (vma->vm_file)
+			uprobe_munmap(vma, vma->vm_start, vma->vm_end);
+		if (unlikely(vma->vm_flags & VM_PFNMAP))
+			untrack_pfn(vma, 0, 0);
+		if (is_vm_hugetlb_page(vma))
+			vma->vm_flags &= ~VM_MAYSHARE;
+	}
+
+	downgrade_write(&mm->mmap_sem);
+
+	/* Zap mappings with read mmap_sem */
+	unmap_region(mm, start_vma, prev, start, end, true);
+
+	arch_unmap(mm, start_vma, start, end);
+	remove_vma_list(mm, start_vma);
+	up_read(&mm->mmap_sem);
+
+	return 0;
+
+out:
+	up_write(&mm->mmap_sem);
+	return ret;
+}
+
 /* Munmap is split into 2 main parts -- this part which finds
  * what needs doing, and the areas themselves, which do the
  * work.  This now handles partial unmappings.
@@ -2826,7 +2899,7 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
 	 * Remove the vma's, and unmap the actual pages
 	 */
 	detach_vmas_to_be_unmapped(mm, vma, prev, end);
-	unmap_region(mm, vma, prev, start, end);
+	unmap_region(mm, vma, prev, start, end, false);
 
 	arch_unmap(mm, vma, start, end);
 
@@ -2836,6 +2909,17 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
 	return 0;
 }
 
+static int vm_munmap_zap_rlock(unsigned long start, size_t len)
+{
+	int ret;
+	struct mm_struct *mm = current->mm;
+	LIST_HEAD(uf);
+
+	ret = do_munmap_zap_rlock(mm, start, len, &uf);
+	userfaultfd_unmap_complete(mm, &uf);
+	return ret;
+}
+
 int vm_munmap(unsigned long start, size_t len)
 {
 	int ret;
@@ -2855,10 +2939,9 @@ int vm_munmap(unsigned long start, size_t len)
 SYSCALL_DEFINE2(munmap, unsigned long, addr, size_t, len)
 {
 	profile_munmap(addr);
-	return vm_munmap(addr, len);
+	return vm_munmap_zap_rlock(addr, len);
 }
 
-
 /*
  * Emulation of deprecated remap_file_pages() syscall.
  */
@@ -3146,7 +3229,7 @@ void exit_mmap(struct mm_struct *mm)
 	tlb_gather_mmu(&tlb, mm, 0, -1);
 	/* update_hiwater_rss(mm) here? but nobody should be looking */
 	/* Use -1 here to ensure all VMAs in the mm are unmapped */
-	unmap_vmas(&tlb, vma, 0, -1);
+	unmap_vmas(&tlb, vma, 0, -1, false);
 	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
 	tlb_finish_mmu(&tlb, 0, -1);
 
-- 
1.8.3.1

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Mika Penttilä]

On 26.07.2018 21:10, Yang Shi wrote:
> When running some mmap/munmap scalability tests with large memory (i.e.
>> 300GB), the below hung task issue may happen occasionally.
> INFO: task ps:14018 blocked for more than 120 seconds.
>        Tainted: G            E 4.9.79-009.ali3000.alios7.x86_64 #1
>  "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
> message.
>  ps              D    0 14018      1 0x00000004
>   ffff885582f84000 ffff885e8682f000 ffff880972943000 ffff885ebf499bc0
>   ffff8828ee120000 ffffc900349bfca8 ffffffff817154d0 0000000000000040
>   00ffffff812f872a ffff885ebf499bc0 024000d000948300 ffff880972943000
>  Call Trace:
>   [<ffffffff817154d0>] ? __schedule+0x250/0x730
>   [<ffffffff817159e6>] schedule+0x36/0x80
>   [<ffffffff81718560>] rwsem_down_read_failed+0xf0/0x150
>   [<ffffffff81390a28>] call_rwsem_down_read_failed+0x18/0x30
>   [<ffffffff81717db0>] down_read+0x20/0x40
>   [<ffffffff812b9439>] proc_pid_cmdline_read+0xd9/0x4e0
>   [<ffffffff81253c95>] ? do_filp_open+0xa5/0x100
>   [<ffffffff81241d87>] __vfs_read+0x37/0x150
>   [<ffffffff812f824b>] ? security_file_permission+0x9b/0xc0
>   [<ffffffff81242266>] vfs_read+0x96/0x130
>   [<ffffffff812437b5>] SyS_read+0x55/0xc0
>   [<ffffffff8171a6da>] entry_SYSCALL_64_fastpath+0x1a/0xc5
>
> It is because munmap holds mmap_sem exclusively from very beginning to
> all the way down to the end, and doesn't release it in the middle. When
> unmapping large mapping, it may take long time (take ~18 seconds to
> unmap 320GB mapping with every single page mapped on an idle machine).
>
> Zapping pages is the most time consuming part, according to the
> suggestion from Michal Hocko [1], zapping pages can be done with holding
> read mmap_sem, like what MADV_DONTNEED does. Then re-acquire write
> mmap_sem to cleanup vmas.
>
> But, some part may need write mmap_sem, for example, vma splitting. So,
> the design is as follows:
>         acquire write mmap_sem
>         lookup vmas (find and split vmas)
> 	detach vmas
>         deal with special mappings
>         downgrade_write
>
>         zap pages
> 	free page tables
>         release mmap_sem
>
> The vm events with read mmap_sem may come in during page zapping, but
> since vmas have been detached before, they, i.e. page fault, gup, etc,
> will not be able to find valid vma, then just return SIGSEGV or -EFAULT
> as expected.
>
> If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
> considered as special mappings. They will be dealt with before zapping
> pages with write mmap_sem held. Basically, just update vm_flags.
>
> And, since they are also manipulated by unmap_single_vma() which is
> called by unmap_vma() with read mmap_sem held in this case, to
> prevent from updating vm_flags in read critical section, a new
> parameter, called "skip_flags" is added to unmap_region(), unmap_vmas()
> and unmap_single_vma(). If it is true, then just skip unmap those
> special mappings. Currently, the only place which pass true to this
> parameter is us.
>
> With this approach we don't have to re-acquire mmap_sem again to clean
> up vmas to avoid race window which might get the address space changed.
>
> And, since the lock acquire/release cost is managed to the minimum and
> almost as same as before, the optimization could be extended to any size
> of mapping without incurring significant penalty to small mappings.
>
> For the time being, just do this in munmap syscall path. Other
> vm_munmap() or do_munmap() call sites (i.e mmap, mremap, etc) remain
> intact for stability reason.
>
> With the patches, exclusive mmap_sem hold time when munmap a 80GB
> address space on a machine with 32 cores of E5-2680 @ 2.70GHz dropped to
> us level from second.
>
> munmap_test-15002 [008]   594.380138: funcgraph_entry: |  vm_munmap_zap_rlock() {
> munmap_test-15002 [008]   594.380146: funcgraph_entry:      !2485684 us |    unmap_region();
> munmap_test-15002 [008]   596.865836: funcgraph_exit:       !2485692 us |  }
>
> Here the excution time of unmap_region() is used to evaluate the time of
> holding read mmap_sem, then the remaining time is used with holding
> exclusive lock.
>
> [1] https://lwn.net/Articles/753269/
>
> Suggested-by: Michal Hocko <mhocko@kernel.org>
> Suggested-by: Kirill A. Shutemov <kirill@shutemov.name>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> ---
>  include/linux/mm.h |  2 +-
>  mm/memory.c        | 41 ++++++++++++++++------
>  mm/mmap.c          | 99 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 123 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a0fbb9f..e4480d8 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1321,7 +1321,7 @@ void zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
>  void zap_page_range(struct vm_area_struct *vma, unsigned long address,
>  		    unsigned long size);
>  void unmap_vmas(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
> -		unsigned long start, unsigned long end);
> +		unsigned long start, unsigned long end, bool skip_vm_flags);
>  
>  /**
>   * mm_walk - callbacks for walk_page_range
> diff --git a/mm/memory.c b/mm/memory.c
> index 7206a63..6a772bd 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1514,7 +1514,7 @@ void unmap_page_range(struct mmu_gather *tlb,
>  static void unmap_single_vma(struct mmu_gather *tlb,
>  		struct vm_area_struct *vma, unsigned long start_addr,
>  		unsigned long end_addr,
> -		struct zap_details *details)
> +		struct zap_details *details, bool skip_vm_flags)
>  {
>  	unsigned long start = max(vma->vm_start, start_addr);
>  	unsigned long end;
> @@ -1525,11 +1525,19 @@ static void unmap_single_vma(struct mmu_gather *tlb,
>  	if (end <= vma->vm_start)
>  		return;
>  
> -	if (vma->vm_file)
> -		uprobe_munmap(vma, start, end);
> +	/*
> +	 * Since unmap_single_vma might be called with read mmap_sem held
> +	 * in munmap optimization, so vm_flags can't be updated in this case.
> +	 * They have been updated before this call with write mmap_sem held.
> +	 * Here if skip_vm_flags is true, just skip the update.
> +	 */
> +	if (!skip_vm_flags) {
> +		if (vma->vm_file)
> +			uprobe_munmap(vma, start, end);
>  
> -	if (unlikely(vma->vm_flags & VM_PFNMAP))
> -		untrack_pfn(vma, 0, 0);
> +		if (unlikely(vma->vm_flags & VM_PFNMAP))
> +			untrack_pfn(vma, 0, 0);
> +	}
>  
>  	if (start != end) {
>  		if (unlikely(is_vm_hugetlb_page(vma))) {
> @@ -1546,7 +1554,19 @@ static void unmap_single_vma(struct mmu_gather *tlb,
>  			 */
>  			if (vma->vm_file) {
>  				i_mmap_lock_write(vma->vm_file->f_mapping);
> -				__unmap_hugepage_range_final(tlb, vma, start, end, NULL);
> +				if (!skip_vm_flags) {

Should that be :
	if (skip_vm_flags) {
instead?
 

> +					/*
> +					 * The vma is being unmapped with read
> +					 * mmap_sem.
> +					 * Can't update vm_flags here, it has
> +					 * been updated before this call with
> +					 * write mmap_sem held.
> +					 */
> +					__unmap_hugepage_range(tlb, vma, start,
> +							end, NULL);
> +				} else
> +					__unmap_hugepage_range_final(tlb, vma,
> +							start, end, NULL);
>  				i_mmap_unlock_write(vma->vm_file->f_mapping);
>  			}
>  		} else
>
--Mika

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Yang Shi]

On 7/26/18 11:34 AM, Mika Penttilä wrote:
>
> On 26.07.2018 21:10, Yang Shi wrote:
>> When running some mmap/munmap scalability tests with large memory (i.e.
>>> 300GB), the below hung task issue may happen occasionally.
>> INFO: task ps:14018 blocked for more than 120 seconds.
>>         Tainted: G            E 4.9.79-009.ali3000.alios7.x86_64 #1
>>   "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
>> message.
>>   ps              D    0 14018      1 0x00000004
>>    ffff885582f84000 ffff885e8682f000 ffff880972943000 ffff885ebf499bc0
>>    ffff8828ee120000 ffffc900349bfca8 ffffffff817154d0 0000000000000040
>>    00ffffff812f872a ffff885ebf499bc0 024000d000948300 ffff880972943000
>>   Call Trace:
>>    [<ffffffff817154d0>] ? __schedule+0x250/0x730
>>    [<ffffffff817159e6>] schedule+0x36/0x80
>>    [<ffffffff81718560>] rwsem_down_read_failed+0xf0/0x150
>>    [<ffffffff81390a28>] call_rwsem_down_read_failed+0x18/0x30
>>    [<ffffffff81717db0>] down_read+0x20/0x40
>>    [<ffffffff812b9439>] proc_pid_cmdline_read+0xd9/0x4e0
>>    [<ffffffff81253c95>] ? do_filp_open+0xa5/0x100
>>    [<ffffffff81241d87>] __vfs_read+0x37/0x150
>>    [<ffffffff812f824b>] ? security_file_permission+0x9b/0xc0
>>    [<ffffffff81242266>] vfs_read+0x96/0x130
>>    [<ffffffff812437b5>] SyS_read+0x55/0xc0
>>    [<ffffffff8171a6da>] entry_SYSCALL_64_fastpath+0x1a/0xc5
>>
>> It is because munmap holds mmap_sem exclusively from very beginning to
>> all the way down to the end, and doesn't release it in the middle. When
>> unmapping large mapping, it may take long time (take ~18 seconds to
>> unmap 320GB mapping with every single page mapped on an idle machine).
>>
>> Zapping pages is the most time consuming part, according to the
>> suggestion from Michal Hocko [1], zapping pages can be done with holding
>> read mmap_sem, like what MADV_DONTNEED does. Then re-acquire write
>> mmap_sem to cleanup vmas.
>>
>> But, some part may need write mmap_sem, for example, vma splitting. So,
>> the design is as follows:
>>          acquire write mmap_sem
>>          lookup vmas (find and split vmas)
>> 	detach vmas
>>          deal with special mappings
>>          downgrade_write
>>
>>          zap pages
>> 	free page tables
>>          release mmap_sem
>>
>> The vm events with read mmap_sem may come in during page zapping, but
>> since vmas have been detached before, they, i.e. page fault, gup, etc,
>> will not be able to find valid vma, then just return SIGSEGV or -EFAULT
>> as expected.
>>
>> If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
>> considered as special mappings. They will be dealt with before zapping
>> pages with write mmap_sem held. Basically, just update vm_flags.
>>
>> And, since they are also manipulated by unmap_single_vma() which is
>> called by unmap_vma() with read mmap_sem held in this case, to
>> prevent from updating vm_flags in read critical section, a new
>> parameter, called "skip_flags" is added to unmap_region(), unmap_vmas()
>> and unmap_single_vma(). If it is true, then just skip unmap those
>> special mappings. Currently, the only place which pass true to this
>> parameter is us.
>>
>> With this approach we don't have to re-acquire mmap_sem again to clean
>> up vmas to avoid race window which might get the address space changed.
>>
>> And, since the lock acquire/release cost is managed to the minimum and
>> almost as same as before, the optimization could be extended to any size
>> of mapping without incurring significant penalty to small mappings.
>>
>> For the time being, just do this in munmap syscall path. Other
>> vm_munmap() or do_munmap() call sites (i.e mmap, mremap, etc) remain
>> intact for stability reason.
>>
>> With the patches, exclusive mmap_sem hold time when munmap a 80GB
>> address space on a machine with 32 cores of E5-2680 @ 2.70GHz dropped to
>> us level from second.
>>
>> munmap_test-15002 [008]   594.380138: funcgraph_entry: |  vm_munmap_zap_rlock() {
>> munmap_test-15002 [008]   594.380146: funcgraph_entry:      !2485684 us |    unmap_region();
>> munmap_test-15002 [008]   596.865836: funcgraph_exit:       !2485692 us |  }
>>
>> Here the excution time of unmap_region() is used to evaluate the time of
>> holding read mmap_sem, then the remaining time is used with holding
>> exclusive lock.
>>
>> [1] https://lwn.net/Articles/753269/
>>
>> Suggested-by: Michal Hocko <mhocko@kernel.org>
>> Suggested-by: Kirill A. Shutemov <kirill@shutemov.name>
>> Cc: Matthew Wilcox <willy@infradead.org>
>> Cc: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>> ---
>>   include/linux/mm.h |  2 +-
>>   mm/memory.c        | 41 ++++++++++++++++------
>>   mm/mmap.c          | 99 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>>   3 files changed, 123 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index a0fbb9f..e4480d8 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1321,7 +1321,7 @@ void zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
>>   void zap_page_range(struct vm_area_struct *vma, unsigned long address,
>>   		    unsigned long size);
>>   void unmap_vmas(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
>> -		unsigned long start, unsigned long end);
>> +		unsigned long start, unsigned long end, bool skip_vm_flags);
>>   
>>   /**
>>    * mm_walk - callbacks for walk_page_range
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 7206a63..6a772bd 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -1514,7 +1514,7 @@ void unmap_page_range(struct mmu_gather *tlb,
>>   static void unmap_single_vma(struct mmu_gather *tlb,
>>   		struct vm_area_struct *vma, unsigned long start_addr,
>>   		unsigned long end_addr,
>> -		struct zap_details *details)
>> +		struct zap_details *details, bool skip_vm_flags)
>>   {
>>   	unsigned long start = max(vma->vm_start, start_addr);
>>   	unsigned long end;
>> @@ -1525,11 +1525,19 @@ static void unmap_single_vma(struct mmu_gather *tlb,
>>   	if (end <= vma->vm_start)
>>   		return;
>>   
>> -	if (vma->vm_file)
>> -		uprobe_munmap(vma, start, end);
>> +	/*
>> +	 * Since unmap_single_vma might be called with read mmap_sem held
>> +	 * in munmap optimization, so vm_flags can't be updated in this case.
>> +	 * They have been updated before this call with write mmap_sem held.
>> +	 * Here if skip_vm_flags is true, just skip the update.
>> +	 */
>> +	if (!skip_vm_flags) {
>> +		if (vma->vm_file)
>> +			uprobe_munmap(vma, start, end);
>>   
>> -	if (unlikely(vma->vm_flags & VM_PFNMAP))
>> -		untrack_pfn(vma, 0, 0);
>> +		if (unlikely(vma->vm_flags & VM_PFNMAP))
>> +			untrack_pfn(vma, 0, 0);
>> +	}
>>   
>>   	if (start != end) {
>>   		if (unlikely(is_vm_hugetlb_page(vma))) {
>> @@ -1546,7 +1554,19 @@ static void unmap_single_vma(struct mmu_gather *tlb,
>>   			 */
>>   			if (vma->vm_file) {
>>   				i_mmap_lock_write(vma->vm_file->f_mapping);
>> -				__unmap_hugepage_range_final(tlb, vma, start, end, NULL);
>> +				if (!skip_vm_flags) {
> Should that be :
> 	if (skip_vm_flags) {
> instead?

Oh, yes. Thanks for catching this.

Yang

>   
>
>> +					/*
>> +					 * The vma is being unmapped with read
>> +					 * mmap_sem.
>> +					 * Can't update vm_flags here, it has
>> +					 * been updated before this call with
>> +					 * write mmap_sem held.
>> +					 */
>> +					__unmap_hugepage_range(tlb, vma, start,
>> +							end, NULL);
>> +				} else
>> +					__unmap_hugepage_range_final(tlb, vma,
>> +							start, end, NULL);
>>   				i_mmap_unlock_write(vma->vm_file->f_mapping);
>>   			}
>>   		} else
>>
> --Mika

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Laurent Dufour]

On 26/07/2018 20:10, Yang Shi wrote:
> When running some mmap/munmap scalability tests with large memory (i.e.
>> 300GB), the below hung task issue may happen occasionally.
> 
> INFO: task ps:14018 blocked for more than 120 seconds.
>        Tainted: G            E 4.9.79-009.ali3000.alios7.x86_64 #1
>  "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
> message.
>  ps              D    0 14018      1 0x00000004
>   ffff885582f84000 ffff885e8682f000 ffff880972943000 ffff885ebf499bc0
>   ffff8828ee120000 ffffc900349bfca8 ffffffff817154d0 0000000000000040
>   00ffffff812f872a ffff885ebf499bc0 024000d000948300 ffff880972943000
>  Call Trace:
>   [<ffffffff817154d0>] ? __schedule+0x250/0x730
>   [<ffffffff817159e6>] schedule+0x36/0x80
>   [<ffffffff81718560>] rwsem_down_read_failed+0xf0/0x150
>   [<ffffffff81390a28>] call_rwsem_down_read_failed+0x18/0x30
>   [<ffffffff81717db0>] down_read+0x20/0x40
>   [<ffffffff812b9439>] proc_pid_cmdline_read+0xd9/0x4e0
>   [<ffffffff81253c95>] ? do_filp_open+0xa5/0x100
>   [<ffffffff81241d87>] __vfs_read+0x37/0x150
>   [<ffffffff812f824b>] ? security_file_permission+0x9b/0xc0
>   [<ffffffff81242266>] vfs_read+0x96/0x130
>   [<ffffffff812437b5>] SyS_read+0x55/0xc0
>   [<ffffffff8171a6da>] entry_SYSCALL_64_fastpath+0x1a/0xc5
> 
> It is because munmap holds mmap_sem exclusively from very beginning to
> all the way down to the end, and doesn't release it in the middle. When
> unmapping large mapping, it may take long time (take ~18 seconds to
> unmap 320GB mapping with every single page mapped on an idle machine).
> 
> Zapping pages is the most time consuming part, according to the
> suggestion from Michal Hocko [1], zapping pages can be done with holding
> read mmap_sem, like what MADV_DONTNEED does. Then re-acquire write
> mmap_sem to cleanup vmas.
> 
> But, some part may need write mmap_sem, for example, vma splitting. So,
> the design is as follows:
>         acquire write mmap_sem
>         lookup vmas (find and split vmas)
> 	detach vmas
>         deal with special mappings
>         downgrade_write
> 
>         zap pages
> 	free page tables
>         release mmap_sem
> 
> The vm events with read mmap_sem may come in during page zapping, but
> since vmas have been detached before, they, i.e. page fault, gup, etc,
> will not be able to find valid vma, then just return SIGSEGV or -EFAULT
> as expected.
> 
> If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
> considered as special mappings. They will be dealt with before zapping
> pages with write mmap_sem held. Basically, just update vm_flags.
> 
> And, since they are also manipulated by unmap_single_vma() which is
> called by unmap_vma() with read mmap_sem held in this case, to
> prevent from updating vm_flags in read critical section, a new
> parameter, called "skip_flags" is added to unmap_region(), unmap_vmas()
> and unmap_single_vma(). If it is true, then just skip unmap those
> special mappings. Currently, the only place which pass true to this
> parameter is us.
> 
> With this approach we don't have to re-acquire mmap_sem again to clean
> up vmas to avoid race window which might get the address space changed.
> 
> And, since the lock acquire/release cost is managed to the minimum and
> almost as same as before, the optimization could be extended to any size
> of mapping without incurring significant penalty to small mappings.
> 
> For the time being, just do this in munmap syscall path. Other
> vm_munmap() or do_munmap() call sites (i.e mmap, mremap, etc) remain
> intact for stability reason.
> 
> With the patches, exclusive mmap_sem hold time when munmap a 80GB
> address space on a machine with 32 cores of E5-2680 @ 2.70GHz dropped to
> us level from second.
> 
> munmap_test-15002 [008]   594.380138: funcgraph_entry: |  vm_munmap_zap_rlock() {
> munmap_test-15002 [008]   594.380146: funcgraph_entry:      !2485684 us |    unmap_region();
> munmap_test-15002 [008]   596.865836: funcgraph_exit:       !2485692 us |  }
> 
> Here the excution time of unmap_region() is used to evaluate the time of
> holding read mmap_sem, then the remaining time is used with holding
> exclusive lock.
> 
> [1] https://lwn.net/Articles/753269/
> 
> Suggested-by: Michal Hocko <mhocko@kernel.org>
> Suggested-by: Kirill A. Shutemov <kirill@shutemov.name>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> ---
>  include/linux/mm.h |  2 +-
>  mm/memory.c        | 41 ++++++++++++++++------
>  mm/mmap.c          | 99 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 123 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a0fbb9f..e4480d8 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1321,7 +1321,7 @@ void zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
>  void zap_page_range(struct vm_area_struct *vma, unsigned long address,
>  		    unsigned long size);
>  void unmap_vmas(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
> -		unsigned long start, unsigned long end);
> +		unsigned long start, unsigned long end, bool skip_vm_flags);
> 
>  /**
>   * mm_walk - callbacks for walk_page_range
> diff --git a/mm/memory.c b/mm/memory.c
> index 7206a63..6a772bd 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1514,7 +1514,7 @@ void unmap_page_range(struct mmu_gather *tlb,
>  static void unmap_single_vma(struct mmu_gather *tlb,
>  		struct vm_area_struct *vma, unsigned long start_addr,
>  		unsigned long end_addr,
> -		struct zap_details *details)
> +		struct zap_details *details, bool skip_vm_flags)
>  {
>  	unsigned long start = max(vma->vm_start, start_addr);
>  	unsigned long end;
> @@ -1525,11 +1525,19 @@ static void unmap_single_vma(struct mmu_gather *tlb,
>  	if (end <= vma->vm_start)
>  		return;
> 
> -	if (vma->vm_file)
> -		uprobe_munmap(vma, start, end);
> +	/*
> +	 * Since unmap_single_vma might be called with read mmap_sem held
> +	 * in munmap optimization, so vm_flags can't be updated in this case.
> +	 * They have been updated before this call with write mmap_sem held.
> +	 * Here if skip_vm_flags is true, just skip the update.
> +	 */
> +	if (!skip_vm_flags) {
> +		if (vma->vm_file)
> +			uprobe_munmap(vma, start, end);
> 
> -	if (unlikely(vma->vm_flags & VM_PFNMAP))
> -		untrack_pfn(vma, 0, 0);
> +		if (unlikely(vma->vm_flags & VM_PFNMAP))
> +			untrack_pfn(vma, 0, 0);
> +	}
> 
>  	if (start != end) {
>  		if (unlikely(is_vm_hugetlb_page(vma))) {
> @@ -1546,7 +1554,19 @@ static void unmap_single_vma(struct mmu_gather *tlb,
>  			 */
>  			if (vma->vm_file) {
>  				i_mmap_lock_write(vma->vm_file->f_mapping);
> -				__unmap_hugepage_range_final(tlb, vma, start, end, NULL);
> +				if (!skip_vm_flags) {

As already reported by Mika : if (skip_vm_flags).

> +					/*
> +					 * The vma is being unmapped with read
> +					 * mmap_sem.
> +					 * Can't update vm_flags here, it has
> +					 * been updated before this call with
> +					 * write mmap_sem held.
> +					 */
> +					__unmap_hugepage_range(tlb, vma, start,
> +							end, NULL);
> +				} else
> +					__unmap_hugepage_range_final(tlb, vma,
> +							start, end, NULL);
>  				i_mmap_unlock_write(vma->vm_file->f_mapping);
>  			}
>  		} else
> @@ -1574,13 +1594,14 @@ static void unmap_single_vma(struct mmu_gather *tlb,
>   */
>  void unmap_vmas(struct mmu_gather *tlb,
>  		struct vm_area_struct *vma, unsigned long start_addr,
> -		unsigned long end_addr)
> +		unsigned long end_addr, bool skip_vm_flags)
>  {
>  	struct mm_struct *mm = vma->vm_mm;
> 
>  	mmu_notifier_invalidate_range_start(mm, start_addr, end_addr);
>  	for ( ; vma && vma->vm_start < end_addr; vma = vma->vm_next)
> -		unmap_single_vma(tlb, vma, start_addr, end_addr, NULL);
> +		unmap_single_vma(tlb, vma, start_addr, end_addr, NULL,
> +				 skip_vm_flags);
>  	mmu_notifier_invalidate_range_end(mm, start_addr, end_addr);
>  }
> 
> @@ -1604,7 +1625,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, NULL);
> +		unmap_single_vma(&tlb, vma, start, end, NULL, false);
> 
>  		/*
>  		 * zap_page_range does not specify whether mmap_sem should be
> @@ -1641,7 +1662,7 @@ static void zap_page_range_single(struct vm_area_struct *vma, unsigned long addr
>  	tlb_gather_mmu(&tlb, mm, address, end);
>  	update_hiwater_rss(mm);
>  	mmu_notifier_invalidate_range_start(mm, address, end);
> -	unmap_single_vma(&tlb, vma, address, end, details);
> +	unmap_single_vma(&tlb, vma, address, end, details, false);
>  	mmu_notifier_invalidate_range_end(mm, address, end);
>  	tlb_finish_mmu(&tlb, address, end);
>  }
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 2504094..663a0c5 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -73,7 +73,7 @@
> 
>  static void unmap_region(struct mm_struct *mm,
>  		struct vm_area_struct *vma, struct vm_area_struct *prev,
> -		unsigned long start, unsigned long end);
> +		unsigned long start, unsigned long end, bool skip_flags);

Earlier, you used the name 'skip_vm_flags'. It would be nice to keep the same
parameter name everywhere, isn't it ?

> 
>  /* description of effects of mapping type and prot in current implementation.
>   * this is due to the limited x86 page protection hardware.  The expected
> @@ -1824,7 +1824,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  	fput(file);
> 
>  	/* Undo any partial mapping done by a device driver. */
> -	unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
> +	unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end, false);
>  	charged = 0;
>  	if (vm_flags & VM_SHARED)
>  		mapping_unmap_writable(file->f_mapping);
> @@ -2559,7 +2559,7 @@ static void remove_vma_list(struct mm_struct *mm, struct vm_area_struct *vma)
>   */
>  static void unmap_region(struct mm_struct *mm,
>  		struct vm_area_struct *vma, struct vm_area_struct *prev,
> -		unsigned long start, unsigned long end)
> +		unsigned long start, unsigned long end, bool skip_flags)

Here too.

>  {
>  	struct vm_area_struct *next = prev ? prev->vm_next : mm->mmap;
>  	struct mmu_gather tlb;
> @@ -2567,7 +2567,7 @@ static void unmap_region(struct mm_struct *mm,
>  	lru_add_drain();
>  	tlb_gather_mmu(&tlb, mm, start, end);
>  	update_hiwater_rss(mm);
> -	unmap_vmas(&tlb, vma, start, end);
> +	unmap_vmas(&tlb, vma, start, end, skip_flags);
>  	free_pgtables(&tlb, vma, prev ? prev->vm_end : FIRST_USER_ADDRESS,
>  				 next ? next->vm_start : USER_PGTABLES_CEILING);
>  	tlb_finish_mmu(&tlb, start, end);
> @@ -2778,6 +2778,79 @@ static inline void munmap_mlock_vma(struct vm_area_struct *vma,
>  	}
>  }
> 
> +/*
> + * Zap pages with read mmap_sem held
> + *
> + * uf is the list for userfaultfd
> + */
> +static int do_munmap_zap_rlock(struct mm_struct *mm, unsigned long start,
> +			       size_t len, struct list_head *uf)
> +{
> +	unsigned long end = 0;
> +	struct vm_area_struct *start_vma = NULL, *prev, *vma;
> +	int ret = 0;

No need to initialize end, start_vma and ret here, they will be assigned before
used.

> +
> +	if (!munmap_addr_sanity(start, len))
> +		return -EINVAL;
> +
> +	len = PAGE_ALIGN(len);
> +
> +	end = start + len;
> +
> +	/*
> +	 * Need write mmap_sem to split vmas and detach vmas
> +	 * splitting vma up-front to save PITA to clean if it is failed
> +	 */
> +	if (down_write_killable(&mm->mmap_sem))
> +		return -EINTR;
> +
> +	ret = munmap_lookup_vma(mm, &start_vma, &prev, start, end);
> +	if (ret != 1)
> +		goto out;
> +
> +	if (unlikely(uf)) {
> +		ret = userfaultfd_unmap_prep(start_vma, start, end, uf);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	/* Handle mlocked vmas */
> +	if (mm->locked_vm)
> +		munmap_mlock_vma(start_vma, end);
> +
> +	/* Detach vmas from rbtree */
> +	detach_vmas_to_be_unmapped(mm, start_vma, prev, end);
> +
> +	/*
> +	 * Clear uprobe, VM_PFNMAP and hugetlb mapping in advance since they
> +	 * need update vm_flags with write mmap_sem
> +	 */
> +	vma = start_vma;
> +	for ( ; vma && vma->vm_start < end; vma = vma->vm_next) {

Not critical, but 'vma = start_vma' should be part of the init stuff in for(),
like this:
	for (vma = start_vma; vma && vma->vm_start < end; vma = vma->vm_next) {

> +		if (vma->vm_file)
> +			uprobe_munmap(vma, vma->vm_start, vma->vm_end);
> +		if (unlikely(vma->vm_flags & VM_PFNMAP))
> +			untrack_pfn(vma, 0, 0);
> +		if (is_vm_hugetlb_page(vma))
> +			vma->vm_flags &= ~VM_MAYSHARE;
> +	}
> +
> +	downgrade_write(&mm->mmap_sem);
> +
> +	/* Zap mappings with read mmap_sem */
> +	unmap_region(mm, start_vma, prev, start, end, true);
> +
> +	arch_unmap(mm, start_vma, start, end);
> +	remove_vma_list(mm, start_vma);
> +	up_read(&mm->mmap_sem);
> +
> +	return 0;
> +
> +out:
> +	up_write(&mm->mmap_sem);
> +	return ret;
> +}
> +
>  /* Munmap is split into 2 main parts -- this part which finds
>   * what needs doing, and the areas themselves, which do the
>   * work.  This now handles partial unmappings.
> @@ -2826,7 +2899,7 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>  	 * Remove the vma's, and unmap the actual pages
>  	 */
>  	detach_vmas_to_be_unmapped(mm, vma, prev, end);
> -	unmap_region(mm, vma, prev, start, end);
> +	unmap_region(mm, vma, prev, start, end, false);
> 
>  	arch_unmap(mm, vma, start, end);
> 
> @@ -2836,6 +2909,17 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>  	return 0;
>  }
> 
> +static int vm_munmap_zap_rlock(unsigned long start, size_t len)
> +{
> +	int ret;
> +	struct mm_struct *mm = current->mm;
> +	LIST_HEAD(uf);
> +
> +	ret = do_munmap_zap_rlock(mm, start, len, &uf);
> +	userfaultfd_unmap_complete(mm, &uf);
> +	return ret;
> +}
> +
>  int vm_munmap(unsigned long start, size_t len)
>  {
>  	int ret;
> @@ -2855,10 +2939,9 @@ int vm_munmap(unsigned long start, size_t len)
>  SYSCALL_DEFINE2(munmap, unsigned long, addr, size_t, len)
>  {
>  	profile_munmap(addr);
> -	return vm_munmap(addr, len);
> +	return vm_munmap_zap_rlock(addr, len);
>  }
> 
> -
>  /*
>   * Emulation of deprecated remap_file_pages() syscall.
>   */
> @@ -3146,7 +3229,7 @@ void exit_mmap(struct mm_struct *mm)
>  	tlb_gather_mmu(&tlb, mm, 0, -1);
>  	/* update_hiwater_rss(mm) here? but nobody should be looking */
>  	/* Use -1 here to ensure all VMAs in the mm are unmapped */
> -	unmap_vmas(&tlb, vma, 0, -1);
> +	unmap_vmas(&tlb, vma, 0, -1, false);
>  	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
>  	tlb_finish_mmu(&tlb, 0, -1);
> 

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Yang Shi]

On 7/27/18 1:15 AM, Laurent Dufour wrote:
> On 26/07/2018 20:10, Yang Shi wrote:
>> When running some mmap/munmap scalability tests with large memory (i.e.
>>> 300GB), the below hung task issue may happen occasionally.
>> INFO: task ps:14018 blocked for more than 120 seconds.
>>         Tainted: G            E 4.9.79-009.ali3000.alios7.x86_64 #1
>>   "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
>> message.
>>   ps              D    0 14018      1 0x00000004
>>    ffff885582f84000 ffff885e8682f000 ffff880972943000 ffff885ebf499bc0
>>    ffff8828ee120000 ffffc900349bfca8 ffffffff817154d0 0000000000000040
>>    00ffffff812f872a ffff885ebf499bc0 024000d000948300 ffff880972943000
>>   Call Trace:
>>    [<ffffffff817154d0>] ? __schedule+0x250/0x730
>>    [<ffffffff817159e6>] schedule+0x36/0x80
>>    [<ffffffff81718560>] rwsem_down_read_failed+0xf0/0x150
>>    [<ffffffff81390a28>] call_rwsem_down_read_failed+0x18/0x30
>>    [<ffffffff81717db0>] down_read+0x20/0x40
>>    [<ffffffff812b9439>] proc_pid_cmdline_read+0xd9/0x4e0
>>    [<ffffffff81253c95>] ? do_filp_open+0xa5/0x100
>>    [<ffffffff81241d87>] __vfs_read+0x37/0x150
>>    [<ffffffff812f824b>] ? security_file_permission+0x9b/0xc0
>>    [<ffffffff81242266>] vfs_read+0x96/0x130
>>    [<ffffffff812437b5>] SyS_read+0x55/0xc0
>>    [<ffffffff8171a6da>] entry_SYSCALL_64_fastpath+0x1a/0xc5
>>
>> It is because munmap holds mmap_sem exclusively from very beginning to
>> all the way down to the end, and doesn't release it in the middle. When
>> unmapping large mapping, it may take long time (take ~18 seconds to
>> unmap 320GB mapping with every single page mapped on an idle machine).
>>
>> Zapping pages is the most time consuming part, according to the
>> suggestion from Michal Hocko [1], zapping pages can be done with holding
>> read mmap_sem, like what MADV_DONTNEED does. Then re-acquire write
>> mmap_sem to cleanup vmas.
>>
>> But, some part may need write mmap_sem, for example, vma splitting. So,
>> the design is as follows:
>>          acquire write mmap_sem
>>          lookup vmas (find and split vmas)
>> 	detach vmas
>>          deal with special mappings
>>          downgrade_write
>>
>>          zap pages
>> 	free page tables
>>          release mmap_sem
>>
>> The vm events with read mmap_sem may come in during page zapping, but
>> since vmas have been detached before, they, i.e. page fault, gup, etc,
>> will not be able to find valid vma, then just return SIGSEGV or -EFAULT
>> as expected.
>>
>> If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
>> considered as special mappings. They will be dealt with before zapping
>> pages with write mmap_sem held. Basically, just update vm_flags.
>>
>> And, since they are also manipulated by unmap_single_vma() which is
>> called by unmap_vma() with read mmap_sem held in this case, to
>> prevent from updating vm_flags in read critical section, a new
>> parameter, called "skip_flags" is added to unmap_region(), unmap_vmas()
>> and unmap_single_vma(). If it is true, then just skip unmap those
>> special mappings. Currently, the only place which pass true to this
>> parameter is us.
>>
>> With this approach we don't have to re-acquire mmap_sem again to clean
>> up vmas to avoid race window which might get the address space changed.
>>
>> And, since the lock acquire/release cost is managed to the minimum and
>> almost as same as before, the optimization could be extended to any size
>> of mapping without incurring significant penalty to small mappings.
>>
>> For the time being, just do this in munmap syscall path. Other
>> vm_munmap() or do_munmap() call sites (i.e mmap, mremap, etc) remain
>> intact for stability reason.
>>
>> With the patches, exclusive mmap_sem hold time when munmap a 80GB
>> address space on a machine with 32 cores of E5-2680 @ 2.70GHz dropped to
>> us level from second.
>>
>> munmap_test-15002 [008]   594.380138: funcgraph_entry: |  vm_munmap_zap_rlock() {
>> munmap_test-15002 [008]   594.380146: funcgraph_entry:      !2485684 us |    unmap_region();
>> munmap_test-15002 [008]   596.865836: funcgraph_exit:       !2485692 us |  }
>>
>> Here the excution time of unmap_region() is used to evaluate the time of
>> holding read mmap_sem, then the remaining time is used with holding
>> exclusive lock.
>>
>> [1] https://lwn.net/Articles/753269/
>>
>> Suggested-by: Michal Hocko <mhocko@kernel.org>
>> Suggested-by: Kirill A. Shutemov <kirill@shutemov.name>
>> Cc: Matthew Wilcox <willy@infradead.org>
>> Cc: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>> ---
>>   include/linux/mm.h |  2 +-
>>   mm/memory.c        | 41 ++++++++++++++++------
>>   mm/mmap.c          | 99 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>>   3 files changed, 123 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index a0fbb9f..e4480d8 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1321,7 +1321,7 @@ void zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
>>   void zap_page_range(struct vm_area_struct *vma, unsigned long address,
>>   		    unsigned long size);
>>   void unmap_vmas(struct mmu_gather *tlb, struct vm_area_struct *start_vma,
>> -		unsigned long start, unsigned long end);
>> +		unsigned long start, unsigned long end, bool skip_vm_flags);
>>
>>   /**
>>    * mm_walk - callbacks for walk_page_range
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 7206a63..6a772bd 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -1514,7 +1514,7 @@ void unmap_page_range(struct mmu_gather *tlb,
>>   static void unmap_single_vma(struct mmu_gather *tlb,
>>   		struct vm_area_struct *vma, unsigned long start_addr,
>>   		unsigned long end_addr,
>> -		struct zap_details *details)
>> +		struct zap_details *details, bool skip_vm_flags)
>>   {
>>   	unsigned long start = max(vma->vm_start, start_addr);
>>   	unsigned long end;
>> @@ -1525,11 +1525,19 @@ static void unmap_single_vma(struct mmu_gather *tlb,
>>   	if (end <= vma->vm_start)
>>   		return;
>>
>> -	if (vma->vm_file)
>> -		uprobe_munmap(vma, start, end);
>> +	/*
>> +	 * Since unmap_single_vma might be called with read mmap_sem held
>> +	 * in munmap optimization, so vm_flags can't be updated in this case.
>> +	 * They have been updated before this call with write mmap_sem held.
>> +	 * Here if skip_vm_flags is true, just skip the update.
>> +	 */
>> +	if (!skip_vm_flags) {
>> +		if (vma->vm_file)
>> +			uprobe_munmap(vma, start, end);
>>
>> -	if (unlikely(vma->vm_flags & VM_PFNMAP))
>> -		untrack_pfn(vma, 0, 0);
>> +		if (unlikely(vma->vm_flags & VM_PFNMAP))
>> +			untrack_pfn(vma, 0, 0);
>> +	}
>>
>>   	if (start != end) {
>>   		if (unlikely(is_vm_hugetlb_page(vma))) {
>> @@ -1546,7 +1554,19 @@ static void unmap_single_vma(struct mmu_gather *tlb,
>>   			 */
>>   			if (vma->vm_file) {
>>   				i_mmap_lock_write(vma->vm_file->f_mapping);
>> -				__unmap_hugepage_range_final(tlb, vma, start, end, NULL);
>> +				if (!skip_vm_flags) {
> As already reported by Mika : if (skip_vm_flags).
>
>> +					/*
>> +					 * The vma is being unmapped with read
>> +					 * mmap_sem.
>> +					 * Can't update vm_flags here, it has
>> +					 * been updated before this call with
>> +					 * write mmap_sem held.
>> +					 */
>> +					__unmap_hugepage_range(tlb, vma, start,
>> +							end, NULL);
>> +				} else
>> +					__unmap_hugepage_range_final(tlb, vma,
>> +							start, end, NULL);
>>   				i_mmap_unlock_write(vma->vm_file->f_mapping);
>>   			}
>>   		} else
>> @@ -1574,13 +1594,14 @@ static void unmap_single_vma(struct mmu_gather *tlb,
>>    */
>>   void unmap_vmas(struct mmu_gather *tlb,
>>   		struct vm_area_struct *vma, unsigned long start_addr,
>> -		unsigned long end_addr)
>> +		unsigned long end_addr, bool skip_vm_flags)
>>   {
>>   	struct mm_struct *mm = vma->vm_mm;
>>
>>   	mmu_notifier_invalidate_range_start(mm, start_addr, end_addr);
>>   	for ( ; vma && vma->vm_start < end_addr; vma = vma->vm_next)
>> -		unmap_single_vma(tlb, vma, start_addr, end_addr, NULL);
>> +		unmap_single_vma(tlb, vma, start_addr, end_addr, NULL,
>> +				 skip_vm_flags);
>>   	mmu_notifier_invalidate_range_end(mm, start_addr, end_addr);
>>   }
>>
>> @@ -1604,7 +1625,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, NULL);
>> +		unmap_single_vma(&tlb, vma, start, end, NULL, false);
>>
>>   		/*
>>   		 * zap_page_range does not specify whether mmap_sem should be
>> @@ -1641,7 +1662,7 @@ static void zap_page_range_single(struct vm_area_struct *vma, unsigned long addr
>>   	tlb_gather_mmu(&tlb, mm, address, end);
>>   	update_hiwater_rss(mm);
>>   	mmu_notifier_invalidate_range_start(mm, address, end);
>> -	unmap_single_vma(&tlb, vma, address, end, details);
>> +	unmap_single_vma(&tlb, vma, address, end, details, false);
>>   	mmu_notifier_invalidate_range_end(mm, address, end);
>>   	tlb_finish_mmu(&tlb, address, end);
>>   }
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 2504094..663a0c5 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -73,7 +73,7 @@
>>
>>   static void unmap_region(struct mm_struct *mm,
>>   		struct vm_area_struct *vma, struct vm_area_struct *prev,
>> -		unsigned long start, unsigned long end);
>> +		unsigned long start, unsigned long end, bool skip_flags);
> Earlier, you used the name 'skip_vm_flags'. It would be nice to keep the same
> parameter name everywhere, isn't it ?

Yes, sure.

>
>>   /* description of effects of mapping type and prot in current implementation.
>>    * this is due to the limited x86 page protection hardware.  The expected
>> @@ -1824,7 +1824,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>>   	fput(file);
>>
>>   	/* Undo any partial mapping done by a device driver. */
>> -	unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
>> +	unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end, false);
>>   	charged = 0;
>>   	if (vm_flags & VM_SHARED)
>>   		mapping_unmap_writable(file->f_mapping);
>> @@ -2559,7 +2559,7 @@ static void remove_vma_list(struct mm_struct *mm, struct vm_area_struct *vma)
>>    */
>>   static void unmap_region(struct mm_struct *mm,
>>   		struct vm_area_struct *vma, struct vm_area_struct *prev,
>> -		unsigned long start, unsigned long end)
>> +		unsigned long start, unsigned long end, bool skip_flags)
> Here too.
>
>>   {
>>   	struct vm_area_struct *next = prev ? prev->vm_next : mm->mmap;
>>   	struct mmu_gather tlb;
>> @@ -2567,7 +2567,7 @@ static void unmap_region(struct mm_struct *mm,
>>   	lru_add_drain();
>>   	tlb_gather_mmu(&tlb, mm, start, end);
>>   	update_hiwater_rss(mm);
>> -	unmap_vmas(&tlb, vma, start, end);
>> +	unmap_vmas(&tlb, vma, start, end, skip_flags);
>>   	free_pgtables(&tlb, vma, prev ? prev->vm_end : FIRST_USER_ADDRESS,
>>   				 next ? next->vm_start : USER_PGTABLES_CEILING);
>>   	tlb_finish_mmu(&tlb, start, end);
>> @@ -2778,6 +2778,79 @@ static inline void munmap_mlock_vma(struct vm_area_struct *vma,
>>   	}
>>   }
>>
>> +/*
>> + * Zap pages with read mmap_sem held
>> + *
>> + * uf is the list for userfaultfd
>> + */
>> +static int do_munmap_zap_rlock(struct mm_struct *mm, unsigned long start,
>> +			       size_t len, struct list_head *uf)
>> +{
>> +	unsigned long end = 0;
>> +	struct vm_area_struct *start_vma = NULL, *prev, *vma;
>> +	int ret = 0;
> No need to initialize end, start_vma and ret here, they will be assigned before
> used.

OK

>
>> +
>> +	if (!munmap_addr_sanity(start, len))
>> +		return -EINVAL;
>> +
>> +	len = PAGE_ALIGN(len);
>> +
>> +	end = start + len;
>> +
>> +	/*
>> +	 * Need write mmap_sem to split vmas and detach vmas
>> +	 * splitting vma up-front to save PITA to clean if it is failed
>> +	 */
>> +	if (down_write_killable(&mm->mmap_sem))
>> +		return -EINTR;
>> +
>> +	ret = munmap_lookup_vma(mm, &start_vma, &prev, start, end);
>> +	if (ret != 1)
>> +		goto out;
>> +
>> +	if (unlikely(uf)) {
>> +		ret = userfaultfd_unmap_prep(start_vma, start, end, uf);
>> +		if (ret)
>> +			goto out;
>> +	}
>> +
>> +	/* Handle mlocked vmas */
>> +	if (mm->locked_vm)
>> +		munmap_mlock_vma(start_vma, end);
>> +
>> +	/* Detach vmas from rbtree */
>> +	detach_vmas_to_be_unmapped(mm, start_vma, prev, end);
>> +
>> +	/*
>> +	 * Clear uprobe, VM_PFNMAP and hugetlb mapping in advance since they
>> +	 * need update vm_flags with write mmap_sem
>> +	 */
>> +	vma = start_vma;
>> +	for ( ; vma && vma->vm_start < end; vma = vma->vm_next) {
> Not critical, but 'vma = start_vma' should be part of the init stuff in for(),
> like this:
> 	for (vma = start_vma; vma && vma->vm_start < end; vma = vma->vm_next) {

OK

Thanks for reviewing these patches. Will fix these comments in next 
version. Before I prepare the next version, I would like to wait for one 
or two days to see if anyone else has more comments.

Andrew & Michal,

Do you have any comment on this version?

Thanks,
Yang

>
>> +		if (vma->vm_file)
>> +			uprobe_munmap(vma, vma->vm_start, vma->vm_end);
>> +		if (unlikely(vma->vm_flags & VM_PFNMAP))
>> +			untrack_pfn(vma, 0, 0);
>> +		if (is_vm_hugetlb_page(vma))
>> +			vma->vm_flags &= ~VM_MAYSHARE;
>> +	}
>> +
>> +	downgrade_write(&mm->mmap_sem);
>> +
>> +	/* Zap mappings with read mmap_sem */
>> +	unmap_region(mm, start_vma, prev, start, end, true);
>> +
>> +	arch_unmap(mm, start_vma, start, end);
>> +	remove_vma_list(mm, start_vma);
>> +	up_read(&mm->mmap_sem);
>> +
>> +	return 0;
>> +
>> +out:
>> +	up_write(&mm->mmap_sem);
>> +	return ret;
>> +}
>> +
>>   /* Munmap is split into 2 main parts -- this part which finds
>>    * what needs doing, and the areas themselves, which do the
>>    * work.  This now handles partial unmappings.
>> @@ -2826,7 +2899,7 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>>   	 * Remove the vma's, and unmap the actual pages
>>   	 */
>>   	detach_vmas_to_be_unmapped(mm, vma, prev, end);
>> -	unmap_region(mm, vma, prev, start, end);
>> +	unmap_region(mm, vma, prev, start, end, false);
>>
>>   	arch_unmap(mm, vma, start, end);
>>
>> @@ -2836,6 +2909,17 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>>   	return 0;
>>   }
>>
>> +static int vm_munmap_zap_rlock(unsigned long start, size_t len)
>> +{
>> +	int ret;
>> +	struct mm_struct *mm = current->mm;
>> +	LIST_HEAD(uf);
>> +
>> +	ret = do_munmap_zap_rlock(mm, start, len, &uf);
>> +	userfaultfd_unmap_complete(mm, &uf);
>> +	return ret;
>> +}
>> +
>>   int vm_munmap(unsigned long start, size_t len)
>>   {
>>   	int ret;
>> @@ -2855,10 +2939,9 @@ int vm_munmap(unsigned long start, size_t len)
>>   SYSCALL_DEFINE2(munmap, unsigned long, addr, size_t, len)
>>   {
>>   	profile_munmap(addr);
>> -	return vm_munmap(addr, len);
>> +	return vm_munmap_zap_rlock(addr, len);
>>   }
>>
>> -
>>   /*
>>    * Emulation of deprecated remap_file_pages() syscall.
>>    */
>> @@ -3146,7 +3229,7 @@ void exit_mmap(struct mm_struct *mm)
>>   	tlb_gather_mmu(&tlb, mm, 0, -1);
>>   	/* update_hiwater_rss(mm) here? but nobody should be looking */
>>   	/* Use -1 here to ensure all VMAs in the mm are unmapped */
>> -	unmap_vmas(&tlb, vma, 0, -1);
>> +	unmap_vmas(&tlb, vma, 0, -1, false);
>>   	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
>>   	tlb_finish_mmu(&tlb, 0, -1);
>>

Re: [RFC v6 PATCH 1/2] mm: refactor do_munmap() to extract the common part

[Michal Hocko]

On Fri 27-07-18 02:10:13, Yang Shi wrote:
> Introduces three new helper functions:
>   * munmap_addr_sanity()
>   * munmap_lookup_vma()
>   * munmap_mlock_vma()
> 
> They will be used by do_munmap() and the new do_munmap with zapping
> large mapping early in the later patch.
> 
> There is no functional change, just code refactor.
> 
> Reviewed-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> ---
>  mm/mmap.c | 120 ++++++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 82 insertions(+), 38 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index d1eb87e..2504094 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2686,34 +2686,44 @@ int split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
>  	return __split_vma(mm, vma, addr, new_below);
>  }
>  
> -/* Munmap is split into 2 main parts -- this part which finds
> - * what needs doing, and the areas themselves, which do the
> - * work.  This now handles partial unmappings.
> - * Jeremy Fitzhardinge <jeremy@goop.org>
> - */
> -int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
> -	      struct list_head *uf)
> +static inline bool munmap_addr_sanity(unsigned long start, size_t len)

munmap_check_addr? Btw. why does this need to have munmap prefix at all?
This is a general address space check.

>  {
> -	unsigned long end;
> -	struct vm_area_struct *vma, *prev, *last;
> -
>  	if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start)
> -		return -EINVAL;
> +		return false;
>  
> -	len = PAGE_ALIGN(len);
> -	if (len == 0)
> -		return -EINVAL;
> +	if (PAGE_ALIGN(len) == 0)
> +		return false;
> +
> +	return true;
> +}
> +
> +/*
> + * munmap_lookup_vma: find the first overlap vma and split overlap vmas.
> + * @mm: mm_struct
> + * @vma: the first overlapping vma
> + * @prev: vma's prev
> + * @start: start address
> + * @end: end address

This really doesn't help me to understand how to use the function.
Why do we need both prev and vma etc...

> + *
> + * returns 1 if successful, 0 or errno otherwise

This is a really weird calling convention. So what does 0 tell? /me
checks the code. Ohh, it is nothing to do. Why cannot you simply return
the vma. NULL implies nothing to do, ERR_PTR on error.

> + */
> +static int munmap_lookup_vma(struct mm_struct *mm, struct vm_area_struct **vma,
> +			     struct vm_area_struct **prev, unsigned long start,
> +			     unsigned long end)
> +{
> +	struct vm_area_struct *tmp, *last;
>  
>  	/* Find the first overlapping VMA */
> -	vma = find_vma(mm, start);
> -	if (!vma)
> +	tmp = find_vma(mm, start);
> +	if (!tmp)
>  		return 0;
> -	prev = vma->vm_prev;
> -	/* we have  start < vma->vm_end  */
> +
> +	*prev = tmp->vm_prev;

Why do you set prev here. We might "fail" with 0 right after this

> +
> +	/* we have start < vma->vm_end  */
>  
>  	/* if it doesn't overlap, we have nothing.. */
> -	end = start + len;
> -	if (vma->vm_start >= end)
> +	if (tmp->vm_start >= end)
>  		return 0;
>  
>  	/*
> @@ -2723,7 +2733,7 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>  	 * unmapped vm_area_struct will remain in use: so lower split_vma
>  	 * places tmp vma above, and higher split_vma places tmp vma below.
>  	 */
> -	if (start > vma->vm_start) {
> +	if (start > tmp->vm_start) {
>  		int error;
>  
>  		/*
> @@ -2731,13 +2741,14 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>  		 * not exceed its limit; but let map_count go just above
>  		 * its limit temporarily, to help free resources as expected.
>  		 */
> -		if (end < vma->vm_end && mm->map_count >= sysctl_max_map_count)
> +		if (end < tmp->vm_end &&
> +		    mm->map_count > sysctl_max_map_count)
>  			return -ENOMEM;
>  
> -		error = __split_vma(mm, vma, start, 0);
> +		error = __split_vma(mm, tmp, start, 0);
>  		if (error)
>  			return error;
> -		prev = vma;
> +		*prev = tmp;
>  	}
>  
>  	/* Does it split the last one? */
> @@ -2747,7 +2758,48 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>  		if (error)
>  			return error;
>  	}
> -	vma = prev ? prev->vm_next : mm->mmap;
> +
> +	*vma = *prev ? (*prev)->vm_next : mm->mmap;
> +
> +	return 1;
> +}

the patch would be much more easier to read if you didn't do vma->tmp
renaming.

-- 
Michal Hocko
SUSE Labs

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Michal Hocko]

On Fri 27-07-18 02:10:14, Yang Shi wrote:
> When running some mmap/munmap scalability tests with large memory (i.e.
> > 300GB), the below hung task issue may happen occasionally.
> 
> INFO: task ps:14018 blocked for more than 120 seconds.
>        Tainted: G            E 4.9.79-009.ali3000.alios7.x86_64 #1
>  "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
> message.
>  ps              D    0 14018      1 0x00000004
>   ffff885582f84000 ffff885e8682f000 ffff880972943000 ffff885ebf499bc0
>   ffff8828ee120000 ffffc900349bfca8 ffffffff817154d0 0000000000000040
>   00ffffff812f872a ffff885ebf499bc0 024000d000948300 ffff880972943000
>  Call Trace:
>   [<ffffffff817154d0>] ? __schedule+0x250/0x730
>   [<ffffffff817159e6>] schedule+0x36/0x80
>   [<ffffffff81718560>] rwsem_down_read_failed+0xf0/0x150
>   [<ffffffff81390a28>] call_rwsem_down_read_failed+0x18/0x30
>   [<ffffffff81717db0>] down_read+0x20/0x40
>   [<ffffffff812b9439>] proc_pid_cmdline_read+0xd9/0x4e0
>   [<ffffffff81253c95>] ? do_filp_open+0xa5/0x100
>   [<ffffffff81241d87>] __vfs_read+0x37/0x150
>   [<ffffffff812f824b>] ? security_file_permission+0x9b/0xc0
>   [<ffffffff81242266>] vfs_read+0x96/0x130
>   [<ffffffff812437b5>] SyS_read+0x55/0xc0
>   [<ffffffff8171a6da>] entry_SYSCALL_64_fastpath+0x1a/0xc5
> 
> It is because munmap holds mmap_sem exclusively from very beginning to
> all the way down to the end, and doesn't release it in the middle. When
> unmapping large mapping, it may take long time (take ~18 seconds to
> unmap 320GB mapping with every single page mapped on an idle machine).
> 
> Zapping pages is the most time consuming part, according to the
> suggestion from Michal Hocko [1], zapping pages can be done with holding
> read mmap_sem, like what MADV_DONTNEED does. Then re-acquire write
> mmap_sem to cleanup vmas.
> 
> But, some part may need write mmap_sem, for example, vma splitting. So,
> the design is as follows:
>         acquire write mmap_sem
>         lookup vmas (find and split vmas)
> 	detach vmas
>         deal with special mappings
>         downgrade_write
> 
>         zap pages
> 	free page tables
>         release mmap_sem
> 
> The vm events with read mmap_sem may come in during page zapping, but
> since vmas have been detached before, they, i.e. page fault, gup, etc,
> will not be able to find valid vma, then just return SIGSEGV or -EFAULT
> as expected.
> 
> If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
> considered as special mappings. They will be dealt with before zapping
> pages with write mmap_sem held. Basically, just update vm_flags.

Well, I think it would be safer to simply fallback to the current
implementation with these mappings and deal with them on top. This would
make potential issues easier to bisect and partial reverts as well.

> And, since they are also manipulated by unmap_single_vma() which is
> called by unmap_vma() with read mmap_sem held in this case, to
> prevent from updating vm_flags in read critical section, a new
> parameter, called "skip_flags" is added to unmap_region(), unmap_vmas()
> and unmap_single_vma(). If it is true, then just skip unmap those
> special mappings. Currently, the only place which pass true to this
> parameter is us.

skip parameters are usually ugly and lead to more mess later on. Can we
do without them?

> With this approach we don't have to re-acquire mmap_sem again to clean
> up vmas to avoid race window which might get the address space changed.

By with this approach you mean detaching right?

> And, since the lock acquire/release cost is managed to the minimum and
> almost as same as before, the optimization could be extended to any size
> of mapping without incurring significant penalty to small mappings.

I guess you mean to say that lock downgrade approach doesn't lead to
regressions because the overal time mmap_sem is taken is not longer?

> For the time being, just do this in munmap syscall path. Other
> vm_munmap() or do_munmap() call sites (i.e mmap, mremap, etc) remain
> intact for stability reason.

You have used this argument previously and several people have asked.
I think it is just wrong. Either the concept is safe and all callers can
use it or it is not and then those subtle differences should be called
out. Your previous response was that you simply haven't tested other
paths. Well, that is not an argument, I am afraid. The whole thing
should be done at a proper layer. If there are some difficulties to
achieve that for all callers then OK just be explicit about that. I can
imagine some callers really require the exclusive look when munmap
returns for example.

> With the patches, exclusive mmap_sem hold time when munmap a 80GB
> address space on a machine with 32 cores of E5-2680 @ 2.70GHz dropped to
> us level from second.
> 
> munmap_test-15002 [008]   594.380138: funcgraph_entry: |  vm_munmap_zap_rlock() {
> munmap_test-15002 [008]   594.380146: funcgraph_entry:      !2485684 us |    unmap_region();
> munmap_test-15002 [008]   596.865836: funcgraph_exit:       !2485692 us |  }
> 
> Here the excution time of unmap_region() is used to evaluate the time of
> holding read mmap_sem, then the remaining time is used with holding
> exclusive lock.

I will be reading through the patch and follow up on that separately.
-- 
Michal Hocko
SUSE Labs

Re: [RFC v6 PATCH 1/2] mm: refactor do_munmap() to extract the common part

[Yang Shi]

On 8/3/18 1:53 AM, Michal Hocko wrote:
> On Fri 27-07-18 02:10:13, Yang Shi wrote:
>> Introduces three new helper functions:
>>    * munmap_addr_sanity()
>>    * munmap_lookup_vma()
>>    * munmap_mlock_vma()
>>
>> They will be used by do_munmap() and the new do_munmap with zapping
>> large mapping early in the later patch.
>>
>> There is no functional change, just code refactor.
>>
>> Reviewed-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>> ---
>>   mm/mmap.c | 120 ++++++++++++++++++++++++++++++++++++++++++--------------------
>>   1 file changed, 82 insertions(+), 38 deletions(-)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index d1eb87e..2504094 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -2686,34 +2686,44 @@ int split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
>>   	return __split_vma(mm, vma, addr, new_below);
>>   }
>>   
>> -/* Munmap is split into 2 main parts -- this part which finds
>> - * what needs doing, and the areas themselves, which do the
>> - * work.  This now handles partial unmappings.
>> - * Jeremy Fitzhardinge <jeremy@goop.org>
>> - */
>> -int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>> -	      struct list_head *uf)
>> +static inline bool munmap_addr_sanity(unsigned long start, size_t len)
> munmap_check_addr? Btw. why does this need to have munmap prefix at all?
> This is a general address space check.

Just because I extracted this from do_munmap, no special consideration. 
It is definitely ok to use another name.

>
>>   {
>> -	unsigned long end;
>> -	struct vm_area_struct *vma, *prev, *last;
>> -
>>   	if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start)
>> -		return -EINVAL;
>> +		return false;
>>   
>> -	len = PAGE_ALIGN(len);
>> -	if (len == 0)
>> -		return -EINVAL;
>> +	if (PAGE_ALIGN(len) == 0)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +/*
>> + * munmap_lookup_vma: find the first overlap vma and split overlap vmas.
>> + * @mm: mm_struct
>> + * @vma: the first overlapping vma
>> + * @prev: vma's prev
>> + * @start: start address
>> + * @end: end address
> This really doesn't help me to understand how to use the function.
> Why do we need both prev and vma etc...

prev will be used by unmap_region later.

>
>> + *
>> + * returns 1 if successful, 0 or errno otherwise
> This is a really weird calling convention. So what does 0 tell? /me
> checks the code. Ohh, it is nothing to do. Why cannot you simply return
> the vma. NULL implies nothing to do, ERR_PTR on error.

A couple of reasons why it is implemented as so:

     * do_munmap returns 0 for both success and no suitable vma

     * Since prev is needed by finding the start vma, and prev will be 
used by unmap_region later too, so I just thought it would look clean to 
have one function to return both start vma and prev. In this way, we can 
share as much as possible common code.

     * In this way, we just need return 0, 1 or error no just as same as 
what do_munmap does currently. Then we know what is failure case exactly 
to just bail out right away.

Actually, I tried the same approach as you suggested, but it had two 
problems:

     * If it returns the start vma, we have to re-find its prev later, 
but the prev has been found during finding start vma. And, duplicate the 
code in do_munmap_zap_rlock. It sounds not that ideal.

     * If it returns prev, it might be null (start vma is the first 
vma). We can't tell if null is a failure or success case

>
>> + */
>> +static int munmap_lookup_vma(struct mm_struct *mm, struct vm_area_struct **vma,
>> +			     struct vm_area_struct **prev, unsigned long start,
>> +			     unsigned long end)
>> +{
>> +	struct vm_area_struct *tmp, *last;
>>   
>>   	/* Find the first overlapping VMA */
>> -	vma = find_vma(mm, start);
>> -	if (!vma)
>> +	tmp = find_vma(mm, start);
>> +	if (!tmp)
>>   		return 0;
>> -	prev = vma->vm_prev;
>> -	/* we have  start < vma->vm_end  */
>> +
>> +	*prev = tmp->vm_prev;
> Why do you set prev here. We might "fail" with 0 right after this

No special reason, just copied from do_munmap. Yes, it is ideal to have 
prev set here. It can be moved further down.

>
>> +
>> +	/* we have start < vma->vm_end  */
>>   
>>   	/* if it doesn't overlap, we have nothing.. */
>> -	end = start + len;
>> -	if (vma->vm_start >= end)
>> +	if (tmp->vm_start >= end)
>>   		return 0;
>>   
>>   	/*
>> @@ -2723,7 +2733,7 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>>   	 * unmapped vm_area_struct will remain in use: so lower split_vma
>>   	 * places tmp vma above, and higher split_vma places tmp vma below.
>>   	 */
>> -	if (start > vma->vm_start) {
>> +	if (start > tmp->vm_start) {
>>   		int error;
>>   
>>   		/*
>> @@ -2731,13 +2741,14 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>>   		 * not exceed its limit; but let map_count go just above
>>   		 * its limit temporarily, to help free resources as expected.
>>   		 */
>> -		if (end < vma->vm_end && mm->map_count >= sysctl_max_map_count)
>> +		if (end < tmp->vm_end &&
>> +		    mm->map_count > sysctl_max_map_count)
>>   			return -ENOMEM;
>>   
>> -		error = __split_vma(mm, vma, start, 0);
>> +		error = __split_vma(mm, tmp, start, 0);
>>   		if (error)
>>   			return error;
>> -		prev = vma;
>> +		*prev = tmp;
>>   	}
>>   
>>   	/* Does it split the last one? */
>> @@ -2747,7 +2758,48 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>>   		if (error)
>>   			return error;
>>   	}
>> -	vma = prev ? prev->vm_next : mm->mmap;
>> +
>> +	*vma = *prev ? (*prev)->vm_next : mm->mmap;
>> +
>> +	return 1;
>> +}
> the patch would be much more easier to read if you didn't do vma->tmp
> renaming.

Yes, I should used another name for the "vma" argument.

Thanks,
Yang

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Yang Shi]

On 8/3/18 2:07 AM, Michal Hocko wrote:
> On Fri 27-07-18 02:10:14, Yang Shi wrote:
>> When running some mmap/munmap scalability tests with large memory (i.e.
>>> 300GB), the below hung task issue may happen occasionally.
>> INFO: task ps:14018 blocked for more than 120 seconds.
>>         Tainted: G            E 4.9.79-009.ali3000.alios7.x86_64 #1
>>   "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
>> message.
>>   ps              D    0 14018      1 0x00000004
>>    ffff885582f84000 ffff885e8682f000 ffff880972943000 ffff885ebf499bc0
>>    ffff8828ee120000 ffffc900349bfca8 ffffffff817154d0 0000000000000040
>>    00ffffff812f872a ffff885ebf499bc0 024000d000948300 ffff880972943000
>>   Call Trace:
>>    [<ffffffff817154d0>] ? __schedule+0x250/0x730
>>    [<ffffffff817159e6>] schedule+0x36/0x80
>>    [<ffffffff81718560>] rwsem_down_read_failed+0xf0/0x150
>>    [<ffffffff81390a28>] call_rwsem_down_read_failed+0x18/0x30
>>    [<ffffffff81717db0>] down_read+0x20/0x40
>>    [<ffffffff812b9439>] proc_pid_cmdline_read+0xd9/0x4e0
>>    [<ffffffff81253c95>] ? do_filp_open+0xa5/0x100
>>    [<ffffffff81241d87>] __vfs_read+0x37/0x150
>>    [<ffffffff812f824b>] ? security_file_permission+0x9b/0xc0
>>    [<ffffffff81242266>] vfs_read+0x96/0x130
>>    [<ffffffff812437b5>] SyS_read+0x55/0xc0
>>    [<ffffffff8171a6da>] entry_SYSCALL_64_fastpath+0x1a/0xc5
>>
>> It is because munmap holds mmap_sem exclusively from very beginning to
>> all the way down to the end, and doesn't release it in the middle. When
>> unmapping large mapping, it may take long time (take ~18 seconds to
>> unmap 320GB mapping with every single page mapped on an idle machine).
>>
>> Zapping pages is the most time consuming part, according to the
>> suggestion from Michal Hocko [1], zapping pages can be done with holding
>> read mmap_sem, like what MADV_DONTNEED does. Then re-acquire write
>> mmap_sem to cleanup vmas.
>>
>> But, some part may need write mmap_sem, for example, vma splitting. So,
>> the design is as follows:
>>          acquire write mmap_sem
>>          lookup vmas (find and split vmas)
>> 	detach vmas
>>          deal with special mappings
>>          downgrade_write
>>
>>          zap pages
>> 	free page tables
>>          release mmap_sem
>>
>> The vm events with read mmap_sem may come in during page zapping, but
>> since vmas have been detached before, they, i.e. page fault, gup, etc,
>> will not be able to find valid vma, then just return SIGSEGV or -EFAULT
>> as expected.
>>
>> If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
>> considered as special mappings. They will be dealt with before zapping
>> pages with write mmap_sem held. Basically, just update vm_flags.
> Well, I think it would be safer to simply fallback to the current
> implementation with these mappings and deal with them on top. This would
> make potential issues easier to bisect and partial reverts as well.

Do you mean just call do_munmap()? It sounds ok. Although we may waste 
some cycles to repeat what has done, it sounds not too bad since those 
special mappings should be not very common.

>
>> And, since they are also manipulated by unmap_single_vma() which is
>> called by unmap_vma() with read mmap_sem held in this case, to
>> prevent from updating vm_flags in read critical section, a new
>> parameter, called "skip_flags" is added to unmap_region(), unmap_vmas()
>> and unmap_single_vma(). If it is true, then just skip unmap those
>> special mappings. Currently, the only place which pass true to this
>> parameter is us.
> skip parameters are usually ugly and lead to more mess later on. Can we
> do without them?

We need a way to tell unmap_region() that it is called in a kind of 
special context which updating vm_flags is not allowed. I didn't think 
of a better way.

We could add a new API to do what unmap_region() does without updating 
vm_flags, but we would have to  duplicate some code.

>
>> With this approach we don't have to re-acquire mmap_sem again to clean
>> up vmas to avoid race window which might get the address space changed.
> By with this approach you mean detaching right?

Yes, the detaching approach.

>
>> And, since the lock acquire/release cost is managed to the minimum and
>> almost as same as before, the optimization could be extended to any size
>> of mapping without incurring significant penalty to small mappings.
> I guess you mean to say that lock downgrade approach doesn't lead to
> regressions because the overal time mmap_sem is taken is not longer?

Yes. And, there is not lock take/retake cost since we don't release it.

>
>> For the time being, just do this in munmap syscall path. Other
>> vm_munmap() or do_munmap() call sites (i.e mmap, mremap, etc) remain
>> intact for stability reason.
> You have used this argument previously and several people have asked.
> I think it is just wrong. Either the concept is safe and all callers can
> use it or it is not and then those subtle differences should be called
> out. Your previous response was that you simply haven't tested other
> paths. Well, that is not an argument, I am afraid. The whole thing
> should be done at a proper layer. If there are some difficulties to
> achieve that for all callers then OK just be explicit about that. I can
> imagine some callers really require the exclusive look when munmap
> returns for example.

Yes, the statement here sounds ambiguous. There are definitely some 
difficulties to achieve that in mmap and mremap. Since they acquire 
write mmap_sem at the very beginning, then do their stuff, which may 
call do_munmap if overlapped address space has to be changed.

But, the optimized do_munmap would like to be called without mmap_sem 
held so that we can do the optimization. So, if we want to do the 
similar optimization for mmap/mremap path, I'm afraid we would have to 
redesign them.

I assumes munmap itself is the main source of the latency issue. 
mmap/mremap might hit the latency problem if they are trying to map or 
remap a huge overlapped address space, but it should be rare. So, I 
leave them untouched.

>
>> With the patches, exclusive mmap_sem hold time when munmap a 80GB
>> address space on a machine with 32 cores of E5-2680 @ 2.70GHz dropped to
>> us level from second.
>>
>> munmap_test-15002 [008]   594.380138: funcgraph_entry: |  vm_munmap_zap_rlock() {
>> munmap_test-15002 [008]   594.380146: funcgraph_entry:      !2485684 us |    unmap_region();
>> munmap_test-15002 [008]   596.865836: funcgraph_exit:       !2485692 us |  }
>>
>> Here the excution time of unmap_region() is used to evaluate the time of
>> holding read mmap_sem, then the remaining time is used with holding
>> exclusive lock.
> I will be reading through the patch and follow up on that separately.

Thanks,
Yang

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Michal Hocko]

On Fri 03-08-18 14:01:58, Yang Shi wrote:
> 
> 
> On 8/3/18 2:07 AM, Michal Hocko wrote:
> > On Fri 27-07-18 02:10:14, Yang Shi wrote:
> > > When running some mmap/munmap scalability tests with large memory (i.e.
> > > > 300GB), the below hung task issue may happen occasionally.
> > > INFO: task ps:14018 blocked for more than 120 seconds.
> > >         Tainted: G            E 4.9.79-009.ali3000.alios7.x86_64 #1
> > >   "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
> > > message.
> > >   ps              D    0 14018      1 0x00000004
> > >    ffff885582f84000 ffff885e8682f000 ffff880972943000 ffff885ebf499bc0
> > >    ffff8828ee120000 ffffc900349bfca8 ffffffff817154d0 0000000000000040
> > >    00ffffff812f872a ffff885ebf499bc0 024000d000948300 ffff880972943000
> > >   Call Trace:
> > >    [<ffffffff817154d0>] ? __schedule+0x250/0x730
> > >    [<ffffffff817159e6>] schedule+0x36/0x80
> > >    [<ffffffff81718560>] rwsem_down_read_failed+0xf0/0x150
> > >    [<ffffffff81390a28>] call_rwsem_down_read_failed+0x18/0x30
> > >    [<ffffffff81717db0>] down_read+0x20/0x40
> > >    [<ffffffff812b9439>] proc_pid_cmdline_read+0xd9/0x4e0
> > >    [<ffffffff81253c95>] ? do_filp_open+0xa5/0x100
> > >    [<ffffffff81241d87>] __vfs_read+0x37/0x150
> > >    [<ffffffff812f824b>] ? security_file_permission+0x9b/0xc0
> > >    [<ffffffff81242266>] vfs_read+0x96/0x130
> > >    [<ffffffff812437b5>] SyS_read+0x55/0xc0
> > >    [<ffffffff8171a6da>] entry_SYSCALL_64_fastpath+0x1a/0xc5
> > > 
> > > It is because munmap holds mmap_sem exclusively from very beginning to
> > > all the way down to the end, and doesn't release it in the middle. When
> > > unmapping large mapping, it may take long time (take ~18 seconds to
> > > unmap 320GB mapping with every single page mapped on an idle machine).
> > > 
> > > Zapping pages is the most time consuming part, according to the
> > > suggestion from Michal Hocko [1], zapping pages can be done with holding
> > > read mmap_sem, like what MADV_DONTNEED does. Then re-acquire write
> > > mmap_sem to cleanup vmas.
> > > 
> > > But, some part may need write mmap_sem, for example, vma splitting. So,
> > > the design is as follows:
> > >          acquire write mmap_sem
> > >          lookup vmas (find and split vmas)
> > > 	detach vmas
> > >          deal with special mappings
> > >          downgrade_write
> > > 
> > >          zap pages
> > > 	free page tables
> > >          release mmap_sem
> > > 
> > > The vm events with read mmap_sem may come in during page zapping, but
> > > since vmas have been detached before, they, i.e. page fault, gup, etc,
> > > will not be able to find valid vma, then just return SIGSEGV or -EFAULT
> > > as expected.
> > > 
> > > If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
> > > considered as special mappings. They will be dealt with before zapping
> > > pages with write mmap_sem held. Basically, just update vm_flags.
> > Well, I think it would be safer to simply fallback to the current
> > implementation with these mappings and deal with them on top. This would
> > make potential issues easier to bisect and partial reverts as well.
> 
> Do you mean just call do_munmap()? It sounds ok. Although we may waste some
> cycles to repeat what has done, it sounds not too bad since those special
> mappings should be not very common.

VM_HUGETLB is quite spread. Especially for DB workloads.

> > > And, since they are also manipulated by unmap_single_vma() which is
> > > called by unmap_vma() with read mmap_sem held in this case, to
> > > prevent from updating vm_flags in read critical section, a new
> > > parameter, called "skip_flags" is added to unmap_region(), unmap_vmas()
> > > and unmap_single_vma(). If it is true, then just skip unmap those
> > > special mappings. Currently, the only place which pass true to this
> > > parameter is us.
> > skip parameters are usually ugly and lead to more mess later on. Can we
> > do without them?
> 
> We need a way to tell unmap_region() that it is called in a kind of special
> context which updating vm_flags is not allowed. I didn't think of a better
> way.
> 
> We could add a new API to do what unmap_region() does without updating
> vm_flags, but we would have to  duplicate some code.

I really didn't get to think about a better way myself but I strongly
suspect we can do without special hacks here. Is updating flags under
read lock a real problem? Assuming that special mappings are not really
considered at this stage.

> > > With this approach we don't have to re-acquire mmap_sem again to clean
> > > up vmas to avoid race window which might get the address space changed.
> > By with this approach you mean detaching right?
> 
> Yes, the detaching approach.

Please make it explicit in the changelog.
 
> > > And, since the lock acquire/release cost is managed to the minimum and
> > > almost as same as before, the optimization could be extended to any size
> > > of mapping without incurring significant penalty to small mappings.
> > I guess you mean to say that lock downgrade approach doesn't lead to
> > regressions because the overal time mmap_sem is taken is not longer?
> 
> Yes. And, there is not lock take/retake cost since we don't release it.

Please also be explicit.
 
> > > For the time being, just do this in munmap syscall path. Other
> > > vm_munmap() or do_munmap() call sites (i.e mmap, mremap, etc) remain
> > > intact for stability reason.
> > You have used this argument previously and several people have asked.
> > I think it is just wrong. Either the concept is safe and all callers can
> > use it or it is not and then those subtle differences should be called
> > out. Your previous response was that you simply haven't tested other
> > paths. Well, that is not an argument, I am afraid. The whole thing
> > should be done at a proper layer. If there are some difficulties to
> > achieve that for all callers then OK just be explicit about that. I can
> > imagine some callers really require the exclusive look when munmap
> > returns for example.
> 
> Yes, the statement here sounds ambiguous. There are definitely some
> difficulties to achieve that in mmap and mremap. Since they acquire write
> mmap_sem at the very beginning, then do their stuff, which may call
> do_munmap if overlapped address space has to be changed.

Do call them out. Maybe even add a comment in the code so that people
who would like those other paths know what they need to look at.

> But, the optimized do_munmap would like to be called without mmap_sem held
> so that we can do the optimization. So, if we want to do the similar
> optimization for mmap/mremap path, I'm afraid we would have to redesign
> them.
> 
> I assumes munmap itself is the main source of the latency issue. mmap/mremap
> might hit the latency problem if they are trying to map or remap a huge
> overlapped address space, but it should be rare. So, I leave them untouched.

That depends on usecases very much. mremap might be called on very large
areas as well. But let's go in smaller steps and build on top...
-- 
Michal Hocko
SUSE Labs

Re: [RFC v6 PATCH 1/2] mm: refactor do_munmap() to extract the common part

[Michal Hocko]

On Fri 03-08-18 13:47:19, Yang Shi wrote:
> 
> 
> On 8/3/18 1:53 AM, Michal Hocko wrote:
> > On Fri 27-07-18 02:10:13, Yang Shi wrote:
> > > Introduces three new helper functions:
> > >    * munmap_addr_sanity()
> > >    * munmap_lookup_vma()
> > >    * munmap_mlock_vma()
> > > 
> > > They will be used by do_munmap() and the new do_munmap with zapping
> > > large mapping early in the later patch.
> > > 
> > > There is no functional change, just code refactor.
> > > 
> > > Reviewed-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> > > ---
> > >   mm/mmap.c | 120 ++++++++++++++++++++++++++++++++++++++++++--------------------
> > >   1 file changed, 82 insertions(+), 38 deletions(-)
> > > 
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index d1eb87e..2504094 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -2686,34 +2686,44 @@ int split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
> > >   	return __split_vma(mm, vma, addr, new_below);
> > >   }
> > > -/* Munmap is split into 2 main parts -- this part which finds
> > > - * what needs doing, and the areas themselves, which do the
> > > - * work.  This now handles partial unmappings.
> > > - * Jeremy Fitzhardinge <jeremy@goop.org>
> > > - */
> > > -int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
> > > -	      struct list_head *uf)
> > > +static inline bool munmap_addr_sanity(unsigned long start, size_t len)
> > munmap_check_addr? Btw. why does this need to have munmap prefix at all?
> > This is a general address space check.
> 
> Just because I extracted this from do_munmap, no special consideration. It
> is definitely ok to use another name.
> 
> > 
> > >   {
> > > -	unsigned long end;
> > > -	struct vm_area_struct *vma, *prev, *last;
> > > -
> > >   	if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start)
> > > -		return -EINVAL;
> > > +		return false;
> > > -	len = PAGE_ALIGN(len);
> > > -	if (len == 0)
> > > -		return -EINVAL;
> > > +	if (PAGE_ALIGN(len) == 0)
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +/*
> > > + * munmap_lookup_vma: find the first overlap vma and split overlap vmas.
> > > + * @mm: mm_struct
> > > + * @vma: the first overlapping vma
> > > + * @prev: vma's prev
> > > + * @start: start address
> > > + * @end: end address
> > This really doesn't help me to understand how to use the function.
> > Why do we need both prev and vma etc...
> 
> prev will be used by unmap_region later.

But what does it stand for? Why cannot you take prev from the returned
vma? In other words, if somebody reads this documentation how does he
know what the prev is supposed to be used for?

> > > + *
> > > + * returns 1 if successful, 0 or errno otherwise
> > This is a really weird calling convention. So what does 0 tell? /me
> > checks the code. Ohh, it is nothing to do. Why cannot you simply return
> > the vma. NULL implies nothing to do, ERR_PTR on error.
> 
> A couple of reasons why it is implemented as so:
> 
>     * do_munmap returns 0 for both success and no suitable vma
> 
>     * Since prev is needed by finding the start vma, and prev will be used
> by unmap_region later too, so I just thought it would look clean to have one
> function to return both start vma and prev. In this way, we can share as
> much as possible common code.
> 
>     * In this way, we just need return 0, 1 or error no just as same as what
> do_munmap does currently. Then we know what is failure case exactly to just
> bail out right away.
> 
> Actually, I tried the same approach as you suggested, but it had two
> problems:
> 
>     * If it returns the start vma, we have to re-find its prev later, but
> the prev has been found during finding start vma. And, duplicate the code in
> do_munmap_zap_rlock. It sounds not that ideal.
> 
>     * If it returns prev, it might be null (start vma is the first vma). We
> can't tell if null is a failure or success case

Even if you need to return both vma and prev then it would be better to
simply return vma directly than having this -errno, 0 or 1 return
semantic.
-- 
Michal Hocko
SUSE Labs

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Yang Shi]

On 8/6/18 2:40 AM, Michal Hocko wrote:
> On Fri 03-08-18 14:01:58, Yang Shi wrote:
>>
>> On 8/3/18 2:07 AM, Michal Hocko wrote:
>>> On Fri 27-07-18 02:10:14, Yang Shi wrote:
>>>> When running some mmap/munmap scalability tests with large memory (i.e.
>>>>> 300GB), the below hung task issue may happen occasionally.
>>>> INFO: task ps:14018 blocked for more than 120 seconds.
>>>>          Tainted: G            E 4.9.79-009.ali3000.alios7.x86_64 #1
>>>>    "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
>>>> message.
>>>>    ps              D    0 14018      1 0x00000004
>>>>     ffff885582f84000 ffff885e8682f000 ffff880972943000 ffff885ebf499bc0
>>>>     ffff8828ee120000 ffffc900349bfca8 ffffffff817154d0 0000000000000040
>>>>     00ffffff812f872a ffff885ebf499bc0 024000d000948300 ffff880972943000
>>>>    Call Trace:
>>>>     [<ffffffff817154d0>] ? __schedule+0x250/0x730
>>>>     [<ffffffff817159e6>] schedule+0x36/0x80
>>>>     [<ffffffff81718560>] rwsem_down_read_failed+0xf0/0x150
>>>>     [<ffffffff81390a28>] call_rwsem_down_read_failed+0x18/0x30
>>>>     [<ffffffff81717db0>] down_read+0x20/0x40
>>>>     [<ffffffff812b9439>] proc_pid_cmdline_read+0xd9/0x4e0
>>>>     [<ffffffff81253c95>] ? do_filp_open+0xa5/0x100
>>>>     [<ffffffff81241d87>] __vfs_read+0x37/0x150
>>>>     [<ffffffff812f824b>] ? security_file_permission+0x9b/0xc0
>>>>     [<ffffffff81242266>] vfs_read+0x96/0x130
>>>>     [<ffffffff812437b5>] SyS_read+0x55/0xc0
>>>>     [<ffffffff8171a6da>] entry_SYSCALL_64_fastpath+0x1a/0xc5
>>>>
>>>> It is because munmap holds mmap_sem exclusively from very beginning to
>>>> all the way down to the end, and doesn't release it in the middle. When
>>>> unmapping large mapping, it may take long time (take ~18 seconds to
>>>> unmap 320GB mapping with every single page mapped on an idle machine).
>>>>
>>>> Zapping pages is the most time consuming part, according to the
>>>> suggestion from Michal Hocko [1], zapping pages can be done with holding
>>>> read mmap_sem, like what MADV_DONTNEED does. Then re-acquire write
>>>> mmap_sem to cleanup vmas.
>>>>
>>>> But, some part may need write mmap_sem, for example, vma splitting. So,
>>>> the design is as follows:
>>>>           acquire write mmap_sem
>>>>           lookup vmas (find and split vmas)
>>>> 	detach vmas
>>>>           deal with special mappings
>>>>           downgrade_write
>>>>
>>>>           zap pages
>>>> 	free page tables
>>>>           release mmap_sem
>>>>
>>>> The vm events with read mmap_sem may come in during page zapping, but
>>>> since vmas have been detached before, they, i.e. page fault, gup, etc,
>>>> will not be able to find valid vma, then just return SIGSEGV or -EFAULT
>>>> as expected.
>>>>
>>>> If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
>>>> considered as special mappings. They will be dealt with before zapping
>>>> pages with write mmap_sem held. Basically, just update vm_flags.
>>> Well, I think it would be safer to simply fallback to the current
>>> implementation with these mappings and deal with them on top. This would
>>> make potential issues easier to bisect and partial reverts as well.
>> Do you mean just call do_munmap()? It sounds ok. Although we may waste some
>> cycles to repeat what has done, it sounds not too bad since those special
>> mappings should be not very common.
> VM_HUGETLB is quite spread. Especially for DB workloads.

Wait a minute. In this way, it sounds we go back to my old 
implementation with special handling for those mappings with write 
mmap_sem held, right?

>
>>>> And, since they are also manipulated by unmap_single_vma() which is
>>>> called by unmap_vma() with read mmap_sem held in this case, to
>>>> prevent from updating vm_flags in read critical section, a new
>>>> parameter, called "skip_flags" is added to unmap_region(), unmap_vmas()
>>>> and unmap_single_vma(). If it is true, then just skip unmap those
>>>> special mappings. Currently, the only place which pass true to this
>>>> parameter is us.
>>> skip parameters are usually ugly and lead to more mess later on. Can we
>>> do without them?
>> We need a way to tell unmap_region() that it is called in a kind of special
>> context which updating vm_flags is not allowed. I didn't think of a better
>> way.
>>
>> We could add a new API to do what unmap_region() does without updating
>> vm_flags, but we would have to  duplicate some code.
> I really didn't get to think about a better way myself but I strongly
> suspect we can do without special hacks here. Is updating flags under
> read lock a real problem? Assuming that special mappings are not really
> considered at this stage.

In normal case, I don't think vm_flags can be updated with read 
mmap_sem, but in this patch the vmas have been detached from the rb 
tree, nobody can find them anymore (I'm supposed all vma looking up is 
done by find_vma), so it might be safe to update vm_flags with read 
mmap_sem.

If it is safe, we don't have to have any special handling to those 
special mappings anymore.

>
>>>> With this approach we don't have to re-acquire mmap_sem again to clean
>>>> up vmas to avoid race window which might get the address space changed.
>>> By with this approach you mean detaching right?
>> Yes, the detaching approach.
> Please make it explicit in the changelog.

Sure.

>   
>>>> And, since the lock acquire/release cost is managed to the minimum and
>>>> almost as same as before, the optimization could be extended to any size
>>>> of mapping without incurring significant penalty to small mappings.
>>> I guess you mean to say that lock downgrade approach doesn't lead to
>>> regressions because the overal time mmap_sem is taken is not longer?
>> Yes. And, there is not lock take/retake cost since we don't release it.
> Please also be explicit.

Sure.

>   
>>>> For the time being, just do this in munmap syscall path. Other
>>>> vm_munmap() or do_munmap() call sites (i.e mmap, mremap, etc) remain
>>>> intact for stability reason.
>>> You have used this argument previously and several people have asked.
>>> I think it is just wrong. Either the concept is safe and all callers can
>>> use it or it is not and then those subtle differences should be called
>>> out. Your previous response was that you simply haven't tested other
>>> paths. Well, that is not an argument, I am afraid. The whole thing
>>> should be done at a proper layer. If there are some difficulties to
>>> achieve that for all callers then OK just be explicit about that. I can
>>> imagine some callers really require the exclusive look when munmap
>>> returns for example.
>> Yes, the statement here sounds ambiguous. There are definitely some
>> difficulties to achieve that in mmap and mremap. Since they acquire write
>> mmap_sem at the very beginning, then do their stuff, which may call
>> do_munmap if overlapped address space has to be changed.
> Do call them out. Maybe even add a comment in the code so that people
> who would like those other paths know what they need to look at.

OK

>
>> But, the optimized do_munmap would like to be called without mmap_sem held
>> so that we can do the optimization. So, if we want to do the similar
>> optimization for mmap/mremap path, I'm afraid we would have to redesign
>> them.
>>
>> I assumes munmap itself is the main source of the latency issue. mmap/mremap
>> might hit the latency problem if they are trying to map or remap a huge
>> overlapped address space, but it should be rare. So, I leave them untouched.
> That depends on usecases very much. mremap might be called on very large
> areas as well. But let's go in smaller steps and build on top...

Yes, agree. And, I agree to achieve it step by step.

Thanks,
Yang

Re: [RFC v6 PATCH 1/2] mm: refactor do_munmap() to extract the common part

[Yang Shi]

On 8/6/18 6:26 AM, Michal Hocko wrote:
> On Fri 03-08-18 13:47:19, Yang Shi wrote:
>>
>> On 8/3/18 1:53 AM, Michal Hocko wrote:
>>> On Fri 27-07-18 02:10:13, Yang Shi wrote:
>>>> Introduces three new helper functions:
>>>>     * munmap_addr_sanity()
>>>>     * munmap_lookup_vma()
>>>>     * munmap_mlock_vma()
>>>>
>>>> They will be used by do_munmap() and the new do_munmap with zapping
>>>> large mapping early in the later patch.
>>>>
>>>> There is no functional change, just code refactor.
>>>>
>>>> Reviewed-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>>>> ---
>>>>    mm/mmap.c | 120 ++++++++++++++++++++++++++++++++++++++++++--------------------
>>>>    1 file changed, 82 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/mm/mmap.c b/mm/mmap.c
>>>> index d1eb87e..2504094 100644
>>>> --- a/mm/mmap.c
>>>> +++ b/mm/mmap.c
>>>> @@ -2686,34 +2686,44 @@ int split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
>>>>    	return __split_vma(mm, vma, addr, new_below);
>>>>    }
>>>> -/* Munmap is split into 2 main parts -- this part which finds
>>>> - * what needs doing, and the areas themselves, which do the
>>>> - * work.  This now handles partial unmappings.
>>>> - * Jeremy Fitzhardinge <jeremy@goop.org>
>>>> - */
>>>> -int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>>>> -	      struct list_head *uf)
>>>> +static inline bool munmap_addr_sanity(unsigned long start, size_t len)
>>> munmap_check_addr? Btw. why does this need to have munmap prefix at all?
>>> This is a general address space check.
>> Just because I extracted this from do_munmap, no special consideration. It
>> is definitely ok to use another name.
>>
>>>>    {
>>>> -	unsigned long end;
>>>> -	struct vm_area_struct *vma, *prev, *last;
>>>> -
>>>>    	if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start)
>>>> -		return -EINVAL;
>>>> +		return false;
>>>> -	len = PAGE_ALIGN(len);
>>>> -	if (len == 0)
>>>> -		return -EINVAL;
>>>> +	if (PAGE_ALIGN(len) == 0)
>>>> +		return false;
>>>> +
>>>> +	return true;
>>>> +}
>>>> +
>>>> +/*
>>>> + * munmap_lookup_vma: find the first overlap vma and split overlap vmas.
>>>> + * @mm: mm_struct
>>>> + * @vma: the first overlapping vma
>>>> + * @prev: vma's prev
>>>> + * @start: start address
>>>> + * @end: end address
>>> This really doesn't help me to understand how to use the function.
>>> Why do we need both prev and vma etc...
>> prev will be used by unmap_region later.
> But what does it stand for? Why cannot you take prev from the returned
> vma? In other words, if somebody reads this documentation how does he
> know what the prev is supposed to be used for?
>
>>>> + *
>>>> + * returns 1 if successful, 0 or errno otherwise
>>> This is a really weird calling convention. So what does 0 tell? /me
>>> checks the code. Ohh, it is nothing to do. Why cannot you simply return
>>> the vma. NULL implies nothing to do, ERR_PTR on error.
>> A couple of reasons why it is implemented as so:
>>
>>      * do_munmap returns 0 for both success and no suitable vma
>>
>>      * Since prev is needed by finding the start vma, and prev will be used
>> by unmap_region later too, so I just thought it would look clean to have one
>> function to return both start vma and prev. In this way, we can share as
>> much as possible common code.
>>
>>      * In this way, we just need return 0, 1 or error no just as same as what
>> do_munmap does currently. Then we know what is failure case exactly to just
>> bail out right away.
>>
>> Actually, I tried the same approach as you suggested, but it had two
>> problems:
>>
>>      * If it returns the start vma, we have to re-find its prev later, but
>> the prev has been found during finding start vma. And, duplicate the code in
>> do_munmap_zap_rlock. It sounds not that ideal.
>>
>>      * If it returns prev, it might be null (start vma is the first vma). We
>> can't tell if null is a failure or success case
> Even if you need to return both vma and prev then it would be better to
> simply return vma directly than having this -errno, 0 or 1 return
> semantic.

OK, I will try to refactor the code.

Thanks,
Yang

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Michal Hocko]

On Mon 06-08-18 09:46:30, Yang Shi wrote:
> 
> 
> On 8/6/18 2:40 AM, Michal Hocko wrote:
> > On Fri 03-08-18 14:01:58, Yang Shi wrote:
> > > 
> > > On 8/3/18 2:07 AM, Michal Hocko wrote:
> > > > On Fri 27-07-18 02:10:14, Yang Shi wrote:
[...]
> > > > > If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
> > > > > considered as special mappings. They will be dealt with before zapping
> > > > > pages with write mmap_sem held. Basically, just update vm_flags.
> > > > Well, I think it would be safer to simply fallback to the current
> > > > implementation with these mappings and deal with them on top. This would
> > > > make potential issues easier to bisect and partial reverts as well.
> > > Do you mean just call do_munmap()? It sounds ok. Although we may waste some
> > > cycles to repeat what has done, it sounds not too bad since those special
> > > mappings should be not very common.
> > VM_HUGETLB is quite spread. Especially for DB workloads.
> 
> Wait a minute. In this way, it sounds we go back to my old implementation
> with special handling for those mappings with write mmap_sem held, right?

Yes, I would really start simple and add further enhacements on top.
-- 
Michal Hocko
SUSE Labs

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Yang Shi]

On 8/6/18 1:41 PM, Michal Hocko wrote:
> On Mon 06-08-18 09:46:30, Yang Shi wrote:
>>
>> On 8/6/18 2:40 AM, Michal Hocko wrote:
>>> On Fri 03-08-18 14:01:58, Yang Shi wrote:
>>>> On 8/3/18 2:07 AM, Michal Hocko wrote:
>>>>> On Fri 27-07-18 02:10:14, Yang Shi wrote:
> [...]
>>>>>> If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
>>>>>> considered as special mappings. They will be dealt with before zapping
>>>>>> pages with write mmap_sem held. Basically, just update vm_flags.
>>>>> Well, I think it would be safer to simply fallback to the current
>>>>> implementation with these mappings and deal with them on top. This would
>>>>> make potential issues easier to bisect and partial reverts as well.
>>>> Do you mean just call do_munmap()? It sounds ok. Although we may waste some
>>>> cycles to repeat what has done, it sounds not too bad since those special
>>>> mappings should be not very common.
>>> VM_HUGETLB is quite spread. Especially for DB workloads.
>> Wait a minute. In this way, it sounds we go back to my old implementation
>> with special handling for those mappings with write mmap_sem held, right?
> Yes, I would really start simple and add further enhacements on top.

If updating vm_flags with read lock is safe in this case, we don't have 
to do this. The only reason for this special handling is about vm_flags 
update.

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Michal Hocko]

On Mon 06-08-18 13:48:35, Yang Shi wrote:
> 
> 
> On 8/6/18 1:41 PM, Michal Hocko wrote:
> > On Mon 06-08-18 09:46:30, Yang Shi wrote:
> > > 
> > > On 8/6/18 2:40 AM, Michal Hocko wrote:
> > > > On Fri 03-08-18 14:01:58, Yang Shi wrote:
> > > > > On 8/3/18 2:07 AM, Michal Hocko wrote:
> > > > > > On Fri 27-07-18 02:10:14, Yang Shi wrote:
> > [...]
> > > > > > > If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
> > > > > > > considered as special mappings. They will be dealt with before zapping
> > > > > > > pages with write mmap_sem held. Basically, just update vm_flags.
> > > > > > Well, I think it would be safer to simply fallback to the current
> > > > > > implementation with these mappings and deal with them on top. This would
> > > > > > make potential issues easier to bisect and partial reverts as well.
> > > > > Do you mean just call do_munmap()? It sounds ok. Although we may waste some
> > > > > cycles to repeat what has done, it sounds not too bad since those special
> > > > > mappings should be not very common.
> > > > VM_HUGETLB is quite spread. Especially for DB workloads.
> > > Wait a minute. In this way, it sounds we go back to my old implementation
> > > with special handling for those mappings with write mmap_sem held, right?
> > Yes, I would really start simple and add further enhacements on top.
> 
> If updating vm_flags with read lock is safe in this case, we don't have to
> do this. The only reason for this special handling is about vm_flags update.
 
Yes, maybe you are right that this is safe. I would still argue to have
it in a separate patch for easier review, bisectability etc...

-- 
Michal Hocko
SUSE Labs

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Yang Shi]

On 8/6/18 1:52 PM, Michal Hocko wrote:
> On Mon 06-08-18 13:48:35, Yang Shi wrote:
>>
>> On 8/6/18 1:41 PM, Michal Hocko wrote:
>>> On Mon 06-08-18 09:46:30, Yang Shi wrote:
>>>> On 8/6/18 2:40 AM, Michal Hocko wrote:
>>>>> On Fri 03-08-18 14:01:58, Yang Shi wrote:
>>>>>> On 8/3/18 2:07 AM, Michal Hocko wrote:
>>>>>>> On Fri 27-07-18 02:10:14, Yang Shi wrote:
>>> [...]
>>>>>>>> If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
>>>>>>>> considered as special mappings. They will be dealt with before zapping
>>>>>>>> pages with write mmap_sem held. Basically, just update vm_flags.
>>>>>>> Well, I think it would be safer to simply fallback to the current
>>>>>>> implementation with these mappings and deal with them on top. This would
>>>>>>> make potential issues easier to bisect and partial reverts as well.
>>>>>> Do you mean just call do_munmap()? It sounds ok. Although we may waste some
>>>>>> cycles to repeat what has done, it sounds not too bad since those special
>>>>>> mappings should be not very common.
>>>>> VM_HUGETLB is quite spread. Especially for DB workloads.
>>>> Wait a minute. In this way, it sounds we go back to my old implementation
>>>> with special handling for those mappings with write mmap_sem held, right?
>>> Yes, I would really start simple and add further enhacements on top.
>> If updating vm_flags with read lock is safe in this case, we don't have to
>> do this. The only reason for this special handling is about vm_flags update.
>   
> Yes, maybe you are right that this is safe. I would still argue to have
> it in a separate patch for easier review, bisectability etc...

Sorry, I'm a little bit confused. Do you mean I should have the patch 
*without* handling the special case (just like to assume it is safe to 
update vm_flags with read lock), then have the other patch on top of it, 
which simply calls do_munmap() to deal with the special cases?

>

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Michal Hocko]

On Mon 06-08-18 15:19:06, Yang Shi wrote:
> 
> 
> On 8/6/18 1:52 PM, Michal Hocko wrote:
> > On Mon 06-08-18 13:48:35, Yang Shi wrote:
> > > 
> > > On 8/6/18 1:41 PM, Michal Hocko wrote:
> > > > On Mon 06-08-18 09:46:30, Yang Shi wrote:
> > > > > On 8/6/18 2:40 AM, Michal Hocko wrote:
> > > > > > On Fri 03-08-18 14:01:58, Yang Shi wrote:
> > > > > > > On 8/3/18 2:07 AM, Michal Hocko wrote:
> > > > > > > > On Fri 27-07-18 02:10:14, Yang Shi wrote:
> > > > [...]
> > > > > > > > > If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
> > > > > > > > > considered as special mappings. They will be dealt with before zapping
> > > > > > > > > pages with write mmap_sem held. Basically, just update vm_flags.
> > > > > > > > Well, I think it would be safer to simply fallback to the current
> > > > > > > > implementation with these mappings and deal with them on top. This would
> > > > > > > > make potential issues easier to bisect and partial reverts as well.
> > > > > > > Do you mean just call do_munmap()? It sounds ok. Although we may waste some
> > > > > > > cycles to repeat what has done, it sounds not too bad since those special
> > > > > > > mappings should be not very common.
> > > > > > VM_HUGETLB is quite spread. Especially for DB workloads.
> > > > > Wait a minute. In this way, it sounds we go back to my old implementation
> > > > > with special handling for those mappings with write mmap_sem held, right?
> > > > Yes, I would really start simple and add further enhacements on top.
> > > If updating vm_flags with read lock is safe in this case, we don't have to
> > > do this. The only reason for this special handling is about vm_flags update.
> > Yes, maybe you are right that this is safe. I would still argue to have
> > it in a separate patch for easier review, bisectability etc...
> 
> Sorry, I'm a little bit confused. Do you mean I should have the patch
> *without* handling the special case (just like to assume it is safe to
> update vm_flags with read lock), then have the other patch on top of it,
> which simply calls do_munmap() to deal with the special cases?

Just skip those special cases in the initial implementation and handle
each special case in its own patch on top.
-- 
Michal Hocko
SUSE Labs

Re: [RFC v6 PATCH 1/2] mm: refactor do_munmap() to extract the common part

[Vlastimil Babka]

On 07/26/2018 08:10 PM, Yang Shi wrote:
> Introduces three new helper functions:
>   * munmap_addr_sanity()
>   * munmap_lookup_vma()
>   * munmap_mlock_vma()
> 
> They will be used by do_munmap() and the new do_munmap with zapping
> large mapping early in the later patch.
> 
> There is no functional change, just code refactor.
> 
> Reviewed-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> ---
>  mm/mmap.c | 120 ++++++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 82 insertions(+), 38 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index d1eb87e..2504094 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2686,34 +2686,44 @@ int split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
>  	return __split_vma(mm, vma, addr, new_below);
>  }
>  
> -/* Munmap is split into 2 main parts -- this part which finds
> - * what needs doing, and the areas themselves, which do the
> - * work.  This now handles partial unmappings.
> - * Jeremy Fitzhardinge <jeremy@goop.org>
> - */
> -int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
> -	      struct list_head *uf)
> +static inline bool munmap_addr_sanity(unsigned long start, size_t len)

Since it's returning bool, the proper naming scheme would be something
like "munmap_addr_ok()". I don't know how I would replace the "munmap_"
prefix myself though.

>  {
> -	unsigned long end;
> -	struct vm_area_struct *vma, *prev, *last;
> -
>  	if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start)
> -		return -EINVAL;
> +		return false;
>  
> -	len = PAGE_ALIGN(len);
> -	if (len == 0)
> -		return -EINVAL;
> +	if (PAGE_ALIGN(len) == 0)
> +		return false;
> +
> +	return true;
> +}
> +
> +/*
> + * munmap_lookup_vma: find the first overlap vma and split overlap vmas.
> + * @mm: mm_struct
> + * @vma: the first overlapping vma
> + * @prev: vma's prev
> + * @start: start address
> + * @end: end address
> + *
> + * returns 1 if successful, 0 or errno otherwise
> + */
> +static int munmap_lookup_vma(struct mm_struct *mm, struct vm_area_struct **vma,
> +			     struct vm_area_struct **prev, unsigned long start,
> +			     unsigned long end)

Agree with Michal that you could simply return vma, NULL, or error.
Caller can easily find out prev from that, it's not like we have to
count each cpu cycle here. It will be a bit less tricky code as well,
which is a plus.

...
> +static inline void munmap_mlock_vma(struct vm_area_struct *vma,
> +				    unsigned long end)

This function does munlock, not mlock. You could call it e.g.
munlock_vmas().

> +{
> +	struct vm_area_struct *tmp = vma;
> +
> +	while (tmp && tmp->vm_start < end) {
> +		if (tmp->vm_flags & VM_LOCKED) {
> +			vma->vm_mm->locked_vm -= vma_pages(tmp);

You keep 'vma' just for the vm_mm? Better extract mm pointer first and
then you don't need the 'tmp'.

> +			munlock_vma_pages_all(tmp);
> +		}
> +		tmp = tmp->vm_next;
> +	}
> +}

Re: [RFC v6 PATCH 1/2] mm: refactor do_munmap() to extract the common part

[Yang Shi]

On 8/7/18 7:59 AM, Vlastimil Babka wrote:
> On 07/26/2018 08:10 PM, Yang Shi wrote:
>> Introduces three new helper functions:
>>    * munmap_addr_sanity()
>>    * munmap_lookup_vma()
>>    * munmap_mlock_vma()
>>
>> They will be used by do_munmap() and the new do_munmap with zapping
>> large mapping early in the later patch.
>>
>> There is no functional change, just code refactor.
>>
>> Reviewed-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>> ---
>>   mm/mmap.c | 120 ++++++++++++++++++++++++++++++++++++++++++--------------------
>>   1 file changed, 82 insertions(+), 38 deletions(-)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index d1eb87e..2504094 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -2686,34 +2686,44 @@ int split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
>>   	return __split_vma(mm, vma, addr, new_below);
>>   }
>>   
>> -/* Munmap is split into 2 main parts -- this part which finds
>> - * what needs doing, and the areas themselves, which do the
>> - * work.  This now handles partial unmappings.
>> - * Jeremy Fitzhardinge <jeremy@goop.org>
>> - */
>> -int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
>> -	      struct list_head *uf)
>> +static inline bool munmap_addr_sanity(unsigned long start, size_t len)
> Since it's returning bool, the proper naming scheme would be something
> like "munmap_addr_ok()". I don't know how I would replace the "munmap_"
> prefix myself though.

OK, thanks for the suggestion.

>
>>   {
>> -	unsigned long end;
>> -	struct vm_area_struct *vma, *prev, *last;
>> -
>>   	if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start)
>> -		return -EINVAL;
>> +		return false;
>>   
>> -	len = PAGE_ALIGN(len);
>> -	if (len == 0)
>> -		return -EINVAL;
>> +	if (PAGE_ALIGN(len) == 0)
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> +/*
>> + * munmap_lookup_vma: find the first overlap vma and split overlap vmas.
>> + * @mm: mm_struct
>> + * @vma: the first overlapping vma
>> + * @prev: vma's prev
>> + * @start: start address
>> + * @end: end address
>> + *
>> + * returns 1 if successful, 0 or errno otherwise
>> + */
>> +static int munmap_lookup_vma(struct mm_struct *mm, struct vm_area_struct **vma,
>> +			     struct vm_area_struct **prev, unsigned long start,
>> +			     unsigned long end)
> Agree with Michal that you could simply return vma, NULL, or error.
> Caller can easily find out prev from that, it's not like we have to
> count each cpu cycle here. It will be a bit less tricky code as well,
> which is a plus.
>
> ...
>> +static inline void munmap_mlock_vma(struct vm_area_struct *vma,
>> +				    unsigned long end)
> This function does munlock, not mlock. You could call it e.g.
> munlock_vmas().

OK

>
>> +{
>> +	struct vm_area_struct *tmp = vma;
>> +
>> +	while (tmp && tmp->vm_start < end) {
>> +		if (tmp->vm_flags & VM_LOCKED) {
>> +			vma->vm_mm->locked_vm -= vma_pages(tmp);
> You keep 'vma' just for the vm_mm? Better extract mm pointer first and
> then you don't need the 'tmp'.

OK

Thanks,
Yang

>
>> +			munlock_vma_pages_all(tmp);
>> +		}
>> +		tmp = tmp->vm_next;
>> +	}
>> +}

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Yang Shi]

On 8/6/18 10:45 PM, Michal Hocko wrote:
> On Mon 06-08-18 15:19:06, Yang Shi wrote:
>>
>> On 8/6/18 1:52 PM, Michal Hocko wrote:
>>> On Mon 06-08-18 13:48:35, Yang Shi wrote:
>>>> On 8/6/18 1:41 PM, Michal Hocko wrote:
>>>>> On Mon 06-08-18 09:46:30, Yang Shi wrote:
>>>>>> On 8/6/18 2:40 AM, Michal Hocko wrote:
>>>>>>> On Fri 03-08-18 14:01:58, Yang Shi wrote:
>>>>>>>> On 8/3/18 2:07 AM, Michal Hocko wrote:
>>>>>>>>> On Fri 27-07-18 02:10:14, Yang Shi wrote:
>>>>> [...]
>>>>>>>>>> If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
>>>>>>>>>> considered as special mappings. They will be dealt with before zapping
>>>>>>>>>> pages with write mmap_sem held. Basically, just update vm_flags.
>>>>>>>>> Well, I think it would be safer to simply fallback to the current
>>>>>>>>> implementation with these mappings and deal with them on top. This would
>>>>>>>>> make potential issues easier to bisect and partial reverts as well.
>>>>>>>> Do you mean just call do_munmap()? It sounds ok. Although we may waste some
>>>>>>>> cycles to repeat what has done, it sounds not too bad since those special
>>>>>>>> mappings should be not very common.
>>>>>>> VM_HUGETLB is quite spread. Especially for DB workloads.
>>>>>> Wait a minute. In this way, it sounds we go back to my old implementation
>>>>>> with special handling for those mappings with write mmap_sem held, right?
>>>>> Yes, I would really start simple and add further enhacements on top.
>>>> If updating vm_flags with read lock is safe in this case, we don't have to
>>>> do this. The only reason for this special handling is about vm_flags update.
>>> Yes, maybe you are right that this is safe. I would still argue to have
>>> it in a separate patch for easier review, bisectability etc...
>> Sorry, I'm a little bit confused. Do you mean I should have the patch
>> *without* handling the special case (just like to assume it is safe to
>> update vm_flags with read lock), then have the other patch on top of it,
>> which simply calls do_munmap() to deal with the special cases?
> Just skip those special cases in the initial implementation and handle
> each special case in its own patch on top.

Thanks. VM_LOCKED area will not be handled specially since it is easy to 
handle it, just follow what do_munmap does. The special cases will just 
handle VM_HUGETLB, VM_PFNMAP and uprobe mappings.

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Vlastimil Babka]

On 08/08/2018 03:51 AM, Yang Shi wrote:
> On 8/6/18 10:45 PM, Michal Hocko wrote:
>> On Mon 06-08-18 15:19:06, Yang Shi wrote:
>>>
>>> On 8/6/18 1:52 PM, Michal Hocko wrote:
>>>> On Mon 06-08-18 13:48:35, Yang Shi wrote:
>>>>> On 8/6/18 1:41 PM, Michal Hocko wrote:
>>>>>> On Mon 06-08-18 09:46:30, Yang Shi wrote:
>>>>>>> On 8/6/18 2:40 AM, Michal Hocko wrote:
>>>>>>>> On Fri 03-08-18 14:01:58, Yang Shi wrote:
>>>>>>>>> On 8/3/18 2:07 AM, Michal Hocko wrote:
>>>>>>>>>> On Fri 27-07-18 02:10:14, Yang Shi wrote:
>>>>>> [...]
>>>>>>>>>>> If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
>>>>>>>>>>> considered as special mappings. They will be dealt with before zapping
>>>>>>>>>>> pages with write mmap_sem held. Basically, just update vm_flags.
>>>>>>>>>> Well, I think it would be safer to simply fallback to the current
>>>>>>>>>> implementation with these mappings and deal with them on top. This would
>>>>>>>>>> make potential issues easier to bisect and partial reverts as well.
>>>>>>>>> Do you mean just call do_munmap()? It sounds ok. Although we may waste some
>>>>>>>>> cycles to repeat what has done, it sounds not too bad since those special
>>>>>>>>> mappings should be not very common.
>>>>>>>> VM_HUGETLB is quite spread. Especially for DB workloads.
>>>>>>> Wait a minute. In this way, it sounds we go back to my old implementation
>>>>>>> with special handling for those mappings with write mmap_sem held, right?
>>>>>> Yes, I would really start simple and add further enhacements on top.
>>>>> If updating vm_flags with read lock is safe in this case, we don't have to
>>>>> do this. The only reason for this special handling is about vm_flags update.
>>>> Yes, maybe you are right that this is safe. I would still argue to have
>>>> it in a separate patch for easier review, bisectability etc...
>>> Sorry, I'm a little bit confused. Do you mean I should have the patch
>>> *without* handling the special case (just like to assume it is safe to
>>> update vm_flags with read lock), then have the other patch on top of it,
>>> which simply calls do_munmap() to deal with the special cases?
>> Just skip those special cases in the initial implementation and handle
>> each special case in its own patch on top.
> 
> Thanks. VM_LOCKED area will not be handled specially since it is easy to 
> handle it, just follow what do_munmap does. The special cases will just 
> handle VM_HUGETLB, VM_PFNMAP and uprobe mappings.

So I think you could maybe structure code like this: instead of
introducing do_munmap_zap_rlock() and all those "bool skip_vm_flags"
additions, add a boolean parameter in do_munmap() to use the new
behavior, with only the first user SYSCALL_DEFINE2(munmap) setting it to
true. If true, do_munmap() will do the
- down_write_killable() itself instead of assuming it's already locked
- munmap_lookup_vma()
- check if any of the vma's in the range is "special", if yes, change
the boolean param to "false", and continue like previously, e.g. no mmap
sem downgrade etc.

That would be a basis for further optimizing the special vma cases in
subsequent patches (maybe it's really ok to touch the vma flags with
mmap sem for read as vma's are detached), and to eventually convert more
do_munmap() callers to the new mode.

HTH,
Vlastimil

Re: [RFC v6 PATCH 2/2] mm: mmap: zap pages with read mmap_sem in munmap

[Yang Shi]

On 8/8/18 2:22 AM, Vlastimil Babka wrote:
> On 08/08/2018 03:51 AM, Yang Shi wrote:
>> On 8/6/18 10:45 PM, Michal Hocko wrote:
>>> On Mon 06-08-18 15:19:06, Yang Shi wrote:
>>>> On 8/6/18 1:52 PM, Michal Hocko wrote:
>>>>> On Mon 06-08-18 13:48:35, Yang Shi wrote:
>>>>>> On 8/6/18 1:41 PM, Michal Hocko wrote:
>>>>>>> On Mon 06-08-18 09:46:30, Yang Shi wrote:
>>>>>>>> On 8/6/18 2:40 AM, Michal Hocko wrote:
>>>>>>>>> On Fri 03-08-18 14:01:58, Yang Shi wrote:
>>>>>>>>>> On 8/3/18 2:07 AM, Michal Hocko wrote:
>>>>>>>>>>> On Fri 27-07-18 02:10:14, Yang Shi wrote:
>>>>>>> [...]
>>>>>>>>>>>> If the vma has VM_LOCKED | VM_HUGETLB | VM_PFNMAP or uprobe, they are
>>>>>>>>>>>> considered as special mappings. They will be dealt with before zapping
>>>>>>>>>>>> pages with write mmap_sem held. Basically, just update vm_flags.
>>>>>>>>>>> Well, I think it would be safer to simply fallback to the current
>>>>>>>>>>> implementation with these mappings and deal with them on top. This would
>>>>>>>>>>> make potential issues easier to bisect and partial reverts as well.
>>>>>>>>>> Do you mean just call do_munmap()? It sounds ok. Although we may waste some
>>>>>>>>>> cycles to repeat what has done, it sounds not too bad since those special
>>>>>>>>>> mappings should be not very common.
>>>>>>>>> VM_HUGETLB is quite spread. Especially for DB workloads.
>>>>>>>> Wait a minute. In this way, it sounds we go back to my old implementation
>>>>>>>> with special handling for those mappings with write mmap_sem held, right?
>>>>>>> Yes, I would really start simple and add further enhacements on top.
>>>>>> If updating vm_flags with read lock is safe in this case, we don't have to
>>>>>> do this. The only reason for this special handling is about vm_flags update.
>>>>> Yes, maybe you are right that this is safe. I would still argue to have
>>>>> it in a separate patch for easier review, bisectability etc...
>>>> Sorry, I'm a little bit confused. Do you mean I should have the patch
>>>> *without* handling the special case (just like to assume it is safe to
>>>> update vm_flags with read lock), then have the other patch on top of it,
>>>> which simply calls do_munmap() to deal with the special cases?
>>> Just skip those special cases in the initial implementation and handle
>>> each special case in its own patch on top.
>> Thanks. VM_LOCKED area will not be handled specially since it is easy to
>> handle it, just follow what do_munmap does. The special cases will just
>> handle VM_HUGETLB, VM_PFNMAP and uprobe mappings.
> So I think you could maybe structure code like this: instead of
> introducing do_munmap_zap_rlock() and all those "bool skip_vm_flags"
> additions, add a boolean parameter in do_munmap() to use the new
> behavior, with only the first user SYSCALL_DEFINE2(munmap) setting it to
> true. If true, do_munmap() will do the
> - down_write_killable() itself instead of assuming it's already locked
> - munmap_lookup_vma()
> - check if any of the vma's in the range is "special", if yes, change
> the boolean param to "false", and continue like previously, e.g. no mmap
> sem downgrade etc.

Thanks for the suggestion. Actually, I did the similar thing in v1 
patches, which added a bool parameter in vm_munmap() to tell if 
releasing mmap_sem is acceptable for some code paths. But, it got pushed 
back by tglx since vm_munmap() is called by x86 specific code too (and 
some other architectures). He suggested to define a new function to do 
the optimization. So, I followed this approach in the later versions.

Yang

>
> That would be a basis for further optimizing the special vma cases in
> subsequent patches (maybe it's really ok to touch the vma flags with
> mmap sem for read as vma's are detached), and to eventually convert more
> do_munmap() callers to the new mode.
>
> HTH,
> Vlastimil
>
>
>