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

[RFC v10 PATCH 0/3] 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)
        deal with special mappings
        detach vmas
        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_HUGETLB | VM_PFNMAP, they are considered as special
mappings. They will be handled by falling back to regular do_munmap()
with exclusive mmap_sem held in this patch since they may update vm flags.

But, with the "detach vmas first" approach, the vmas have been detached
when vm flags are updated, so it sounds safe to update vm flags with
read mmap_sem for this specific case. So, VM_HUGETLB and VM_PFNMAP will
be handled by using the optimized path in the following separate patches
for bisectable sake.

Unmapping uprobe areas may need update mm flags (MMF_RECALC_UPROBES).
However it is fine to have false-positive MMF_RECALC_UPROBES according
to uprobes developer. So, uprobe unmap will not be handled by the
regular path.

With the "detach vmas first" 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 since downgrade_write() doesn't release the lock
to lead regression, which simply downgrades to read lock.

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 due to some implementation difficulties since they acquire write
mmap_sem from very beginning and hold it until the end, do_munmap()
might be called in the middle. 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. mremap might be called on very
large area depending on the usecases, the optimization to it will be
considered in the future.


Changelog
v9 -> v10:
* Adopted the suggestion from Willy by not duplicating do_munmap. No change to
  the overall design of the optimization.

v8 -> v9:
* Uprobe developer (Oleg Nesterov and Srikar Dronamraju) helped to confirm it is
  fine to have a false-positive MMF_RECALC_UPROBES. So, unmapping uprobe areas
  doesn't have to be handled by regular path. Thanks Oleg.
* Dave hansen helped to confirm mpx unmap has to be called under write mmap_sem,
  but it has not to be after unmap_region(). So move arch_unmap() before
  downgrade_write(). The other user of arch_unmap() is PowerPC, which just set
  mm->context.vdso_base, so it sounds fine for this change too. Thanks Dave.
* The above two resolved the concern from Vlastimil.

v7 -> v8:
* Added Acked-by from Vlastimil for patch 1/5. Thanks.
* Fixed the wrong "evolution" direction. Converted VM_HUGETLB and VM_PFNMAP
  mapping use the optimized path in separate patches respectively for safe and
  bisectable sake per Michal's suggestion.
* Extracted has_uprobes() helper from uprobes_munmap() to check if mm or vmas
  have uprobes, which could save some cycles instead of calling
  vma_has_uprobes() directly for some cases. Per Vlastimil's suggestion.
* Keep unmapping uprobes area using regular do_munmap() since it might update
  mm flags, that might be not safe with read mmap_sem even though vmas have
  been detached.
* Fixed some comments from Willy.

v6 -> v7:
* Rename some helper functions per Michal and Vlastimil's comments.
* Refactor munmap_lookup_vma() to return the pointer of start vma per Michal's
  suggestion.
* Rephrase some commit log for patch 2/4 per Michal's comments.
* Deal with special mappings (VM_HUGETLB | VM_PFNMAP | uprobes) with regular
  do_munmap() in a separate patch per Michal's suggestion.
* Bring the patch which makes vma_has_uprobes() non-static back since it is
  needed to check if a vma has uprobes or not.

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 {
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 (3):
      mm: mmap: zap pages with read mmap_sem in munmap
      mm: unmap VM_HUGETLB mappings with optimized path
      mm: unmap VM_PFNMAP mappings with optimized path

 mm/mmap.c | 50 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 11 deletions(-)

[RFC v10 PATCH 1/3] 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)
        deal with special mappings
        detach vmas
        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_HUGETLB | VM_PFNMAP, they are considered as special
mappings. They will be handled by without downgrading mmap_sem in this
patch since they may update vm flags.

But, with the "detach vmas first" approach, the vmas have been detached
when vm flags are updated, so it sounds safe to update vm flags with
read mmap_sem for this specific case. So, VM_HUGETLB and VM_PFNMAP will
be handled by using the optimized path in the following separate patches
for bisectable sake.

Unmapping uprobe areas may need update mm flags (MMF_RECALC_UPROBES).
However it is fine to have false-positive MMF_RECALC_UPROBES according
to uprobes developer.

With the "detach vmas first" 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 since downgrade_write() doesn't release the lock
to lead regression, which simply downgrades to read lock.

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 due to some implementation difficulties since they acquire write
mmap_sem from very beginning and hold it until the end, do_munmap()
might be called in the middle. 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. mremap might be called on very
large area depending on the usecases, the optimization to it will be
considered in the future.

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() {
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>
Suggested-by: Matthew Wilcox <willy@infradead.org>
Cc: Laurent Dufour <ldufour@linux.vnet.ibm.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
 mm/mmap.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 11 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 5f2b2b1..2879b19 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2687,8 +2687,8 @@ int split_vma(struct mm_struct *mm, struct vm_area_struct *vma,
  * 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 int __do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
+		       struct list_head *uf, bool downgrade)
 {
 	unsigned long end;
 	struct vm_area_struct *vma, *prev, *last;
@@ -2770,25 +2770,47 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
 				mm->locked_vm -= vma_pages(tmp);
 				munlock_vma_pages_all(tmp);
 			}
+
+			/*
+			 * Unmapping vmas, which have VM_HUGETLB or VM_PFNMAP,
+			 * need get done with write mmap_sem held since they may
+			 * update vm_flags.
+			 */
+			if (downgrade &&
+			    (tmp->vm_flags & (VM_HUGETLB | VM_PFNMAP)))
+				downgrade = false;
+
 			tmp = tmp->vm_next;
 		}
 	}
 
-	/*
-	 * Remove the vma's, and unmap the actual pages
-	 */
+	/* Detatch vmas from rbtree */
 	detach_vmas_to_be_unmapped(mm, vma, prev, end);
-	unmap_region(mm, vma, prev, start, end);
 
+	/*
+	 * mpx unmap need to be handled with write mmap_sem. It is safe to
+	 * deal with it before unmap_region().
+	 */
 	arch_unmap(mm, vma, start, end);
 
+	if (downgrade)
+		downgrade_write(&mm->mmap_sem);
+
+	unmap_region(mm, vma, prev, start, end);
+
 	/* Fix up all other VM information */
 	remove_vma_list(mm, vma);
 
-	return 0;
+	return downgrade ? 1 : 0;
 }
 
-int vm_munmap(unsigned long start, size_t len)
+int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
+	      struct list_head *uf)
+{
+	return __do_munmap(mm, start, len, uf, false);
+}
+
+static int __vm_munmap(unsigned long start, size_t len, bool downgrade)
 {
 	int ret;
 	struct mm_struct *mm = current->mm;
@@ -2797,17 +2819,32 @@ int vm_munmap(unsigned long start, size_t len)
 	if (down_write_killable(&mm->mmap_sem))
 		return -EINTR;
 
-	ret = do_munmap(mm, start, len, &uf);
-	up_write(&mm->mmap_sem);
+	ret = __do_munmap(mm, start, len, &uf, downgrade);
+	/*
+	 * Returning 1 indicates mmap_sem is down graded.
+	 * But 1 is not legal return value of vm_munmap() and munmap(), reset
+	 * it to 0 before return.
+	 */
+	if (ret == 1) {
+		up_read(&mm->mmap_sem);
+		ret = 0;
+	} else
+		up_write(&mm->mmap_sem);
+
 	userfaultfd_unmap_complete(mm, &uf);
 	return ret;
 }
+
+int vm_munmap(unsigned long start, size_t len)
+{
+	return __vm_munmap(start, len, false);
+}
 EXPORT_SYMBOL(vm_munmap);
 
 SYSCALL_DEFINE2(munmap, unsigned long, addr, size_t, len)
 {
 	profile_munmap(addr);
-	return vm_munmap(addr, len);
+	return __vm_munmap(addr, len, true);
 }
 
 
-- 
1.8.3.1

[RFC v10 PATCH 2/3] mm: unmap VM_HUGETLB mappings with optimized path

[Yang Shi]

When unmapping VM_HUGETLB mappings, vm flags need to be updated. Since
the vmas have been detached, so it sounds safe to update vm flags with
read mmap_sem.

Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
 mm/mmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 2879b19..991e066 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2777,7 +2777,7 @@ static int __do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
 			 * update vm_flags.
 			 */
 			if (downgrade &&
-			    (tmp->vm_flags & (VM_HUGETLB | VM_PFNMAP)))
+			    (tmp->vm_flags & VM_PFNMAP))
 				downgrade = false;
 
 			tmp = tmp->vm_next;
-- 
1.8.3.1

[RFC v10 PATCH 3/3] mm: unmap VM_PFNMAP mappings with optimized path

[Yang Shi]

When unmapping VM_PFNMAP mappings, vm flags need to be updated. Since
the vmas have been detached, so it sounds safe to update vm flags with
read mmap_sem.

Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
 mm/mmap.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 991e066..04c1d3b 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2771,15 +2771,6 @@ static int __do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
 				munlock_vma_pages_all(tmp);
 			}
 
