[PATCH 0/4] Improve anon_vma scalability for anon VMAs

[PATCH 0/4] Improve anon_vma scalability for anon VMAs

[Matthew Wilcox (Oracle)]

We have a 3x throughput improvement reported by Intel's kernel test
robot: https://lore.kernel.org/all/202404261055.c5e24608-oliver.sang@intel.com/

This is from delaying taking the mmap_lock for page faults until we
actually need the mmap_lock in order to assign an anon_vma to the vma.
It cleans up the page fault path a little by making the anon fault
handler more similar to the file fault handler.

Matthew Wilcox (Oracle) (4):
  mm: Assert the mmap_lock is held in __anon_vma_prepare()
  mm: Delay the check for a NULL anon_vma
  mm: Fix some minor per-VMA lock issues in userfaultfd
  mm: Optimise vmf_anon_prepare() for VMAs without an anon_vma

 mm/huge_memory.c |  6 ++++--
 mm/memory.c      | 42 +++++++++++++++++++++++++++---------------
 mm/rmap.c        |  3 +--
 mm/userfaultfd.c | 20 +++++++++-----------
 4 files changed, 41 insertions(+), 30 deletions(-)

-- 
2.43.0

[PATCH 1/4] mm: Assert the mmap_lock is held in __anon_vma_prepare()

[Matthew Wilcox (Oracle)]

