[PATCH v1 0/3] mm/rmap: convert page_move_anon_rmap() to folio_move_anon_rmap()

[PATCH v1 0/3] mm/rmap: convert page_move_anon_rmap() to folio_move_anon_rmap()

[David Hildenbrand]

Convert page_move_anon_rmap() to folio_move_anon_rmap(), letting the
callers handle PageAnonExclusive. I'm including cleanup patch #3 because it
fits into the picture and can be done cleaner by the conversion.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
Cc: Muchun Song <muchun.song@linux.dev>
Cc: Suren Baghdasaryan <surenb@google.com>

David Hildenbrand (3):
  mm/rmap: move SetPageAnonExclusive() out of page_move_anon_rmap()
  mm/rmap: convert page_move_anon_rmap() to folio_move_anon_rmap()
  memory: move exclusivity detection in do_wp_page() into
    wp_can_reuse_anon_folio()

 include/linux/rmap.h |  2 +-
 mm/huge_memory.c     |  3 +-
 mm/hugetlb.c         |  6 ++-
 mm/memory.c          | 87 +++++++++++++++++++++++---------------------
 mm/rmap.c            | 17 ++++-----
 5 files changed, 59 insertions(+), 56 deletions(-)

-- 
2.41.0

[PATCH v1 1/3] mm/rmap: move SetPageAnonExclusive() out of page_move_anon_rmap()

[David Hildenbrand]

Let's move it into the caller: there is a difference between whether an
anon folio can only be mapped by one process (e.g., into one VMA), and
whether it is truly exclusive (e.g., no references -- including GUP --
from other processes).

Further, for large folios the page might not actually be pointing at the
head page of the folio, so it better be handled in the caller. This is a
preparation for converting page_move_anon_rmap() to consume a folio.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/huge_memory.c | 1 +
 mm/hugetlb.c     | 4 +++-
 mm/memory.c      | 1 +
 mm/rmap.c        | 1 -
 4 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index e54fb9c542bb..01d0d65ece13 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1506,6 +1506,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
 		pmd_t entry;
 
 		page_move_anon_rmap(page, vma);
+		SetPageAnonExclusive(page);
 		folio_unlock(folio);
 reuse:
 		if (unlikely(unshare)) {
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 9c22297d9c57..24591fc145ff 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5460,8 +5460,10 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
 	 * owner and can reuse this page.
 	 */
 	if (folio_mapcount(old_folio) == 1 && folio_test_anon(old_folio)) {
-		if (!PageAnonExclusive(&old_folio->page))
+		if (!PageAnonExclusive(&old_folio->page)) {
 			page_move_anon_rmap(&old_folio->page, vma);
+			SetPageAnonExclusive(&old_folio->page);
+		}
 		if (likely(!unshare))
 			set_huge_ptep_writable(vma, haddr, ptep);
 
diff --git a/mm/memory.c b/mm/memory.c
index d4820802b01b..9de231c92769 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3484,6 +3484,7 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
 		 * sunglasses. Hit it.
 		 */
 		page_move_anon_rmap(vmf->page, vma);
+		SetPageAnonExclusive(vmf->page);
 		folio_unlock(folio);
 reuse:
 		if (unlikely(unshare)) {
diff --git a/mm/rmap.c b/mm/rmap.c
index 77222adccda1..854ccbd66954 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1165,7 +1165,6 @@ void page_move_anon_rmap(struct page *page, struct vm_area_struct *vma)
 	 * folio_test_anon()) will not see one without the other.
 	 */
 	WRITE_ONCE(folio->mapping, anon_vma);
-	SetPageAnonExclusive(page);
 }
 
 /**
-- 
2.41.0

Re: [PATCH v1 1/3] mm/rmap: move SetPageAnonExclusive() out of page_move_anon_rmap()

[Suren Baghdasaryan]

On Mon, Oct 2, 2023 at 7:29 AM David Hildenbrand <david@redhat.com> wrote:
>
> Let's move it into the caller: there is a difference between whether an
> anon folio can only be mapped by one process (e.g., into one VMA), and
> whether it is truly exclusive (e.g., no references -- including GUP --
> from other processes).
>
> Further, for large folios the page might not actually be pointing at the
> head page of the folio, so it better be handled in the caller. This is a
> preparation for converting page_move_anon_rmap() to consume a folio.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

> ---
>  mm/huge_memory.c | 1 +
>  mm/hugetlb.c     | 4 +++-
>  mm/memory.c      | 1 +
>  mm/rmap.c        | 1 -
>  4 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index e54fb9c542bb..01d0d65ece13 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1506,6 +1506,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
>                 pmd_t entry;
>
>                 page_move_anon_rmap(page, vma);
> +               SetPageAnonExclusive(page);
>                 folio_unlock(folio);
>  reuse:
>                 if (unlikely(unshare)) {
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 9c22297d9c57..24591fc145ff 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5460,8 +5460,10 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
>          * owner and can reuse this page.
>          */
>         if (folio_mapcount(old_folio) == 1 && folio_test_anon(old_folio)) {
> -               if (!PageAnonExclusive(&old_folio->page))
> +               if (!PageAnonExclusive(&old_folio->page)) {
>                         page_move_anon_rmap(&old_folio->page, vma);
> +                       SetPageAnonExclusive(&old_folio->page);
> +               }
>                 if (likely(!unshare))
>                         set_huge_ptep_writable(vma, haddr, ptep);
>
> diff --git a/mm/memory.c b/mm/memory.c
> index d4820802b01b..9de231c92769 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3484,6 +3484,7 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
>                  * sunglasses. Hit it.
>                  */
>                 page_move_anon_rmap(vmf->page, vma);
> +               SetPageAnonExclusive(vmf->page);
>                 folio_unlock(folio);
>  reuse:
>                 if (unlikely(unshare)) {
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 77222adccda1..854ccbd66954 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1165,7 +1165,6 @@ void page_move_anon_rmap(struct page *page, struct vm_area_struct *vma)
>          * folio_test_anon()) will not see one without the other.
>          */
>         WRITE_ONCE(folio->mapping, anon_vma);
> -       SetPageAnonExclusive(page);
>  }
>
>  /**
> --
> 2.41.0
>

Re: [PATCH v1 1/3] mm/rmap: move SetPageAnonExclusive() out of page_move_anon_rmap()

[Vishal Moola]

On Mon, Oct 02, 2023 at 04:29:47PM +0200, David Hildenbrand wrote:
> Let's move it into the caller: there is a difference between whether an
> anon folio can only be mapped by one process (e.g., into one VMA), and
> whether it is truly exclusive (e.g., no references -- including GUP --
> from other processes).
> 
> Further, for large folios the page might not actually be pointing at the
> head page of the folio, so it better be handled in the caller. This is a
> preparation for converting page_move_anon_rmap() to consume a folio.

Reviewed-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>

[PATCH v1 2/3] mm/rmap: convert page_move_anon_rmap() to folio_move_anon_rmap()

[David Hildenbrand]

Let's convert it to consume a folio.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/linux/rmap.h |  2 +-
 mm/huge_memory.c     |  2 +-
 mm/hugetlb.c         |  2 +-
 mm/memory.c          |  2 +-
 mm/rmap.c            | 16 +++++++---------
 5 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index 064b432a4033..8034eda972e5 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -194,7 +194,7 @@ typedef int __bitwise rmap_t;
 /*
  * rmap interfaces called when adding or removing pte of page
  */
-void page_move_anon_rmap(struct page *, struct vm_area_struct *);
+void folio_move_anon_rmap(struct folio *, struct vm_area_struct *);
 void page_add_anon_rmap(struct page *, struct vm_area_struct *,
 		unsigned long address, rmap_t flags);
 void page_add_new_anon_rmap(struct page *, struct vm_area_struct *,
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 01d0d65ece13..08245226ccb8 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1505,7 +1505,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
 	if (folio_ref_count(folio) == 1) {
 		pmd_t entry;
 
-		page_move_anon_rmap(page, vma);
+		folio_move_anon_rmap(folio, vma);
 		SetPageAnonExclusive(page);
 		folio_unlock(folio);
 reuse:
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 24591fc145ff..e52c6048e74f 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5461,7 +5461,7 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
 	 */
 	if (folio_mapcount(old_folio) == 1 && folio_test_anon(old_folio)) {
 		if (!PageAnonExclusive(&old_folio->page)) {
-			page_move_anon_rmap(&old_folio->page, vma);
+			folio_move_anon_rmap(old_folio, vma);
 			SetPageAnonExclusive(&old_folio->page);
 		}
 		if (likely(!unshare))
diff --git a/mm/memory.c b/mm/memory.c
index 9de231c92769..1f0e3317cbdd 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3483,7 +3483,7 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
 		 * and the folio is locked, it's dark out, and we're wearing
 		 * sunglasses. Hit it.
 		 */
-		page_move_anon_rmap(vmf->page, vma);
+		folio_move_anon_rmap(folio, vma);
 		SetPageAnonExclusive(vmf->page);
 		folio_unlock(folio);
 reuse:
diff --git a/mm/rmap.c b/mm/rmap.c
index 854ccbd66954..37f05f33559b 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1141,19 +1141,17 @@ int folio_total_mapcount(struct folio *folio)
 }
 
 /**
- * page_move_anon_rmap - move a page to our anon_vma
- * @page:	the page to move to our anon_vma
- * @vma:	the vma the page belongs to
+ * folio_move_anon_rmap - move a folio to our anon_vma
+ * @page:	The folio to move to our anon_vma
+ * @vma:	The vma the folio belongs to
  *
- * When a page belongs exclusively to one process after a COW event,
- * that page can be moved into the anon_vma that belongs to just that
- * process, so the rmap code will not search the parent or sibling
- * processes.
+ * When a folio belongs exclusively to one process after a COW event,
+ * that folio can be moved into the anon_vma that belongs to just that
+ * process, so the rmap code will not search the parent or sibling processes.
  */
-void page_move_anon_rmap(struct page *page, struct vm_area_struct *vma)
+void folio_move_anon_rmap(struct folio *folio, struct vm_area_struct *vma)
 {
 	void *anon_vma = vma->anon_vma;
-	struct folio *folio = page_folio(page);
 
 	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
 	VM_BUG_ON_VMA(!anon_vma, vma);
-- 
2.41.0

Re: [PATCH v1 2/3] mm/rmap: convert page_move_anon_rmap() to folio_move_anon_rmap()

[Suren Baghdasaryan]

On Mon, Oct 2, 2023 at 7:29 AM David Hildenbrand <david@redhat.com> wrote:
>
> Let's convert it to consume a folio.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Thanks!

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


> ---
>  include/linux/rmap.h |  2 +-
>  mm/huge_memory.c     |  2 +-
>  mm/hugetlb.c         |  2 +-
>  mm/memory.c          |  2 +-
>  mm/rmap.c            | 16 +++++++---------
>  5 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index 064b432a4033..8034eda972e5 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -194,7 +194,7 @@ typedef int __bitwise rmap_t;
>  /*
>   * rmap interfaces called when adding or removing pte of page
>   */
> -void page_move_anon_rmap(struct page *, struct vm_area_struct *);
> +void folio_move_anon_rmap(struct folio *, struct vm_area_struct *);
>  void page_add_anon_rmap(struct page *, struct vm_area_struct *,
>                 unsigned long address, rmap_t flags);
>  void page_add_new_anon_rmap(struct page *, struct vm_area_struct *,
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 01d0d65ece13..08245226ccb8 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1505,7 +1505,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
>         if (folio_ref_count(folio) == 1) {
>                 pmd_t entry;
>
> -               page_move_anon_rmap(page, vma);
> +               folio_move_anon_rmap(folio, vma);
>                 SetPageAnonExclusive(page);
>                 folio_unlock(folio);
>  reuse:
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 24591fc145ff..e52c6048e74f 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -5461,7 +5461,7 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
>          */
>         if (folio_mapcount(old_folio) == 1 && folio_test_anon(old_folio)) {
>                 if (!PageAnonExclusive(&old_folio->page)) {
> -                       page_move_anon_rmap(&old_folio->page, vma);
> +                       folio_move_anon_rmap(old_folio, vma);
>                         SetPageAnonExclusive(&old_folio->page);
>                 }
>                 if (likely(!unshare))
> diff --git a/mm/memory.c b/mm/memory.c
> index 9de231c92769..1f0e3317cbdd 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3483,7 +3483,7 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
>                  * and the folio is locked, it's dark out, and we're wearing
>                  * sunglasses. Hit it.
>                  */
> -               page_move_anon_rmap(vmf->page, vma);
> +               folio_move_anon_rmap(folio, vma);
>                 SetPageAnonExclusive(vmf->page);
>                 folio_unlock(folio);
>  reuse:
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 854ccbd66954..37f05f33559b 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1141,19 +1141,17 @@ int folio_total_mapcount(struct folio *folio)
>  }
>
>  /**
> - * page_move_anon_rmap - move a page to our anon_vma
> - * @page:      the page to move to our anon_vma
> - * @vma:       the vma the page belongs to
> + * folio_move_anon_rmap - move a folio to our anon_vma
> + * @page:      The folio to move to our anon_vma
> + * @vma:       The vma the folio belongs to
>   *
> - * When a page belongs exclusively to one process after a COW event,
> - * that page can be moved into the anon_vma that belongs to just that
> - * process, so the rmap code will not search the parent or sibling
> - * processes.
> + * When a folio belongs exclusively to one process after a COW event,
> + * that folio can be moved into the anon_vma that belongs to just that
> + * process, so the rmap code will not search the parent or sibling processes.
>   */
> -void page_move_anon_rmap(struct page *page, struct vm_area_struct *vma)
> +void folio_move_anon_rmap(struct folio *folio, struct vm_area_struct *vma)
>  {
>         void *anon_vma = vma->anon_vma;
> -       struct folio *folio = page_folio(page);
>
>         VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
>         VM_BUG_ON_VMA(!anon_vma, vma);
> --
> 2.41.0
>

Re: [PATCH v1 2/3] mm/rmap: convert page_move_anon_rmap() to folio_move_anon_rmap()

[Vishal Moola]

On Mon, Oct 02, 2023 at 04:29:48PM +0200, David Hildenbrand wrote:
> Let's convert it to consume a folio.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>

[PATCH v1 3/3] memory: move exclusivity detection in do_wp_page() into wp_can_reuse_anon_folio()

[David Hildenbrand]

Let's clean up do_wp_page() a bit, removing two labels and making it
a easier to read.

wp_can_reuse_anon_folio() now only operates on the whole folio. Move the
SetPageAnonExclusive() out into do_wp_page(). No need to do this under
page lock -- the page table lock is sufficient.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/memory.c | 88 +++++++++++++++++++++++++++--------------------------
 1 file changed, 45 insertions(+), 43 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 1f0e3317cbdd..512f6f05620e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3358,6 +3358,44 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf, struct folio *folio)
 	return ret;
 }
 
+static bool wp_can_reuse_anon_folio(struct folio *folio,
+				    struct vm_area_struct *vma)
+{
+	/*
+	 * We have to verify under folio lock: these early checks are
+	 * just an optimization to avoid locking the folio and freeing
+	 * the swapcache if there is little hope that we can reuse.
+	 *
+	 * KSM doesn't necessarily raise the folio refcount.
+	 */
+	if (folio_test_ksm(folio) || folio_ref_count(folio) > 3)
+		return false;
+	if (!folio_test_lru(folio))
+		/*
+		 * We cannot easily detect+handle references from
+		 * remote LRU caches or references to LRU folios.
+		 */
+		lru_add_drain();
+	if (folio_ref_count(folio) > 1 + folio_test_swapcache(folio))
+		return false;
+	if (!folio_trylock(folio))
+		return false;
+	if (folio_test_swapcache(folio))
+		folio_free_swap(folio);
+	if (folio_test_ksm(folio) || folio_ref_count(folio) != 1) {
+		folio_unlock(folio);
+		return false;
+	}
+	/*
+	 * Ok, we've got the only folio reference from our mapping
+	 * and the folio is locked, it's dark out, and we're wearing
+	 * sunglasses. Hit it.
+	 */
+	folio_move_anon_rmap(folio, vma);
+	folio_unlock(folio);
+	return true;
+}
+
 /*
  * This routine handles present pages, when
  * * users try to write to a shared page (FAULT_FLAG_WRITE)
@@ -3444,49 +3482,14 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
 	/*
 	 * Private mapping: create an exclusive anonymous page copy if reuse
 	 * is impossible. We might miss VM_WRITE for FOLL_FORCE handling.
+	 *
+	 * If we encounter a page that is marked exclusive, we must reuse
+	 * the page without further checks.
 	 */
-	if (folio && folio_test_anon(folio)) {
-		/*
-		 * If the page is exclusive to this process we must reuse the
-		 * page without further checks.
-		 */
-		if (PageAnonExclusive(vmf->page))
-			goto reuse;
-
-		/*
-		 * We have to verify under folio lock: these early checks are
-		 * just an optimization to avoid locking the folio and freeing
-		 * the swapcache if there is little hope that we can reuse.
-		 *
-		 * KSM doesn't necessarily raise the folio refcount.
-		 */
-		if (folio_test_ksm(folio) || folio_ref_count(folio) > 3)
-			goto copy;
-		if (!folio_test_lru(folio))
-			/*
-			 * We cannot easily detect+handle references from
-			 * remote LRU caches or references to LRU folios.
-			 */
-			lru_add_drain();
-		if (folio_ref_count(folio) > 1 + folio_test_swapcache(folio))
-			goto copy;
-		if (!folio_trylock(folio))
-			goto copy;
-		if (folio_test_swapcache(folio))
-			folio_free_swap(folio);
-		if (folio_test_ksm(folio) || folio_ref_count(folio) != 1) {
-			folio_unlock(folio);
-			goto copy;
-		}
-		/*
-		 * Ok, we've got the only folio reference from our mapping
-		 * and the folio is locked, it's dark out, and we're wearing
-		 * sunglasses. Hit it.
-		 */
-		folio_move_anon_rmap(folio, vma);
-		SetPageAnonExclusive(vmf->page);
-		folio_unlock(folio);
-reuse:
+	if (folio && folio_test_anon(folio) &&
+	    (PageAnonExclusive(vmf->page) || wp_can_reuse_anon_folio(folio, vma))) {
+		if (!PageAnonExclusive(vmf->page))
+			SetPageAnonExclusive(vmf->page);
 		if (unlikely(unshare)) {
 			pte_unmap_unlock(vmf->pte, vmf->ptl);
 			return 0;
@@ -3494,7 +3497,6 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
 		wp_page_reuse(vmf);
 		return 0;
 	}
-copy:
 	/*
 	 * Ok, we need to copy. Oh, well..
 	 */
-- 
2.41.0

Re: [PATCH v1 3/3] memory: move exclusivity detection in do_wp_page() into wp_can_reuse_anon_folio()

[Suren Baghdasaryan]

On Mon, Oct 2, 2023 at 7:29 AM David Hildenbrand <david@redhat.com> wrote:
>
> Let's clean up do_wp_page() a bit, removing two labels and making it
> a easier to read.
>
> wp_can_reuse_anon_folio() now only operates on the whole folio. Move the
> SetPageAnonExclusive() out into do_wp_page(). No need to do this under
> page lock -- the page table lock is sufficient.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/memory.c | 88 +++++++++++++++++++++++++++--------------------------
>  1 file changed, 45 insertions(+), 43 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 1f0e3317cbdd..512f6f05620e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3358,6 +3358,44 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf, struct folio *folio)
>         return ret;
>  }
>
> +static bool wp_can_reuse_anon_folio(struct folio *folio,
> +                                   struct vm_area_struct *vma)

Since this function is calling folio_move_anon_rmap(), I would suggest
changing its name to wp_reuse_anon_folio(). This would clarify that
it's actually doing that operation instead of just checking if it's
possible. That would also let us keep unconditional
SetPageAnonExclusive() in it and do that under folio lock like it used
to do (keeping rules simple). Other than that, it looks good to me.

> +{
> +       /*
> +        * We have to verify under folio lock: these early checks are
> +        * just an optimization to avoid locking the folio and freeing
> +        * the swapcache if there is little hope that we can reuse.
> +        *
> +        * KSM doesn't necessarily raise the folio refcount.
> +        */
> +       if (folio_test_ksm(folio) || folio_ref_count(folio) > 3)
> +               return false;
> +       if (!folio_test_lru(folio))
> +               /*
> +                * We cannot easily detect+handle references from
> +                * remote LRU caches or references to LRU folios.
> +                */
> +               lru_add_drain();
> +       if (folio_ref_count(folio) > 1 + folio_test_swapcache(folio))
> +               return false;
> +       if (!folio_trylock(folio))
> +               return false;
> +       if (folio_test_swapcache(folio))
> +               folio_free_swap(folio);
> +       if (folio_test_ksm(folio) || folio_ref_count(folio) != 1) {
> +               folio_unlock(folio);
> +               return false;
> +       }
> +       /*
> +        * Ok, we've got the only folio reference from our mapping
> +        * and the folio is locked, it's dark out, and we're wearing
> +        * sunglasses. Hit it.
> +        */
> +       folio_move_anon_rmap(folio, vma);
> +       folio_unlock(folio);
> +       return true;
> +}
> +
>  /*
>   * This routine handles present pages, when
>   * * users try to write to a shared page (FAULT_FLAG_WRITE)
> @@ -3444,49 +3482,14 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
>         /*
>          * Private mapping: create an exclusive anonymous page copy if reuse
>          * is impossible. We might miss VM_WRITE for FOLL_FORCE handling.
> +        *
> +        * If we encounter a page that is marked exclusive, we must reuse
> +        * the page without further checks.
>          */
> -       if (folio && folio_test_anon(folio)) {
> -               /*
> -                * If the page is exclusive to this process we must reuse the
> -                * page without further checks.
> -                */
> -               if (PageAnonExclusive(vmf->page))
> -                       goto reuse;
> -
> -               /*
> -                * We have to verify under folio lock: these early checks are
> -                * just an optimization to avoid locking the folio and freeing
> -                * the swapcache if there is little hope that we can reuse.
> -                *
> -                * KSM doesn't necessarily raise the folio refcount.
> -                */
> -               if (folio_test_ksm(folio) || folio_ref_count(folio) > 3)
> -                       goto copy;
> -               if (!folio_test_lru(folio))
> -                       /*
> -                        * We cannot easily detect+handle references from
> -                        * remote LRU caches or references to LRU folios.
> -                        */
> -                       lru_add_drain();
> -               if (folio_ref_count(folio) > 1 + folio_test_swapcache(folio))
> -                       goto copy;
> -               if (!folio_trylock(folio))
> -                       goto copy;
> -               if (folio_test_swapcache(folio))
> -                       folio_free_swap(folio);
> -               if (folio_test_ksm(folio) || folio_ref_count(folio) != 1) {
> -                       folio_unlock(folio);
> -                       goto copy;
> -               }
> -               /*
> -                * Ok, we've got the only folio reference from our mapping
> -                * and the folio is locked, it's dark out, and we're wearing
> -                * sunglasses. Hit it.
> -                */
> -               folio_move_anon_rmap(folio, vma);
> -               SetPageAnonExclusive(vmf->page);
> -               folio_unlock(folio);
> -reuse:
> +       if (folio && folio_test_anon(folio) &&
> +           (PageAnonExclusive(vmf->page) || wp_can_reuse_anon_folio(folio, vma))) {
> +               if (!PageAnonExclusive(vmf->page))
> +                       SetPageAnonExclusive(vmf->page);
>                 if (unlikely(unshare)) {
>                         pte_unmap_unlock(vmf->pte, vmf->ptl);
>                         return 0;
> @@ -3494,7 +3497,6 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
>                 wp_page_reuse(vmf);
>                 return 0;
>         }
> -copy:
>         /*
>          * Ok, we need to copy. Oh, well..
>          */
> --
> 2.41.0
>

Re: [PATCH v1 3/3] memory: move exclusivity detection in do_wp_page() into wp_can_reuse_anon_folio()

[David Hildenbrand]

On 03.10.23 19:05, Suren Baghdasaryan wrote:
> On Mon, Oct 2, 2023 at 7:29 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> Let's clean up do_wp_page() a bit, removing two labels and making it
>> a easier to read.
>>
>> wp_can_reuse_anon_folio() now only operates on the whole folio. Move the
>> SetPageAnonExclusive() out into do_wp_page(). No need to do this under
>> page lock -- the page table lock is sufficient.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   mm/memory.c | 88 +++++++++++++++++++++++++++--------------------------
>>   1 file changed, 45 insertions(+), 43 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 1f0e3317cbdd..512f6f05620e 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3358,6 +3358,44 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf, struct folio *folio)
>>          return ret;
>>   }
>>

Sorry for the late response.

>> +static bool wp_can_reuse_anon_folio(struct folio *folio,
>> +                                   struct vm_area_struct *vma)
> 
> Since this function is calling folio_move_anon_rmap(), I would suggest
> changing its name to wp_reuse_anon_folio(). This would clarify that

folio_move_anon_rmap() is *not* the reuse part, it's just an rmap 
optimization. You could remove the call here and the whole thing would 
still work :) In fact, we can call folio_move_anon_rmap() whenever we 
sure the folio belongs to a single VMA only and we're holding the page 
lock. ... but we cannot always reuse a folio in that case because there 
might be GUP references from another process.

Reuse is
* Setting PageAnonExclusive
* Write fault: wunprotect the page -> wp_page_reuse()

I went a bit back and forth while cleaning that function up, but calling 
it wp_reuse_anon_folio() would end up being confusing with 
wp_page_reuse() called afterwards [we should probably rename that one to 
wp_page_wunprotect() independently]. So I prefer to leave the actual 
(sub)page reuse part in the caller.

> it's actually doing that operation instead of just checking if it's
> possible. That would also let us keep unconditional
> SetPageAnonExclusive() in it and do that under folio lock like it used
> to do (keeping rules simple). Other than that, it looks good to me.

I really want to avoid passing a "struct page" to that function; once 
we're dealing with PTE-mapped THP, the page might actually be a tail 
page of the folio.

Thanks!

-- 
Cheers,

David / dhildenb

Re: [PATCH v1 3/3] memory: move exclusivity detection in do_wp_page() into wp_can_reuse_anon_folio()

[Suren Baghdasaryan]

On Mon, Oct 9, 2023 at 3:03 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 03.10.23 19:05, Suren Baghdasaryan wrote:
> > On Mon, Oct 2, 2023 at 7:29 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> Let's clean up do_wp_page() a bit, removing two labels and making it
> >> a easier to read.
> >>
> >> wp_can_reuse_anon_folio() now only operates on the whole folio. Move the
> >> SetPageAnonExclusive() out into do_wp_page(). No need to do this under
> >> page lock -- the page table lock is sufficient.
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>   mm/memory.c | 88 +++++++++++++++++++++++++++--------------------------
> >>   1 file changed, 45 insertions(+), 43 deletions(-)
> >>
> >> diff --git a/mm/memory.c b/mm/memory.c
> >> index 1f0e3317cbdd..512f6f05620e 100644
> >> --- a/mm/memory.c
> >> +++ b/mm/memory.c
> >> @@ -3358,6 +3358,44 @@ static vm_fault_t wp_page_shared(struct vm_fault *vmf, struct folio *folio)
> >>          return ret;
> >>   }
> >>
>
> Sorry for the late response.
>
> >> +static bool wp_can_reuse_anon_folio(struct folio *folio,
> >> +                                   struct vm_area_struct *vma)
> >
> > Since this function is calling folio_move_anon_rmap(), I would suggest
> > changing its name to wp_reuse_anon_folio(). This would clarify that
>
> folio_move_anon_rmap() is *not* the reuse part, it's just an rmap
> optimization. You could remove the call here and the whole thing would
> still work :) In fact, we can call folio_move_anon_rmap() whenever we
> sure the folio belongs to a single VMA only and we're holding the page
> lock. ... but we cannot always reuse a folio in that case because there
> might be GUP references from another process.
>
> Reuse is
> * Setting PageAnonExclusive
> * Write fault: wunprotect the page -> wp_page_reuse()

Ok, fair enough. It's not the reuse, only a preparation step. My
concern is that wp_can_reuse_anon_folio() with a bool being returned
looks like a function that only checks for a possibility of an
operation while it's doing more than that. However I don't have a
really good suggestion to improve the naming, so treat it as a nitpick
and feel free to ignore.

>
> I went a bit back and forth while cleaning that function up, but calling
> it wp_reuse_anon_folio() would end up being confusing with
> wp_page_reuse() called afterwards [we should probably rename that one to
> wp_page_wunprotect() independently]. So I prefer to leave the actual
> (sub)page reuse part in the caller.
>
> > it's actually doing that operation instead of just checking if it's
> > possible. That would also let us keep unconditional
> > SetPageAnonExclusive() in it and do that under folio lock like it used
> > to do (keeping rules simple). Other than that, it looks good to me.
>
> I really want to avoid passing a "struct page" to that function; once
> we're dealing with PTE-mapped THP, the page might actually be a tail
> page of the folio.

Oh, I didn't realize that vmf->page and folio->page might differ in
here. Ok, sounds reasonable.
Thanks,
Suren.

>
> Thanks!
>
> --
> Cheers,
>
> David / dhildenb
>