-			/*
-			 * Unmapping vmas, which have VM_HUGETLB or VM_PFNMAP,
-			 * need get done with write mmap_sem held since they may
-			 * update vm_flags.
-			 */
-			if (downgrade &&
-			    (tmp->vm_flags & VM_PFNMAP))
-				downgrade = false;
-
 			tmp = tmp->vm_next;
 		}
 	}
-- 
1.8.3.1

Re: [RFC v10 PATCH 1/3] mm: mmap: zap pages with read mmap_sem in munmap

[Matthew Wilcox]

On Sat, Sep 15, 2018 at 04:34:57AM +0800, Yang Shi wrote:
> Suggested-by: Michal Hocko <mhocko@kernel.org>
> Suggested-by: Kirill A. Shutemov <kirill@shutemov.name>
> Suggested-by: Matthew Wilcox <willy@infradead.org>

Reviewed-by: Matthew Wilcox <willy@infradead.org>

Looks good!  Thanks for sticking with this patch series.

Minor spelling fixes:

> -	/*
> -	 * Remove the vma's, and unmap the actual pages
> -	 */
> +	/* Detatch vmas from rbtree */

"Detach"

> +	/*
> +	 * mpx unmap need to be handled with write mmap_sem. It is safe to
> +	 * deal with it before unmap_region().
> +	 */

	 * mpx unmap needs to be called with mmap_sem held for write.
	 * It is safe to call it before unmap_region()

> +	ret = __do_munmap(mm, start, len, &uf, downgrade);
> +	/*
> +	 * Returning 1 indicates mmap_sem is down graded.
> +	 * But 1 is not legal return value of vm_munmap() and munmap(), reset
> +	 * it to 0 before return.
> +	 */

"downgraded" is one word.

Re: [RFC v10 PATCH 2/3] mm: unmap VM_HUGETLB mappings with optimized path

[Matthew Wilcox]

On Sat, Sep 15, 2018 at 04:34:58AM +0800, Yang Shi wrote:
> When unmapping VM_HUGETLB mappings, vm flags need to be updated. Since
> the vmas have been detached, so it sounds safe to update vm flags with
> read mmap_sem.
> 
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>

Reviewed-by: Matthew Wilcox <willy@infradead.org>

Re: [RFC v10 PATCH 3/3] mm: unmap VM_PFNMAP mappings with optimized path

[Matthew Wilcox]

On Sat, Sep 15, 2018 at 04:34:59AM +0800, Yang Shi wrote:
> When unmapping VM_PFNMAP mappings, vm flags need to be updated. Since
> the vmas have been detached, so it sounds safe to update vm flags with
> read mmap_sem.
> 
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>

Reviewed-by: Matthew Wilcox <willy@infradead.org>

Re: [RFC v10 PATCH 0/3] mm: zap pages with read mmap_sem in munmap for large mapping

[Matthew Wilcox]

On Sat, Sep 15, 2018 at 04:34:56AM +0800, Yang Shi wrote:
> 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 {
> 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.

Something I've been wondering about for a while is whether we should "sort"
the readers together.  ie if the acquirers look like this:

A write
B read
C read
D write
E read
F read
G write

then we should grant the lock to A, BCEF, D, G rather than A, BC, D, EF, G.
A quick way to test this is in __rwsem_down_read_failed_common do
something like:

-	if (list_empty(&sem->wait_list))
+	if (list_empty(&sem->wait_list)) {
 		adjustment += RWSEM_WAITING_BIAS;
+		list_add(&waiter.list, &sem->wait_list);
+	} else {
+		struct rwsem_waiter *first = list_first_entry(&sem->wait_list,
+						struct rwsem_waiter, list);
+		if (first.type == RWSEM_WAITING_FOR_READ)
+			list_add(&waiter.list, &sem->wait_list);
+		else
+			list_add_tail(&waiter.list, &sem->wait_list);
+	}
-	list_add_tail(&waiter.list, &sem->wait_list);

It'd be interesting to know if this makes any difference with your tests.

(this isn't perfect, of course; it'll fail to sort readers together if there's
a writer at the head of the queue; eg:

A write
B write
C read
D write
E read
F write
G read

but it won't do any worse than we have at the moment).

Re: [RFC v10 PATCH 1/3] mm: mmap: zap pages with read mmap_sem in munmap

[Yang Shi]

On 9/15/18 2:21 AM, Matthew Wilcox wrote:
> On Sat, Sep 15, 2018 at 04:34:57AM +0800, Yang Shi wrote:
>> Suggested-by: Michal Hocko <mhocko@kernel.org>
>> Suggested-by: Kirill A. Shutemov <kirill@shutemov.name>
>> Suggested-by: Matthew Wilcox <willy@infradead.org>
> Reviewed-by: Matthew Wilcox <willy@infradead.org>
>
> Looks good!  Thanks for sticking with this patch series.

Thanks for reviewing this patch series. I'm going to wait for one or two 
days to see whether anyone else has more comments before I have the 
spelling error fixed.

Yang

>
> Minor spelling fixes:
>
>> -	/*
>> -	 * Remove the vma's, and unmap the actual pages
>> -	 */
>> +	/* Detatch vmas from rbtree */
> "Detach"
>
>> +	/*
>> +	 * mpx unmap need to be handled with write mmap_sem. It is safe to
>> +	 * deal with it before unmap_region().
>> +	 */
> 	 * mpx unmap needs to be called with mmap_sem held for write.
> 	 * It is safe to call it before unmap_region()
>
>> +	ret = __do_munmap(mm, start, len, &uf, downgrade);
>> +	/*
>> +	 * Returning 1 indicates mmap_sem is down graded.
>> +	 * But 1 is not legal return value of vm_munmap() and munmap(), reset
>> +	 * it to 0 before return.
>> +	 */
> "downgraded" is one word.

Re: [RFC v10 PATCH 0/3] mm: zap pages with read mmap_sem in munmap for large mapping

[Yang Shi]

On 9/15/18 3:10 AM, Matthew Wilcox wrote:
> On Sat, Sep 15, 2018 at 04:34:56AM +0800, Yang Shi wrote:
>> 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 {
>> 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.
> Something I've been wondering about for a while is whether we should "sort"
> the readers together.  ie if the acquirers look like this:
>
> A write
> B read
> C read
> D write
> E read
> F read
> G write
>
> then we should grant the lock to A, BCEF, D, G rather than A, BC, D, EF, G.

I'm not sure how much this can help to the real world workload.

Typically, there are multi threads to contend for one mmap_sem. So, they 
are trying to read/write the same address space. There might be 
dependency or synchronization among them. Sorting read together might 
break the dependency?

Thanks,
Yang

> A quick way to test this is in __rwsem_down_read_failed_common do
> something like:
>
> -	if (list_empty(&sem->wait_list))
> +	if (list_empty(&sem->wait_list)) {
>   		adjustment += RWSEM_WAITING_BIAS;
> +		list_add(&waiter.list, &sem->wait_list);
> +	} else {
> +		struct rwsem_waiter *first = list_first_entry(&sem->wait_list,
> +						struct rwsem_waiter, list);
> +		if (first.type == RWSEM_WAITING_FOR_READ)
> +			list_add(&waiter.list, &sem->wait_list);
> +		else
> +			list_add_tail(&waiter.list, &sem->wait_list);
> +	}
> -	list_add_tail(&waiter.list, &sem->wait_list);
>
> It'd be interesting to know if this makes any difference with your tests.
>
> (this isn't perfect, of course; it'll fail to sort readers together if there's
> a writer at the head of the queue; eg:
>
> A write
> B write
> C read
> D write
> E read
> F write
> G read
>
> but it won't do any worse than we have at the moment).

Re: [RFC v10 PATCH 0/3] mm: zap pages with read mmap_sem in munmap for large mapping

[Matthew Wilcox]

On Mon, Sep 17, 2018 at 01:00:58PM -0700, Yang Shi wrote:
> On 9/15/18 3:10 AM, Matthew Wilcox wrote:
> > Something I've been wondering about for a while is whether we should "sort"
> > the readers together.  ie if the acquirers look like this:
> > 
> > A write
> > B read
> > C read
> > D write
> > E read
> > F read
> > G write
> > 
> > then we should grant the lock to A, BCEF, D, G rather than A, BC, D, EF, G.
> 
> I'm not sure how much this can help to the real world workload.
> 
> Typically, there are multi threads to contend for one mmap_sem. So, they are
> trying to read/write the same address space. There might be dependency or
> synchronization among them. Sorting read together might break the
> dependency?

I don't think that's true for the mmap_sem.  If one thread is trying to
get the sem for read then it's a page fault.  Another thread trying to
get the sem for write is trying to modify the address space.  If an
application depends on the ordering of an mmap vs a page fault, it has
to have its own synchronisation.