Convert the comment into an assertion.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/rmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 2608c40dffad..619d4d65d99b 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -182,8 +182,6 @@ static void anon_vma_chain_link(struct vm_area_struct *vma,
  * for the new allocation. At the same time, we do not want
  * to do any locking for the common case of already having
  * an anon_vma.
- *
- * This must be called with the mmap_lock held for reading.
  */
 int __anon_vma_prepare(struct vm_area_struct *vma)
 {
@@ -191,6 +189,7 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
 	struct anon_vma *anon_vma, *allocated;
 	struct anon_vma_chain *avc;
 
+	mmap_assert_locked(mm);
 	might_sleep();
 
 	avc = anon_vma_chain_alloc(GFP_KERNEL);
-- 
2.43.0

Re: [PATCH 1/4] mm: Assert the mmap_lock is held in __anon_vma_prepare()

[Suren Baghdasaryan]

On Fri, Apr 26, 2024 at 7:45 AM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
>
> Convert the comment into an assertion.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Suren Baghdasaryan <surenb@google.com>

> ---
>  mm/rmap.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 2608c40dffad..619d4d65d99b 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -182,8 +182,6 @@ static void anon_vma_chain_link(struct vm_area_struct *vma,
>   * for the new allocation. At the same time, we do not want
>   * to do any locking for the common case of already having
>   * an anon_vma.
> - *
> - * This must be called with the mmap_lock held for reading.
>   */
>  int __anon_vma_prepare(struct vm_area_struct *vma)
>  {
> @@ -191,6 +189,7 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
>         struct anon_vma *anon_vma, *allocated;
>         struct anon_vma_chain *avc;
>
> +       mmap_assert_locked(mm);
>         might_sleep();
>
>         avc = anon_vma_chain_alloc(GFP_KERNEL);
> --
> 2.43.0
>

Re: [PATCH 1/4] mm: Assert the mmap_lock is held in __anon_vma_prepare()

[David Hildenbrand]

On 26.04.24 16:45, Matthew Wilcox (Oracle) wrote:
> Convert the comment into an assertion.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>   mm/rmap.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 2608c40dffad..619d4d65d99b 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -182,8 +182,6 @@ static void anon_vma_chain_link(struct vm_area_struct *vma,
>    * for the new allocation. At the same time, we do not want
>    * to do any locking for the common case of already having
>    * an anon_vma.
> - *
> - * This must be called with the mmap_lock held for reading.
>    */
>   int __anon_vma_prepare(struct vm_area_struct *vma)
>   {
> @@ -191,6 +189,7 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
>   	struct anon_vma *anon_vma, *allocated;
>   	struct anon_vma_chain *avc;
>   
> +	mmap_assert_locked(mm);
>   	might_sleep();
>   
>   	avc = anon_vma_chain_alloc(GFP_KERNEL);

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

[PATCH 2/4] mm: Delay the check for a NULL anon_vma

[Matthew Wilcox (Oracle)]

Instead of checking the anon_vma early in the fault path where all page
faults pay the cost, delay it until we know we're going to need the
anon_vma to be filled in.  This will have a slight negative effect on the
first fault in an anonymous VMA, but it shortens every other page fault.
It also makes the code slightly cleaner as the anon and file backed
fault handling look more similar.

The Intel kernel test bot reports a 3x improvement in vm-scalability
throughput with the small-allocs-mt test.  This is clearly an extreme
situation that won't be replicated in any real-world workload, but it's
a nice win.

https://lore.kernel.org/all/202404261055.c5e24608-oliver.sang@intel.com/

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
---
 mm/huge_memory.c |  6 ++++--
 mm/memory.c      | 29 ++++++++++++++++++-----------
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 19000fc2c43c..8261b5669397 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1057,11 +1057,13 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
 	gfp_t gfp;
 	struct folio *folio;
 	unsigned long haddr = vmf->address & HPAGE_PMD_MASK;
+	vm_fault_t ret;
 
 	if (!thp_vma_suitable_order(vma, haddr, PMD_ORDER))
 		return VM_FAULT_FALLBACK;
-	if (unlikely(anon_vma_prepare(vma)))
-		return VM_FAULT_OOM;
+	ret = vmf_anon_prepare(vmf);
+	if (ret)
+		return ret;
 	khugepaged_enter_vma(vma, vma->vm_flags);
 
 	if (!(vmf->flags & FAULT_FLAG_WRITE) &&
diff --git a/mm/memory.c b/mm/memory.c
index 6647685fd3c4..7dc112d3a7e4 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3214,6 +3214,21 @@ static inline vm_fault_t vmf_can_call_fault(const struct vm_fault *vmf)
 	return VM_FAULT_RETRY;
 }
 
+/**
+ * vmf_anon_prepare - Prepare to handle an anonymous fault.
+ * @vmf: The vm_fault descriptor passed from the fault handler.
+ *
+ * When preparing to insert an anonymous page into a VMA from a
+ * fault handler, call this function rather than anon_vma_prepare().
+ * If this vma does not already have an associated anon_vma and we are
+ * only protected by the per-VMA lock, the caller must retry with the
+ * mmap_lock held.  __anon_vma_prepare() will look at adjacent VMAs to
+ * determine if this VMA can share its anon_vma, and that's not safe to
+ * do with only the per-VMA lock held for this VMA.
+ *
+ * Return: 0 if fault handling can proceed.  Any other value should be
+ * returned to the caller.
+ */
 vm_fault_t vmf_anon_prepare(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
@@ -4434,8 +4449,9 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
 	}
 
 	/* Allocate our own private page. */
-	if (unlikely(anon_vma_prepare(vma)))
-		goto oom;
+	ret = vmf_anon_prepare(vmf);
+	if (ret)
+		return ret;
 	/* Returns NULL on OOM or ERR_PTR(-EAGAIN) if we must retry the fault */
 	folio = alloc_anon_folio(vmf);
 	if (IS_ERR(folio))
@@ -5823,15 +5839,6 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
 	if (!vma_start_read(vma))
 		goto inval;
 
-	/*
-	 * find_mergeable_anon_vma uses adjacent vmas which are not locked.
-	 * This check must happen after vma_start_read(); otherwise, a
-	 * concurrent mremap() with MREMAP_DONTUNMAP could dissociate the VMA
-	 * from its anon_vma.
-	 */
-	if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma))
-		goto inval_end_read;
-
 	/* Check since vm_start/vm_end might change before we lock the VMA */
 	if (unlikely(address < vma->vm_start || address >= vma->vm_end))
 		goto inval_end_read;
-- 
2.43.0

[PATCH 3/4] mm: Fix some minor per-VMA lock issues in userfaultfd

[Matthew Wilcox (Oracle)]

Rename lock_vma() to uffd_lock_vma() because it really is uffd specific.
Remove comment referencing unlock_vma() which doesn't exist.
Fix the comment about lock_vma_under_rcu() which I just made incorrect.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/userfaultfd.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index e6486923263c..defa5109cc62 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -56,17 +56,16 @@ struct vm_area_struct *find_vma_and_prepare_anon(struct mm_struct *mm,
 
 #ifdef CONFIG_PER_VMA_LOCK
 /*
- * lock_vma() - Lookup and lock vma corresponding to @address.
+ * uffd_lock_vma() - Lookup and lock vma corresponding to @address.
  * @mm: mm to search vma in.
  * @address: address that the vma should contain.
  *
- * Should be called without holding mmap_lock. vma should be unlocked after use
- * with unlock_vma().
+ * Should be called without holding mmap_lock.
  *
  * Return: A locked vma containing @address, -ENOENT if no vma is found, or
  * -ENOMEM if anon_vma couldn't be allocated.
  */
-static struct vm_area_struct *lock_vma(struct mm_struct *mm,
+static struct vm_area_struct *uffd_lock_vma(struct mm_struct *mm,
 				       unsigned long address)
 {
 	struct vm_area_struct *vma;
@@ -74,9 +73,8 @@ static struct vm_area_struct *lock_vma(struct mm_struct *mm,
 	vma = lock_vma_under_rcu(mm, address);
 	if (vma) {
 		/*
-		 * lock_vma_under_rcu() only checks anon_vma for private
-		 * anonymous mappings. But we need to ensure it is assigned in
-		 * private file-backed vmas as well.
+		 * We know we're going to need to use anon_vma, so check
+		 * that early.
 		 */
 		if (!(vma->vm_flags & VM_SHARED) && unlikely(!vma->anon_vma))
 			vma_end_read(vma);
@@ -107,7 +105,7 @@ static struct vm_area_struct *uffd_mfill_lock(struct mm_struct *dst_mm,
 {
 	struct vm_area_struct *dst_vma;
 
-	dst_vma = lock_vma(dst_mm, dst_start);
+	dst_vma = uffd_lock_vma(dst_mm, dst_start);
 	if (IS_ERR(dst_vma) || validate_dst_vma(dst_vma, dst_start + len))
 		return dst_vma;
 
@@ -1436,7 +1434,7 @@ static int uffd_move_lock(struct mm_struct *mm,
 	struct vm_area_struct *vma;
 	int err;
 
-	vma = lock_vma(mm, dst_start);
+	vma = uffd_lock_vma(mm, dst_start);
 	if (IS_ERR(vma))
 		return PTR_ERR(vma);
 
@@ -1451,7 +1449,7 @@ static int uffd_move_lock(struct mm_struct *mm,
 	}
 
 	/*
-	 * Using lock_vma() to get src_vma can lead to following deadlock:
+	 * Using uffd_lock_vma() to get src_vma can lead to following deadlock:
 	 *
 	 * Thread1				Thread2
 	 * -------				-------
@@ -1473,7 +1471,7 @@ static int uffd_move_lock(struct mm_struct *mm,
 	err = find_vmas_mm_locked(mm, dst_start, src_start, dst_vmap, src_vmap);
 	if (!err) {
 		/*
-		 * See comment in lock_vma() as to why not using
+		 * See comment in uffd_lock_vma() as to why not using
 		 * vma_start_read() here.
 		 */
 		down_read(&(*dst_vmap)->vm_lock->lock);
-- 
2.43.0

Re: [PATCH 3/4] mm: Fix some minor per-VMA lock issues in userfaultfd

[Suren Baghdasaryan]

On Fri, Apr 26, 2024 at 7:45 AM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
>
> Rename lock_vma() to uffd_lock_vma() because it really is uffd specific.
> Remove comment referencing unlock_vma() which doesn't exist.
> Fix the comment about lock_vma_under_rcu() which I just made incorrect.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Reviewed-by: Suren Baghdasaryan <surenb@google.com>

> ---
>  mm/userfaultfd.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index e6486923263c..defa5109cc62 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -56,17 +56,16 @@ struct vm_area_struct *find_vma_and_prepare_anon(struct mm_struct *mm,
>
>  #ifdef CONFIG_PER_VMA_LOCK
>  /*
> - * lock_vma() - Lookup and lock vma corresponding to @address.
> + * uffd_lock_vma() - Lookup and lock vma corresponding to @address.
>   * @mm: mm to search vma in.
>   * @address: address that the vma should contain.
>   *
> - * Should be called without holding mmap_lock. vma should be unlocked after use
> - * with unlock_vma().
> + * Should be called without holding mmap_lock.
>   *
>   * Return: A locked vma containing @address, -ENOENT if no vma is found, or
>   * -ENOMEM if anon_vma couldn't be allocated.
>   */
> -static struct vm_area_struct *lock_vma(struct mm_struct *mm,
> +static struct vm_area_struct *uffd_lock_vma(struct mm_struct *mm,
>                                        unsigned long address)
>  {
>         struct vm_area_struct *vma;
> @@ -74,9 +73,8 @@ static struct vm_area_struct *lock_vma(struct mm_struct *mm,
>         vma = lock_vma_under_rcu(mm, address);
>         if (vma) {
>                 /*
> -                * lock_vma_under_rcu() only checks anon_vma for private
> -                * anonymous mappings. But we need to ensure it is assigned in
> -                * private file-backed vmas as well.
> +                * We know we're going to need to use anon_vma, so check
> +                * that early.
>                  */
>                 if (!(vma->vm_flags & VM_SHARED) && unlikely(!vma->anon_vma))
>                         vma_end_read(vma);
> @@ -107,7 +105,7 @@ static struct vm_area_struct *uffd_mfill_lock(struct mm_struct *dst_mm,
>  {
>         struct vm_area_struct *dst_vma;
>
> -       dst_vma = lock_vma(dst_mm, dst_start);
> +       dst_vma = uffd_lock_vma(dst_mm, dst_start);
>         if (IS_ERR(dst_vma) || validate_dst_vma(dst_vma, dst_start + len))
>                 return dst_vma;
>
> @@ -1436,7 +1434,7 @@ static int uffd_move_lock(struct mm_struct *mm,
>         struct vm_area_struct *vma;
>         int err;
>
> -       vma = lock_vma(mm, dst_start);
> +       vma = uffd_lock_vma(mm, dst_start);
>         if (IS_ERR(vma))
>                 return PTR_ERR(vma);
>
> @@ -1451,7 +1449,7 @@ static int uffd_move_lock(struct mm_struct *mm,
>         }
>
>         /*
> -        * Using lock_vma() to get src_vma can lead to following deadlock:
> +        * Using uffd_lock_vma() to get src_vma can lead to following deadlock:
>          *
>          * Thread1                              Thread2
>          * -------                              -------
> @@ -1473,7 +1471,7 @@ static int uffd_move_lock(struct mm_struct *mm,
>         err = find_vmas_mm_locked(mm, dst_start, src_start, dst_vmap, src_vmap);
>         if (!err) {
>                 /*
> -                * See comment in lock_vma() as to why not using
> +                * See comment in uffd_lock_vma() as to why not using
>                  * vma_start_read() here.
>                  */
>                 down_read(&(*dst_vmap)->vm_lock->lock);
> --
> 2.43.0
>

[PATCH 4/4] mm: Optimise vmf_anon_prepare() for VMAs without an anon_vma

[Matthew Wilcox (Oracle)]

If the mmap_lock can be taken for read, we can call __anon_vma_prepare()
while holding it, saving ourselves a trip back through the fault handler.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Reviewed-by: Jann Horn <jannh@google.com>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
---
 mm/memory.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 7dc112d3a7e4..b5453b86ec4b 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3232,16 +3232,21 @@ static inline vm_fault_t vmf_can_call_fault(const struct vm_fault *vmf)
 vm_fault_t vmf_anon_prepare(struct vm_fault *vmf)
 {
 	struct vm_area_struct *vma = vmf->vma;
+	vm_fault_t ret = 0;
 
 	if (likely(vma->anon_vma))
 		return 0;
 	if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
-		vma_end_read(vma);
-		return VM_FAULT_RETRY;
+		if (!mmap_read_trylock(vma->vm_mm)) {
+			vma_end_read(vma);
+			return VM_FAULT_RETRY;
+		}
 	}
 	if (__anon_vma_prepare(vma))
-		return VM_FAULT_OOM;
-	return 0;
+		ret = VM_FAULT_OOM;
+	if (vmf->flags & FAULT_FLAG_VMA_LOCK)
+		mmap_read_unlock(vma->vm_mm);
+	return ret;
 }
 
 /*
-- 
2.43.0

Re: [PATCH 4/4] mm: Optimise vmf_anon_prepare() for VMAs without an anon_vma

[David Hildenbrand]

On 26.04.24 16:45, Matthew Wilcox (Oracle) wrote:
> If the mmap_lock can be taken for read, we can call __anon_vma_prepare()
> while holding it, saving ourselves a trip back through the fault handler.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Reviewed-by: Jann Horn <jannh@google.com>
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>
> ---
>   mm/memory.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 7dc112d3a7e4..b5453b86ec4b 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3232,16 +3232,21 @@ static inline vm_fault_t vmf_can_call_fault(const struct vm_fault *vmf)
>   vm_fault_t vmf_anon_prepare(struct vm_fault *vmf)
>   {
>   	struct vm_area_struct *vma = vmf->vma;
> +	vm_fault_t ret = 0;
>   
>   	if (likely(vma->anon_vma))
>   		return 0;
>   	if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
> -		vma_end_read(vma);
> -		return VM_FAULT_RETRY;
> +		if (!mmap_read_trylock(vma->vm_mm)) {
> +			vma_end_read(vma);
> +			return VM_FAULT_RETRY;
> +		}
>   	}
>   	if (__anon_vma_prepare(vma))
> -		return VM_FAULT_OOM;
> -	return 0;
> +		ret = VM_FAULT_OOM;
> +	if (vmf->flags & FAULT_FLAG_VMA_LOCK)
> +		mmap_read_unlock(vma->vm_mm);
> +	return ret;
>   }
>   
>   /*

Fancy

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb