[RFC PATCH 0/5] folio based filemap_map_pages()

[RFC PATCH 0/5] folio based filemap_map_pages()

[Yin Fengwei]

Current filemap_map_pages() still use page granularity
even when the underneath folio is large folio. Making it
use folio based granularity allows batched refcount,
rmap and mm counter opertion. Which brings some level
performance gain. This series tries to update
   filemap_map_pages()
   do_set_pte()
   page_add_file_rmap()
to bring batched refcount, rmap and mm counter in.

Patch 1 enabled the fault around for shared file page
        write fault. As David suggested here:
https://lore.kernel.org/linux-mm/e14b4e9a-612d-fc02-edc0-8f3b6bcf4148@redhat.com/

Patch 2 update filemap_map_pages() to do map based on
        folio granularity and batched refcount update

Patch 3,4,5 Enable batched rmap and mm counter

Function testing:
  - two days daily usage: boot to xfs GUI, kernel build, editor and browser

Performance testing:
  - kernel build: no obvious performance impact
  - micro benchmark: will-it-scale:page_fault3 (shared file write fault)
    Patch 1 brings 3.75X performance gain

  - micro benchmark: will-it-scale:page_fault3 like but with file read fault
    - Patch 2 brings about 2% performance gain (batched refcount)
    - Patch 3,4,5 brings about 15% performance gain (batched rmap and mm counter)


Yin Fengwei (5):
  mm: Enable fault around for shared file page fault
  filemap: do file page mapping with folio granularity
  rmap: add page_add_file_rmap_range()
  mm: add do_set_pte_entry()
  filemap: batched update mm counter,rmap when map file folio

 include/linux/mm.h   |   2 +
 include/linux/rmap.h |   2 +
 mm/filemap.c         | 106 +++++++++++++++++++++++++++++--------------
 mm/memory.c          |  25 +++++++++-
 mm/rmap.c            |  70 +++++++++++++++++-----------
 5 files changed, 143 insertions(+), 62 deletions(-)

-- 
2.30.2

[RFC PATCH 1/5] mm: Enable fault around for shared file page fault

[Yin Fengwei]

Allow the batched page table population added to fault
around handler and benefit both file read and shared
fault.

Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
---
 mm/memory.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/mm/memory.c b/mm/memory.c
index ec833a2e0601..61ccd2d7e6a6 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4545,6 +4545,17 @@ static vm_fault_t do_shared_fault(struct vm_fault *vmf)
 	struct vm_area_struct *vma = vmf->vma;
 	vm_fault_t ret, tmp;
 
+	/*
+	 * Let's call ->map_pages() first and use ->fault() as fallback
+	 * if page by the offset is not ready to be mapped (cold cache or
+	 * something).
+	 */
+	if (should_fault_around(vmf)) {
+		ret = do_fault_around(vmf);
+		if (ret)
+			return ret;
+	}
+
 	ret = __do_fault(vmf);
 	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY)))
 		return ret;
-- 
2.30.2

[RFC PATCH 2/5] filemap: do file page mapping with folio granularity

[Yin Fengwei]

Add function to do file page mapping based on folio and update
filemap_map_pages() to use new function. So the filemap page
mapping will deal with folio granularity instead of page
granularity. This allow batched folio refcount update.

Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
---
 mm/filemap.c | 82 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 48 insertions(+), 34 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index c915ded191f0..fe0c226c8b1e 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3351,6 +3351,43 @@ static inline struct folio *next_map_page(struct address_space *mapping,
 				  mapping, xas, end_pgoff);
 }
 
+
+static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
+	struct folio *folio, struct page *page, unsigned long addr,
+	int len)
+{
+	vm_fault_t ret = 0;
+	struct vm_area_struct *vma = vmf->vma;
+	struct file *file = vma->vm_file;
+	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
+	int ref_count = 0, count = 0;
+
+	do {
+		if (PageHWPoison(page))
+			continue;
+
+		if (mmap_miss > 0)
+			mmap_miss--;
+
+		if (!pte_none(*vmf->pte))
+			continue;
+
+		if (vmf->address == addr)
+			ret = VM_FAULT_NOPAGE;
+
+		ref_count++;
+
+		do_set_pte(vmf, page, addr);
+		update_mmu_cache(vma, addr, vmf->pte);
+
+	} while (vmf->pte++, page++, addr += PAGE_SIZE, ++count < len);
+
+	folio_ref_add(folio, ref_count);
+	WRITE_ONCE(file->f_ra.mmap_miss, mmap_miss);
+
+	return ret;
+}
+
 vm_fault_t filemap_map_pages(struct vm_fault *vmf,
 			     pgoff_t start_pgoff, pgoff_t end_pgoff)
 {
@@ -3361,9 +3398,9 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
 	unsigned long addr;
 	XA_STATE(xas, &mapping->i_pages, start_pgoff);
 	struct folio *folio;
-	struct page *page;
 	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
 	vm_fault_t ret = 0;
+	int len = 0;
 
 	rcu_read_lock();
 	folio = first_map_page(mapping, &xas, end_pgoff);
@@ -3378,45 +3415,22 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
 	addr = vma->vm_start + ((start_pgoff - vma->vm_pgoff) << PAGE_SHIFT);
 	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, addr, &vmf->ptl);
 	do {
-again:
-		page = folio_file_page(folio, xas.xa_index);
-		if (PageHWPoison(page))
-			goto unlock;
-
-		if (mmap_miss > 0)
-			mmap_miss--;
+		struct page *page;
+		unsigned long end;
 
+		page = folio_file_page(folio, xas.xa_index);
 		addr += (xas.xa_index - last_pgoff) << PAGE_SHIFT;
-		vmf->pte += xas.xa_index - last_pgoff;
+		vmf->pte += xas.xa_index - last_pgoff - len;
 		last_pgoff = xas.xa_index;
+		end = folio->index + folio_nr_pages(folio) - 1;
+		len = min(end, end_pgoff) - xas.xa_index + 1;
 
-		/*
-		 * NOTE: If there're PTE markers, we'll leave them to be
-		 * handled in the specific fault path, and it'll prohibit the
-		 * fault-around logic.
-		 */
-		if (!pte_none(*vmf->pte))
-			goto unlock;
-
-		/* We're about to handle the fault */
-		if (vmf->address == addr)
+		if (VM_FAULT_NOPAGE ==
+			filemap_map_folio_range(vmf, folio, page, addr, len))
 			ret = VM_FAULT_NOPAGE;
 
-		do_set_pte(vmf, page, addr);
-		/* no need to invalidate: a not-present page won't be cached */
-		update_mmu_cache(vma, addr, vmf->pte);
-		if (folio_more_pages(folio, xas.xa_index, end_pgoff)) {
-			xas.xa_index++;
-			folio_ref_inc(folio);
-			goto again;
-		}
-		folio_unlock(folio);
-		continue;
-unlock:
-		if (folio_more_pages(folio, xas.xa_index, end_pgoff)) {
-			xas.xa_index++;
-			goto again;
-		}
+		xas.xa_index = end;
+
 		folio_unlock(folio);
 		folio_put(folio);
 	} while ((folio = next_map_page(mapping, &xas, end_pgoff)) != NULL);
-- 
2.30.2

[RFC PATCH 3/5] rmap: add page_add_file_rmap_range()

[Yin Fengwei]

page_add_file_rmap_range() allows to add specific range of
large folio rmap.

Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
---
 include/linux/rmap.h |  2 ++
 mm/rmap.c            | 70 ++++++++++++++++++++++++++++----------------
 2 files changed, 46 insertions(+), 26 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index a6bd1f0a183d..063e0addcbf8 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -198,6 +198,8 @@ void folio_add_new_anon_rmap(struct folio *, struct vm_area_struct *,
 		unsigned long address);
 void page_add_file_rmap(struct page *, struct vm_area_struct *,
 		bool compound);
+void page_add_file_rmap_range(struct folio *, struct page *, int len,
+	struct vm_area_struct *, bool compound);
 void page_remove_rmap(struct page *, struct vm_area_struct *,
 		bool compound);
 
diff --git a/mm/rmap.c b/mm/rmap.c
index 948ca17a96ad..cc7fe3010330 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1301,40 +1301,19 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
 	__page_set_anon_rmap(folio, &folio->page, vma, address, 1);
 }
 
-/**
- * page_add_file_rmap - add pte mapping to a file page
- * @page:	the page to add the mapping to
- * @vma:	the vm area in which the mapping is added
- * @compound:	charge the page as compound or small page
- *
- * The caller needs to hold the pte lock.
- */
-void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
-		bool compound)
+void page_add_file_rmap_range(struct folio *folio, struct page *page, int len,
+	struct vm_area_struct *vma, bool compound)
 {
-	struct folio *folio = page_folio(page);
 	atomic_t *mapped = &folio->_nr_pages_mapped;
-	int nr = 0, nr_pmdmapped = 0;
+	int nr = 0, nr_pmdmapped = 0, nr_pages = folio_nr_pages(folio);
 	bool first;
 
-	VM_BUG_ON_PAGE(compound && !PageTransHuge(page), page);
-
-	/* Is page being mapped by PTE? Is this its first map to be added? */
-	if (likely(!compound)) {
-		first = atomic_inc_and_test(&page->_mapcount);
-		nr = first;
-		if (first && folio_test_large(folio)) {
-			nr = atomic_inc_return_relaxed(mapped);
-			nr = (nr < COMPOUND_MAPPED);
-		}
-	} else if (folio_test_pmd_mappable(folio)) {
-		/* That test is redundant: it's for safety or to optimize out */
-
+	if (compound) {
 		first = atomic_inc_and_test(&folio->_entire_mapcount);
 		if (first) {
 			nr = atomic_add_return_relaxed(COMPOUND_MAPPED, mapped);
 			if (likely(nr < COMPOUND_MAPPED + COMPOUND_MAPPED)) {
-				nr_pmdmapped = folio_nr_pages(folio);
+				nr_pmdmapped = nr_pages;
 				nr = nr_pmdmapped - (nr & FOLIO_PAGES_MAPPED);
 				/* Raced ahead of a remove and another add? */
 				if (unlikely(nr < 0))
@@ -1344,6 +1323,20 @@ void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
 				nr = 0;
 			}
 		}
+	} else {
+		int i = 0, new_mapped = 0, count;
+
+		count = min_t(int, len, nr_pages - folio_page_idx(folio, page));
+		do {
+			first = atomic_inc_and_test(&page->_mapcount);
+			new_mapped = first;
+			if (first && folio_test_large(folio)) {
+				new_mapped = atomic_inc_return_relaxed(mapped);
+				new_mapped = (new_mapped < COMPOUND_MAPPED);
+			}
+			if  (new_mapped)
+				nr++;
+		} while (page++, ++i < len);
 	}
 
 	if (nr_pmdmapped)
@@ -1355,6 +1348,31 @@ void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
 	mlock_vma_folio(folio, vma, compound);
 }
 
+/**
+ * page_add_file_rmap - add pte mapping to a file page
+ * @page:	the page to add the mapping to
+ * @vma:	the vm area in which the mapping is added
+ * @compound:	charge the page as compound or small page
+ *
+ * The caller needs to hold the pte lock.
+ */
+void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
+		bool compound)
+{
+	struct folio *folio = page_folio(page);
+	int nr_pages;
+
+	VM_BUG_ON_PAGE(compound && !PageTransHuge(page), page);
+
+	if (likely(!compound))
+		nr_pages = 1;
+	else
+		nr_pages = folio_nr_pages(folio);
+
+	page_add_file_rmap_range(folio, page, nr_pages, vma, compound);
+}
+
 /**
  * page_remove_rmap - take down pte mapping from a page
  * @page:	page to remove mapping from
-- 
2.30.2

[RFC PATCH 4/5] mm: add do_set_pte_entry()

[Yin Fengwei]

do_set_pte_entry() does same as do_set_pte() but doesn't update
mm_counter or add file rmap. It allows batched mm_counter and
rmap operations later.

Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
---
 include/linux/mm.h |  2 ++
 mm/memory.c        | 14 ++++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 63c0e645f37c..f31426a40e05 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1061,6 +1061,8 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
 
 vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page);
 void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr);
+void do_set_pte_entry(struct vm_fault *vmf, struct page *page,
+	 unsigned long addr);
 
 vm_fault_t finish_fault(struct vm_fault *vmf);
 vm_fault_t finish_mkwrite_fault(struct vm_fault *vmf);
diff --git a/mm/memory.c b/mm/memory.c
index 61ccd2d7e6a6..d0c27e11fab4 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4256,7 +4256,8 @@ vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page)
 }
 #endif
 
-void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr)
+void do_set_pte_entry(struct vm_fault *vmf, struct page *page,
+	unsigned long addr)
 {
 	struct vm_area_struct *vma = vmf->vma;
 	bool uffd_wp = pte_marker_uffd_wp(vmf->orig_pte);
@@ -4276,6 +4277,16 @@ void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr)
 		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
 	if (unlikely(uffd_wp))
 		entry = pte_mkuffd_wp(entry);
+	set_pte_at(vma->vm_mm, addr, vmf->pte, entry);
+}
+
+void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr)
+{
+	struct vm_area_struct *vma = vmf->vma;
+	bool write = vmf->flags & FAULT_FLAG_WRITE;
+
+	do_set_pte_entry(vmf, page, addr);
+
 	/* copy-on-write page */
 	if (write && !(vma->vm_flags & VM_SHARED)) {
 		inc_mm_counter(vma->vm_mm, MM_ANONPAGES);
@@ -4285,7 +4296,6 @@ void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr)
 		inc_mm_counter(vma->vm_mm, mm_counter_file(page));
 		page_add_file_rmap(page, vma, false);
 	}
-	set_pte_at(vma->vm_mm, addr, vmf->pte, entry);
 }
 
 static bool vmf_pte_changed(struct vm_fault *vmf)
-- 
2.30.2

[RFC PATCH 5/5] filemap: batched update mm counter,rmap when map file folio

[Yin Fengwei]

Instead of update mm counter, rmap one by one, batched update
brings some level performance gain.

Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
---
 mm/filemap.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index fe0c226c8b1e..6d9438490025 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3360,28 +3360,52 @@ static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
 	struct vm_area_struct *vma = vmf->vma;
 	struct file *file = vma->vm_file;
 	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
-	int ref_count = 0, count = 0;
+	int ref_count = 0, count = 0, maplen = 0;
+	struct page *pg = page;
 
 	do {
-		if (PageHWPoison(page))
+		if (PageHWPoison(page)) {
+			if (maplen) {
+				page_add_file_rmap_range(folio, pg, maplen,
+					vma, false);
+				add_mm_counter(vma->vm_mm,
+					mm_counter_file(pg), maplen);
+			}
+			pg = page + 1;
+			maplen = 0;
 			continue;
+		}
 
 		if (mmap_miss > 0)
 			mmap_miss--;
 
-		if (!pte_none(*vmf->pte))
+		if (!pte_none(*vmf->pte)) {
+			if (maplen) {
+				page_add_file_rmap_range(folio, pg, maplen,
+					vma, false);
+				add_mm_counter(vma->vm_mm,
+					mm_counter_file(pg), maplen);
+			}
+			pg = page + 1;
+			maplen = 0;
 			continue;
+		}
 
 		if (vmf->address == addr)
 			ret = VM_FAULT_NOPAGE;
 
 		ref_count++;
+		maplen++;
 
-		do_set_pte(vmf, page, addr);
+		do_set_pte_entry(vmf, page, addr);
 		update_mmu_cache(vma, addr, vmf->pte);
 
 	} while (vmf->pte++, page++, addr += PAGE_SIZE, ++count < len);
 
+	if (maplen) {
+		page_add_file_rmap_range(folio, pg, maplen, vma, false);
+		add_mm_counter(vma->vm_mm, mm_counter_file(pg), maplen);
+	}
 	folio_ref_add(folio, ref_count);
 	WRITE_ONCE(file->f_ra.mmap_miss, mmap_miss);
 
-- 
2.30.2

Re: [RFC PATCH 0/5] folio based filemap_map_pages()

[Yin, Fengwei]

On 1/30/2023 8:54 PM, Yin Fengwei wrote:
> Current filemap_map_pages() still use page granularity
> even when the underneath folio is large folio. Making it
> use folio based granularity allows batched refcount,
> rmap and mm counter opertion. Which brings some level
> performance gain. This series tries to update
>    filemap_map_pages()
>    do_set_pte()
>    page_add_file_rmap()
> to bring batched refcount, rmap and mm counter in.
> 
> Patch 1 enabled the fault around for shared file page
>         write fault. As David suggested here:
> https://lore.kernel.org/linux-mm/e14b4e9a-612d-fc02-edc0-8f3b6bcf4148@redhat.com/
> 
> Patch 2 update filemap_map_pages() to do map based on
>         folio granularity and batched refcount update
> 
> Patch 3,4,5 Enable batched rmap and mm counter
> 
> Function testing:
>   - two days daily usage: boot to xfs GUI, kernel build, editor and browser
> 
> Performance testing:
>   - kernel build: no obvious performance impact
>   - micro benchmark: will-it-scale:page_fault3 (shared file write fault)
>     Patch 1 brings 3.75X performance gain
> 
>   - micro benchmark: will-it-scale:page_fault3 like but with file read fault
>     - Patch 2 brings about 2% performance gain (batched refcount)
>     - Patch 3,4,5 brings about 15% performance gain (batched rmap and mm counter)
> 
Forgot to mention: the base of the series is next-20230119.
> 
> Yin Fengwei (5):
>   mm: Enable fault around for shared file page fault
>   filemap: do file page mapping with folio granularity
>   rmap: add page_add_file_rmap_range()
>   mm: add do_set_pte_entry()
>   filemap: batched update mm counter,rmap when map file folio
> 
>  include/linux/mm.h   |   2 +
>  include/linux/rmap.h |   2 +
>  mm/filemap.c         | 106 +++++++++++++++++++++++++++++--------------
>  mm/memory.c          |  25 +++++++++-
>  mm/rmap.c            |  70 +++++++++++++++++-----------
>  5 files changed, 143 insertions(+), 62 deletions(-)
> 

Re: [RFC PATCH 1/5] mm: Enable fault around for shared file page fault

[Matthew Wilcox]

On Mon, Jan 30, 2023 at 08:55:00PM +0800, Yin Fengwei wrote:
> Allow the batched page table population added to fault
> around handler and benefit both file read and shared
> fault.
> 
> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>

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

Perhaps the performance effect of this patch should go into this patch's
description since it's so dramatic?  I would change-log this as:

The shared fault handler can also benefit from fault-around.  While it
is uncommon to write to MAP_SHARED files, applications that do will see
a huge benefit with will-it-scale:page_fault3 (shared file write fault)
improving by 375%.

Re: [RFC PATCH 2/5] filemap: do file page mapping with folio granularity

[Matthew Wilcox]

On Mon, Jan 30, 2023 at 08:55:01PM +0800, Yin Fengwei wrote:
> Add function to do file page mapping based on folio and update
> filemap_map_pages() to use new function. So the filemap page
> mapping will deal with folio granularity instead of page
> granularity. This allow batched folio refcount update.
> 
> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
> ---
>  mm/filemap.c | 82 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 48 insertions(+), 34 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index c915ded191f0..fe0c226c8b1e 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3351,6 +3351,43 @@ static inline struct folio *next_map_page(struct address_space *mapping,
>  				  mapping, xas, end_pgoff);
>  }
>  
> +

I'd remove this blank line, we typically only have one blank line
between functions.

> +static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
> +	struct folio *folio, struct page *page, unsigned long addr,
> +	int len)

I see this under-indentation in other parts of the mm and it drives me
crazy.  Two tabs to indent the arguments please, otherwise they look
like part of the function.

Also, 'len' is ambiguous.  I'd call this 'nr' or 'nr_pages'.  Also
it should be an unsigned int.

> +{
> +	vm_fault_t ret = 0;
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct file *file = vma->vm_file;
> +	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
> +	int ref_count = 0, count = 0;

Also make these unsigned.

> -		/*
> -		 * NOTE: If there're PTE markers, we'll leave them to be
> -		 * handled in the specific fault path, and it'll prohibit the
> -		 * fault-around logic.
> -		 */

I'd rather not lose this comment; can you move it into
filemap_map_folio_range() please?

> -		if (!pte_none(*vmf->pte))
> -			goto unlock;
> -
> -		/* We're about to handle the fault */
> -		if (vmf->address == addr)
> +		if (VM_FAULT_NOPAGE ==
> +			filemap_map_folio_range(vmf, folio, page, addr, len))
>  			ret = VM_FAULT_NOPAGE;

That indentation is also confusing.  Try this:

		if (filemap_map_folio_range(vmf, folio, page, addr, len) ==
				VM_FAULT_NOPAGE)
			ret = VM_FAULT_NOPAGE;

Except there's an easier way to write it:

		ret |= filemap_map_folio_range(vmf, folio, page, addr, len);


Thanks for doing this!  Looks so much better and performs better!

Re: [RFC PATCH 3/5] rmap: add page_add_file_rmap_range()

[Matthew Wilcox]

On Mon, Jan 30, 2023 at 08:55:02PM +0800, Yin Fengwei wrote:
> +++ b/include/linux/rmap.h
> @@ -198,6 +198,8 @@ void folio_add_new_anon_rmap(struct folio *, struct vm_area_struct *,
>  		unsigned long address);
>  void page_add_file_rmap(struct page *, struct vm_area_struct *,
>  		bool compound);
> +void page_add_file_rmap_range(struct folio *, struct page *, int len,
> +	struct vm_area_struct *, bool compound);

Again, two tabs please.

> -/**
> - * page_add_file_rmap - add pte mapping to a file page
> - * @page:	the page to add the mapping to
> - * @vma:	the vm area in which the mapping is added
> - * @compound:	charge the page as compound or small page
> - *
> - * The caller needs to hold the pte lock.
> - */

We really should have kernel-doc for this function.

> -void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
> -		bool compound)
> +void page_add_file_rmap_range(struct folio *folio, struct page *page, int len,
> +	struct vm_area_struct *vma, bool compound)

Indentation

>  {
> -	struct folio *folio = page_folio(page);
>  	atomic_t *mapped = &folio->_nr_pages_mapped;
> -	int nr = 0, nr_pmdmapped = 0;
> +	int nr = 0, nr_pmdmapped = 0, nr_pages = folio_nr_pages(folio);

Should be unsigned.

Also the 'len' parameter should be called nr_pages, and then do
something like:

	if (nr_pages > folio_nr_pages(folio))
		nr_pages = folio_nr_pages(folio);

>  	bool first;
>  
> -	VM_BUG_ON_PAGE(compound && !PageTransHuge(page), page);
> -
> -	/* Is page being mapped by PTE? Is this its first map to be added? */
> -	if (likely(!compound)) {
> -		first = atomic_inc_and_test(&page->_mapcount);
> -		nr = first;
> -		if (first && folio_test_large(folio)) {
> -			nr = atomic_inc_return_relaxed(mapped);
> -			nr = (nr < COMPOUND_MAPPED);
> -		}
> -	} else if (folio_test_pmd_mappable(folio)) {
> -		/* That test is redundant: it's for safety or to optimize out */
> -
> +	if (compound) {

You've dropped the 'unlikely' marker here.  Also I'm not sure why you
switched the order of these two cases around; makes it harder to review.

>  		first = atomic_inc_and_test(&folio->_entire_mapcount);
>  		if (first) {
>  			nr = atomic_add_return_relaxed(COMPOUND_MAPPED, mapped);
>  			if (likely(nr < COMPOUND_MAPPED + COMPOUND_MAPPED)) {
> -				nr_pmdmapped = folio_nr_pages(folio);
> +				nr_pmdmapped = nr_pages;
>  				nr = nr_pmdmapped - (nr & FOLIO_PAGES_MAPPED);
>  				/* Raced ahead of a remove and another add? */
>  				if (unlikely(nr < 0))
> @@ -1344,6 +1323,20 @@ void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
>  				nr = 0;
>  			}
>  		}
> +	} else {
> +		int i = 0, new_mapped = 0, count;
> +
> +		count = min_t(int, len, nr_pages - folio_page_idx(folio, page));
> +		do {
> +			first = atomic_inc_and_test(&page->_mapcount);
> +			new_mapped = first;
> +			if (first && folio_test_large(folio)) {
> +				new_mapped = atomic_inc_return_relaxed(mapped);
> +				new_mapped = (new_mapped < COMPOUND_MAPPED);
> +			}
> +			if  (new_mapped)

Space between if and '('.

I'm not quite sure why you want to pass 'page' in rather than the
offset of the page within the folio, but perhaps that will become
obvious as I read along.

Re: [RFC PATCH 4/5] mm: add do_set_pte_entry()

[Matthew Wilcox]

On Mon, Jan 30, 2023 at 08:55:03PM +0800, Yin Fengwei wrote:
> +++ b/include/linux/mm.h
> @@ -1061,6 +1061,8 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
>  
>  vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page);
>  void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr);
> +void do_set_pte_entry(struct vm_fault *vmf, struct page *page,
> +	 unsigned long addr);

indentation

> -void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr)
> +void do_set_pte_entry(struct vm_fault *vmf, struct page *page,
> +	unsigned long addr)

ditto

>  {
>  	struct vm_area_struct *vma = vmf->vma;
>  	bool uffd_wp = pte_marker_uffd_wp(vmf->orig_pte);
> @@ -4276,6 +4277,16 @@ void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr)
>  		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
>  	if (unlikely(uffd_wp))
>  		entry = pte_mkuffd_wp(entry);
> +	set_pte_at(vma->vm_mm, addr, vmf->pte, entry);

I'm not sure this is safe.  As soon as you call set_pte_at(), the page
can be found by GUP.  If it is, and it doesn't have rmap set up, aren't
things going to go horribly wrong?

Re: [RFC PATCH 5/5] filemap: batched update mm counter,rmap when map file folio

[Matthew Wilcox]

On Mon, Jan 30, 2023 at 08:55:04PM +0800, Yin Fengwei wrote:
> +++ b/mm/filemap.c
> @@ -3360,28 +3360,52 @@ static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
>  	struct vm_area_struct *vma = vmf->vma;
>  	struct file *file = vma->vm_file;
>  	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
> -	int ref_count = 0, count = 0;
> +	int ref_count = 0, count = 0, maplen = 0;
> +	struct page *pg = page;

OK, having read it through, I think you're making your life a lot harder
by keeping page pointers at all.  I'd pass xas.xa_index - folio->index
from filemap_map_pages() as a parameter called something like 'first'.

>  	do {
> -		if (PageHWPoison(page))
> +		if (PageHWPoison(page)) {
> +			if (maplen) {
> +				page_add_file_rmap_range(folio, pg, maplen,
> +					vma, false);
> +				add_mm_counter(vma->vm_mm,
> +					mm_counter_file(pg), maplen);

Again you've made your life harder ;-)  Try something like this:

void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr)
{
	do_set_pte_range(vmf, page, addr, 1);
}

... and then here, you can do:

			if (maplen)
				do_set_pte_range(vmf, page - maplen - 1,
						base_addr, maplen);
			base_addr += (maplen + 1) * PAGE_SIZE
			maplen = 0;
			continue;

Re: [RFC PATCH 1/5] mm: Enable fault around for shared file page fault

[Yin, Fengwei]

On 1/30/2023 9:21 PM, Matthew Wilcox wrote:
> On Mon, Jan 30, 2023 at 08:55:00PM +0800, Yin Fengwei wrote:
>> Allow the batched page table population added to fault
>> around handler and benefit both file read and shared
>> fault.
>>
>> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
> 
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> 
> Perhaps the performance effect of this patch should go into this patch's
> description since it's so dramatic?  I would change-log this as:
> 
> The shared fault handler can also benefit from fault-around.  While it
> is uncommon to write to MAP_SHARED files, applications that do will see
> a huge benefit with will-it-scale:page_fault3 (shared file write fault)
> improving by 375%.
Thanks a lot for reviewing. I will update the commit message.


Regards
Yin, Fengwei

Re: [RFC PATCH 2/5] filemap: do file page mapping with folio granularity

[Yin, Fengwei]

On 1/30/2023 9:35 PM, Matthew Wilcox wrote:
> On Mon, Jan 30, 2023 at 08:55:01PM +0800, Yin Fengwei wrote:
>> Add function to do file page mapping based on folio and update
>> filemap_map_pages() to use new function. So the filemap page
>> mapping will deal with folio granularity instead of page
>> granularity. This allow batched folio refcount update.
>>
>> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
>> ---
>>  mm/filemap.c | 82 ++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 48 insertions(+), 34 deletions(-)
>>
>> diff --git a/mm/filemap.c b/mm/filemap.c
>> index c915ded191f0..fe0c226c8b1e 100644
>> --- a/mm/filemap.c
>> +++ b/mm/filemap.c
>> @@ -3351,6 +3351,43 @@ static inline struct folio *next_map_page(struct address_space *mapping,
>>  				  mapping, xas, end_pgoff);
>>  }
>>  
>> +
> 
> I'd remove this blank line, we typically only have one blank line
> between functions.
OK.

> 
>> +static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
>> +	struct folio *folio, struct page *page, unsigned long addr,
>> +	int len)
> 
> I see this under-indentation in other parts of the mm and it drives me
> crazy.  Two tabs to indent the arguments please, otherwise they look
> like part of the function.
OK. I will correct all the indent problems in this series in next version.
> 
> Also, 'len' is ambiguous.  I'd call this 'nr' or 'nr_pages'.  Also
> it should be an unsigned int.
> 
>> +{
>> +	vm_fault_t ret = 0;
>> +	struct vm_area_struct *vma = vmf->vma;
>> +	struct file *file = vma->vm_file;
>> +	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
>> +	int ref_count = 0, count = 0;
> 
> Also make these unsigned.
> 
>> -		/*
>> -		 * NOTE: If there're PTE markers, we'll leave them to be
>> -		 * handled in the specific fault path, and it'll prohibit the
>> -		 * fault-around logic.
>> -		 */
> 
> I'd rather not lose this comment; can you move it into
> filemap_map_folio_range() please?
I will keep all the comments in the right place in next version.

Regards
Yin, Fengwei

> 
>> -		if (!pte_none(*vmf->pte))
>> -			goto unlock;
>> -
>> -		/* We're about to handle the fault */
>> -		if (vmf->address == addr)
>> +		if (VM_FAULT_NOPAGE ==
>> +			filemap_map_folio_range(vmf, folio, page, addr, len))
>>  			ret = VM_FAULT_NOPAGE;
> 
> That indentation is also confusing.  Try this:
> 
> 		if (filemap_map_folio_range(vmf, folio, page, addr, len) ==
> 				VM_FAULT_NOPAGE)
> 			ret = VM_FAULT_NOPAGE;
> 
> Except there's an easier way to write it:
> 
> 		ret |= filemap_map_folio_range(vmf, folio, page, addr, len);
> 
> 
> Thanks for doing this!  Looks so much better and performs better!

Re: [RFC PATCH 4/5] mm: add do_set_pte_entry()

[Yin, Fengwei]

On 1/30/2023 9:53 PM, Matthew Wilcox wrote:
> On Mon, Jan 30, 2023 at 08:55:03PM +0800, Yin Fengwei wrote:
>> +++ b/include/linux/mm.h
>> @@ -1061,6 +1061,8 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
>>  
>>  vm_fault_t do_set_pmd(struct vm_fault *vmf, struct page *page);
>>  void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr);
>> +void do_set_pte_entry(struct vm_fault *vmf, struct page *page,
>> +	 unsigned long addr);
> 
> indentation
> 
>> -void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr)
>> +void do_set_pte_entry(struct vm_fault *vmf, struct page *page,
>> +	unsigned long addr)
> 
> ditto
> 
>>  {
>>  	struct vm_area_struct *vma = vmf->vma;
>>  	bool uffd_wp = pte_marker_uffd_wp(vmf->orig_pte);
>> @@ -4276,6 +4277,16 @@ void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr)
>>  		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
>>  	if (unlikely(uffd_wp))
>>  		entry = pte_mkuffd_wp(entry);
>> +	set_pte_at(vma->vm_mm, addr, vmf->pte, entry);
> 
> I'm not sure this is safe.  As soon as you call set_pte_at(), the page
> can be found by GUP.  If it is, and it doesn't have rmap set up, aren't
> things going to go horribly wrong?
Thanks a lot for pointing this out. I was not aware of the connection of
the sequence here with GUP. Will take care of this in next version by
putting rmap set up before set_pte_at().

Regards
Yin, Fengwei

> 

Re: [RFC PATCH 5/5] filemap: batched update mm counter,rmap when map file folio

[Yin, Fengwei]

On 1/30/2023 10:14 PM, Matthew Wilcox wrote:
> On Mon, Jan 30, 2023 at 08:55:04PM +0800, Yin Fengwei wrote:
>> +++ b/mm/filemap.c
>> @@ -3360,28 +3360,52 @@ static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
>>  	struct vm_area_struct *vma = vmf->vma;
>>  	struct file *file = vma->vm_file;
>>  	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
>> -	int ref_count = 0, count = 0;
>> +	int ref_count = 0, count = 0, maplen = 0;
>> +	struct page *pg = page;
> 
> OK, having read it through, I think you're making your life a lot harder
> by keeping page pointers at all.  I'd pass xas.xa_index - folio->index
> from filemap_map_pages() as a parameter called something like 'first'.
I use page pointer here because I saw other changes you made kept the
page pointer as parameter. And tried to align with that.

> 
>>  	do {
>> -		if (PageHWPoison(page))
>> +		if (PageHWPoison(page)) {
>> +			if (maplen) {
>> +				page_add_file_rmap_range(folio, pg, maplen,
>> +					vma, false);
>> +				add_mm_counter(vma->vm_mm,
>> +					mm_counter_file(pg), maplen);
> 
> Again you've made your life harder ;-)  Try something like this:
Yes. This is much better.

> 
> void do_set_pte(struct vm_fault *vmf, struct page *page, unsigned long addr)
> {
> 	do_set_pte_range(vmf, page, addr, 1);
> }
> 
> ... and then here, you can do:
> 
> 			if (maplen)
> 				do_set_pte_range(vmf, page - maplen - 1,
> 						base_addr, maplen);
> 			base_addr += (maplen + 1) * PAGE_SIZE
> 			maplen = 0;
> 			continue;
Thanks a lot for all your comments. I will address all your comments in
next version.

Regards
Yin, Fengwei

> 

Re: [RFC PATCH 2/5] filemap: do file page mapping with folio granularity

[Huang, Ying]

Yin Fengwei <fengwei.yin@intel.com> writes:

> Add function to do file page mapping based on folio and update
> filemap_map_pages() to use new function. So the filemap page
> mapping will deal with folio granularity instead of page
> granularity. This allow batched folio refcount update.
>
> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
> ---
>  mm/filemap.c | 82 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 48 insertions(+), 34 deletions(-)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index c915ded191f0..fe0c226c8b1e 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3351,6 +3351,43 @@ static inline struct folio *next_map_page(struct address_space *mapping,
>  				  mapping, xas, end_pgoff);
>  }
>  
> +
> +static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
> +	struct folio *folio, struct page *page, unsigned long addr,
> +	int len)

As Matthew pointed out, we should rename 'len'. And some comments about
the meaning of the parameters should be good.  For example,

/* Map sub-pages [start_page, start_page + nr_pages) of folio */
static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
	struct folio *folio, struct page *start_page, unsigned int nr_pages,
        unsigned long start)

Best Regards,
Huang, Ying

> +{
> +	vm_fault_t ret = 0;
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct file *file = vma->vm_file;
> +	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
> +	int ref_count = 0, count = 0;
> +
> +	do {
> +		if (PageHWPoison(page))
> +			continue;
> +
> +		if (mmap_miss > 0)
> +			mmap_miss--;
> +
> +		if (!pte_none(*vmf->pte))
> +			continue;
> +
> +		if (vmf->address == addr)
> +			ret = VM_FAULT_NOPAGE;
> +
> +		ref_count++;
> +
> +		do_set_pte(vmf, page, addr);
> +		update_mmu_cache(vma, addr, vmf->pte);
> +
> +	} while (vmf->pte++, page++, addr += PAGE_SIZE, ++count < len);
> +
> +	folio_ref_add(folio, ref_count);
> +	WRITE_ONCE(file->f_ra.mmap_miss, mmap_miss);
> +
> +	return ret;
> +}
> +
>  vm_fault_t filemap_map_pages(struct vm_fault *vmf,
>  			     pgoff_t start_pgoff, pgoff_t end_pgoff)
>  {
> @@ -3361,9 +3398,9 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
>  	unsigned long addr;
>  	XA_STATE(xas, &mapping->i_pages, start_pgoff);
>  	struct folio *folio;
> -	struct page *page;
>  	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
>  	vm_fault_t ret = 0;
> +	int len = 0;
>  
>  	rcu_read_lock();
>  	folio = first_map_page(mapping, &xas, end_pgoff);
> @@ -3378,45 +3415,22 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
>  	addr = vma->vm_start + ((start_pgoff - vma->vm_pgoff) << PAGE_SHIFT);
>  	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, addr, &vmf->ptl);
>  	do {
> -again:
> -		page = folio_file_page(folio, xas.xa_index);
> -		if (PageHWPoison(page))
> -			goto unlock;
> -
> -		if (mmap_miss > 0)
> -			mmap_miss--;
> +		struct page *page;
> +		unsigned long end;
>  
> +		page = folio_file_page(folio, xas.xa_index);
>  		addr += (xas.xa_index - last_pgoff) << PAGE_SHIFT;
> -		vmf->pte += xas.xa_index - last_pgoff;
> +		vmf->pte += xas.xa_index - last_pgoff - len;
>  		last_pgoff = xas.xa_index;
> +		end = folio->index + folio_nr_pages(folio) - 1;
> +		len = min(end, end_pgoff) - xas.xa_index + 1;
>  
> -		/*
> -		 * NOTE: If there're PTE markers, we'll leave them to be
> -		 * handled in the specific fault path, and it'll prohibit the
> -		 * fault-around logic.
> -		 */
> -		if (!pte_none(*vmf->pte))
> -			goto unlock;
> -
> -		/* We're about to handle the fault */
> -		if (vmf->address == addr)
> +		if (VM_FAULT_NOPAGE ==
> +			filemap_map_folio_range(vmf, folio, page, addr, len))
>  			ret = VM_FAULT_NOPAGE;
>  
> -		do_set_pte(vmf, page, addr);
> -		/* no need to invalidate: a not-present page won't be cached */
> -		update_mmu_cache(vma, addr, vmf->pte);
> -		if (folio_more_pages(folio, xas.xa_index, end_pgoff)) {
> -			xas.xa_index++;
> -			folio_ref_inc(folio);
> -			goto again;
> -		}
> -		folio_unlock(folio);
> -		continue;
> -unlock:
> -		if (folio_more_pages(folio, xas.xa_index, end_pgoff)) {
> -			xas.xa_index++;
> -			goto again;
> -		}
> +		xas.xa_index = end;
> +
>  		folio_unlock(folio);
>  		folio_put(folio);
>  	} while ((folio = next_map_page(mapping, &xas, end_pgoff)) != NULL);

Re: [RFC PATCH 2/5] filemap: do file page mapping with folio granularity

[Yin, Fengwei]

On 1/31/2023 11:34 AM, Huang, Ying wrote:
> Yin Fengwei <fengwei.yin@intel.com> writes:
> 
>> Add function to do file page mapping based on folio and update
>> filemap_map_pages() to use new function. So the filemap page
>> mapping will deal with folio granularity instead of page
>> granularity. This allow batched folio refcount update.
>>
>> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
>> ---
>>  mm/filemap.c | 82 ++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 48 insertions(+), 34 deletions(-)
>>
>> diff --git a/mm/filemap.c b/mm/filemap.c
>> index c915ded191f0..fe0c226c8b1e 100644
>> --- a/mm/filemap.c
>> +++ b/mm/filemap.c
>> @@ -3351,6 +3351,43 @@ static inline struct folio *next_map_page(struct address_space *mapping,
>>  				  mapping, xas, end_pgoff);
>>  }
>>  
>> +
>> +static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
>> +	struct folio *folio, struct page *page, unsigned long addr,
>> +	int len)
> 
> As Matthew pointed out, we should rename 'len'. And some comments about
> the meaning of the parameters should be good.  For example,
> 
> /* Map sub-pages [start_page, start_page + nr_pages) of folio */
> static vm_fault_t filemap_map_folio_range(struct vm_fault *vmf,
> 	struct folio *folio, struct page *start_page, unsigned int nr_pages,
>         unsigned long start)
Yes. I will address this in next version series. Thanks.


Regards
Yin, Fengwei

> 
> Best Regards,
> Huang, Ying
> 
>> +{
>> +	vm_fault_t ret = 0;
>> +	struct vm_area_struct *vma = vmf->vma;
>> +	struct file *file = vma->vm_file;
>> +	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
>> +	int ref_count = 0, count = 0;
>> +
>> +	do {
>> +		if (PageHWPoison(page))
>> +			continue;
>> +
>> +		if (mmap_miss > 0)
>> +			mmap_miss--;
>> +
>> +		if (!pte_none(*vmf->pte))
>> +			continue;
>> +
>> +		if (vmf->address == addr)
>> +			ret = VM_FAULT_NOPAGE;
>> +
>> +		ref_count++;
>> +
>> +		do_set_pte(vmf, page, addr);
>> +		update_mmu_cache(vma, addr, vmf->pte);
>> +
>> +	} while (vmf->pte++, page++, addr += PAGE_SIZE, ++count < len);
>> +
>> +	folio_ref_add(folio, ref_count);
>> +	WRITE_ONCE(file->f_ra.mmap_miss, mmap_miss);
>> +
>> +	return ret;
>> +}
>> +
>>  vm_fault_t filemap_map_pages(struct vm_fault *vmf,
>>  			     pgoff_t start_pgoff, pgoff_t end_pgoff)
>>  {
>> @@ -3361,9 +3398,9 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
>>  	unsigned long addr;
>>  	XA_STATE(xas, &mapping->i_pages, start_pgoff);
>>  	struct folio *folio;
>> -	struct page *page;
>>  	unsigned int mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
>>  	vm_fault_t ret = 0;
>> +	int len = 0;
>>  
>>  	rcu_read_lock();
>>  	folio = first_map_page(mapping, &xas, end_pgoff);
>> @@ -3378,45 +3415,22 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
>>  	addr = vma->vm_start + ((start_pgoff - vma->vm_pgoff) << PAGE_SHIFT);
>>  	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, addr, &vmf->ptl);
>>  	do {
>> -again:
>> -		page = folio_file_page(folio, xas.xa_index);
>> -		if (PageHWPoison(page))
>> -			goto unlock;
>> -
>> -		if (mmap_miss > 0)
>> -			mmap_miss--;
>> +		struct page *page;
>> +		unsigned long end;
>>  
>> +		page = folio_file_page(folio, xas.xa_index);
>>  		addr += (xas.xa_index - last_pgoff) << PAGE_SHIFT;
>> -		vmf->pte += xas.xa_index - last_pgoff;
>> +		vmf->pte += xas.xa_index - last_pgoff - len;
>>  		last_pgoff = xas.xa_index;
>> +		end = folio->index + folio_nr_pages(folio) - 1;
>> +		len = min(end, end_pgoff) - xas.xa_index + 1;
>>  
>> -		/*
>> -		 * NOTE: If there're PTE markers, we'll leave them to be
>> -		 * handled in the specific fault path, and it'll prohibit the
>> -		 * fault-around logic.
>> -		 */
>> -		if (!pte_none(*vmf->pte))
>> -			goto unlock;
>> -
>> -		/* We're about to handle the fault */
>> -		if (vmf->address == addr)
>> +		if (VM_FAULT_NOPAGE ==
>> +			filemap_map_folio_range(vmf, folio, page, addr, len))
>>  			ret = VM_FAULT_NOPAGE;
>>  
>> -		do_set_pte(vmf, page, addr);
>> -		/* no need to invalidate: a not-present page won't be cached */
>> -		update_mmu_cache(vma, addr, vmf->pte);
>> -		if (folio_more_pages(folio, xas.xa_index, end_pgoff)) {
>> -			xas.xa_index++;
>> -			folio_ref_inc(folio);
>> -			goto again;
>> -		}
>> -		folio_unlock(folio);
>> -		continue;
>> -unlock:
>> -		if (folio_more_pages(folio, xas.xa_index, end_pgoff)) {
>> -			xas.xa_index++;
>> -			goto again;
>> -		}
>> +		xas.xa_index = end;
>> +
>>  		folio_unlock(folio);
>>  		folio_put(folio);
>>  	} while ((folio = next_map_page(mapping, &xas, end_pgoff)) != NULL);

Re: [RFC PATCH 3/5] rmap: add page_add_file_rmap_range()

[Huang, Ying]

Yin Fengwei <fengwei.yin@intel.com> writes:

> page_add_file_rmap_range() allows to add specific range of
> large folio rmap.
>
> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
> ---
>  include/linux/rmap.h |  2 ++
>  mm/rmap.c            | 70 ++++++++++++++++++++++++++++----------------
>  2 files changed, 46 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index a6bd1f0a183d..063e0addcbf8 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -198,6 +198,8 @@ void folio_add_new_anon_rmap(struct folio *, struct vm_area_struct *,
>  		unsigned long address);
>  void page_add_file_rmap(struct page *, struct vm_area_struct *,
>  		bool compound);
> +void page_add_file_rmap_range(struct folio *, struct page *, int len,
> +	struct vm_area_struct *, bool compound);
>  void page_remove_rmap(struct page *, struct vm_area_struct *,
>  		bool compound);
>  
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 948ca17a96ad..cc7fe3010330 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1301,40 +1301,19 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
>  	__page_set_anon_rmap(folio, &folio->page, vma, address, 1);
>  }
>  
> -/**
> - * page_add_file_rmap - add pte mapping to a file page
> - * @page:	the page to add the mapping to
> - * @vma:	the vm area in which the mapping is added
> - * @compound:	charge the page as compound or small page
> - *
> - * The caller needs to hold the pte lock.
> - */
> -void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
> -		bool compound)
> +void page_add_file_rmap_range(struct folio *folio, struct page *page, int len,
> +	struct vm_area_struct *vma, bool compound)
>  {
> -	struct folio *folio = page_folio(page);
>  	atomic_t *mapped = &folio->_nr_pages_mapped;
> -	int nr = 0, nr_pmdmapped = 0;
> +	int nr = 0, nr_pmdmapped = 0, nr_pages = folio_nr_pages(folio);
>  	bool first;
>  
> -	VM_BUG_ON_PAGE(compound && !PageTransHuge(page), page);
> -
> -	/* Is page being mapped by PTE? Is this its first map to be added? */
> -	if (likely(!compound)) {
> -		first = atomic_inc_and_test(&page->_mapcount);
> -		nr = first;
> -		if (first && folio_test_large(folio)) {
> -			nr = atomic_inc_return_relaxed(mapped);
> -			nr = (nr < COMPOUND_MAPPED);
> -		}
> -	} else if (folio_test_pmd_mappable(folio)) {
> -		/* That test is redundant: it's for safety or to optimize out */
> -
> +	if (compound) {
>  		first = atomic_inc_and_test(&folio->_entire_mapcount);
>  		if (first) {
>  			nr = atomic_add_return_relaxed(COMPOUND_MAPPED, mapped);
>  			if (likely(nr < COMPOUND_MAPPED + COMPOUND_MAPPED)) {
> -				nr_pmdmapped = folio_nr_pages(folio);
> +				nr_pmdmapped = nr_pages;
>  				nr = nr_pmdmapped - (nr & FOLIO_PAGES_MAPPED);
>  				/* Raced ahead of a remove and another add? */
>  				if (unlikely(nr < 0))
> @@ -1344,6 +1323,20 @@ void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
>  				nr = 0;
>  			}
>  		}
> +	} else {
> +		int i = 0, new_mapped = 0, count;
> +
> +		count = min_t(int, len, nr_pages - folio_page_idx(folio, page));

It appears that count isn't used after assignment.

Best Regards,
Huang, Ying

> +		do {
> +			first = atomic_inc_and_test(&page->_mapcount);
> +			new_mapped = first;
> +			if (first && folio_test_large(folio)) {
> +				new_mapped = atomic_inc_return_relaxed(mapped);
> +				new_mapped = (new_mapped < COMPOUND_MAPPED);
> +			}
> +			if  (new_mapped)
> +				nr++;
> +		} while (page++, ++i < len);
>  	}
>  
>  	if (nr_pmdmapped)
> @@ -1355,6 +1348,31 @@ void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
>  	mlock_vma_folio(folio, vma, compound);
>  }
>  
> +/**
> + * page_add_file_rmap - add pte mapping to a file page
> + * @page:	the page to add the mapping to
> + * @vma:	the vm area in which the mapping is added
> + * @compound:	charge the page as compound or small page
> + *
> + * The caller needs to hold the pte lock.
> + */
> +void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
> +		bool compound)
> +{
> +	struct folio *folio = page_folio(page);
> +	int nr_pages;
> +
> +	VM_BUG_ON_PAGE(compound && !PageTransHuge(page), page);
> +
> +	if (likely(!compound))
> +		nr_pages = 1;
> +	else
> +		nr_pages = folio_nr_pages(folio);
> +
> +	page_add_file_rmap_range(folio, page, nr_pages, vma, compound);
> +}
> +
>  /**
>   * page_remove_rmap - take down pte mapping from a page
>   * @page:	page to remove mapping from

Re: [RFC PATCH 3/5] rmap: add page_add_file_rmap_range()

[Yin, Fengwei]

On 1/31/2023 3:24 PM, Huang, Ying wrote:
> Yin Fengwei <fengwei.yin@intel.com> writes:
> 
>> page_add_file_rmap_range() allows to add specific range of
>> large folio rmap.
>>
>> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
>> ---
>>  include/linux/rmap.h |  2 ++
>>  mm/rmap.c            | 70 ++++++++++++++++++++++++++++----------------
>>  2 files changed, 46 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
>> index a6bd1f0a183d..063e0addcbf8 100644
>> --- a/include/linux/rmap.h
>> +++ b/include/linux/rmap.h
>> @@ -198,6 +198,8 @@ void folio_add_new_anon_rmap(struct folio *, struct vm_area_struct *,
>>  		unsigned long address);
>>  void page_add_file_rmap(struct page *, struct vm_area_struct *,
>>  		bool compound);
>> +void page_add_file_rmap_range(struct folio *, struct page *, int len,
>> +	struct vm_area_struct *, bool compound);
>>  void page_remove_rmap(struct page *, struct vm_area_struct *,
>>  		bool compound);
>>  
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index 948ca17a96ad..cc7fe3010330 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -1301,40 +1301,19 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
>>  	__page_set_anon_rmap(folio, &folio->page, vma, address, 1);
>>  }
>>  
>> -/**
>> - * page_add_file_rmap - add pte mapping to a file page
>> - * @page:	the page to add the mapping to
>> - * @vma:	the vm area in which the mapping is added
>> - * @compound:	charge the page as compound or small page
>> - *
>> - * The caller needs to hold the pte lock.
>> - */
>> -void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
>> -		bool compound)
>> +void page_add_file_rmap_range(struct folio *folio, struct page *page, int len,
>> +	struct vm_area_struct *vma, bool compound)
>>  {
>> -	struct folio *folio = page_folio(page);
>>  	atomic_t *mapped = &folio->_nr_pages_mapped;
>> -	int nr = 0, nr_pmdmapped = 0;
>> +	int nr = 0, nr_pmdmapped = 0, nr_pages = folio_nr_pages(folio);
>>  	bool first;
>>  
>> -	VM_BUG_ON_PAGE(compound && !PageTransHuge(page), page);
>> -
>> -	/* Is page being mapped by PTE? Is this its first map to be added? */
>> -	if (likely(!compound)) {
>> -		first = atomic_inc_and_test(&page->_mapcount);
>> -		nr = first;
>> -		if (first && folio_test_large(folio)) {
>> -			nr = atomic_inc_return_relaxed(mapped);
>> -			nr = (nr < COMPOUND_MAPPED);
>> -		}
>> -	} else if (folio_test_pmd_mappable(folio)) {
>> -		/* That test is redundant: it's for safety or to optimize out */
>> -
>> +	if (compound) {
>>  		first = atomic_inc_and_test(&folio->_entire_mapcount);
>>  		if (first) {
>>  			nr = atomic_add_return_relaxed(COMPOUND_MAPPED, mapped);
>>  			if (likely(nr < COMPOUND_MAPPED + COMPOUND_MAPPED)) {
>> -				nr_pmdmapped = folio_nr_pages(folio);
>> +				nr_pmdmapped = nr_pages;
>>  				nr = nr_pmdmapped - (nr & FOLIO_PAGES_MAPPED);
>>  				/* Raced ahead of a remove and another add? */
>>  				if (unlikely(nr < 0))
>> @@ -1344,6 +1323,20 @@ void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
>>  				nr = 0;
>>  			}
>>  		}
>> +	} else {
>> +		int i = 0, new_mapped = 0, count;
>> +
>> +		count = min_t(int, len, nr_pages - folio_page_idx(folio, page));
> 
> It appears that count isn't used after assignment.
Oops. This should be used in while (page++, ++i < count);
Maybe this line can be removed if we are sure the range is not out of
folio size? Thanks.

Regards
Yin, Fengwei

> 
> Best Regards,
> Huang, Ying
> 
>> +		do {
>> +			first = atomic_inc_and_test(&page->_mapcount);
>> +			new_mapped = first;
>> +			if (first && folio_test_large(folio)) {
>> +				new_mapped = atomic_inc_return_relaxed(mapped);
>> +				new_mapped = (new_mapped < COMPOUND_MAPPED);
>> +			}
>> +			if  (new_mapped)
>> +				nr++;
>> +		} while (page++, ++i < len);
>>  	}
>>  
>>  	if (nr_pmdmapped)
>> @@ -1355,6 +1348,31 @@ void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
>>  	mlock_vma_folio(folio, vma, compound);
>>  }
>>  
>> +/**
>> + * page_add_file_rmap - add pte mapping to a file page
>> + * @page:	the page to add the mapping to
>> + * @vma:	the vm area in which the mapping is added
>> + * @compound:	charge the page as compound or small page
>> + *
>> + * The caller needs to hold the pte lock.
>> + */
>> +void page_add_file_rmap(struct page *page, struct vm_area_struct *vma,
>> +		bool compound)
>> +{
>> +	struct folio *folio = page_folio(page);
>> +	int nr_pages;
>> +
>> +	VM_BUG_ON_PAGE(compound && !PageTransHuge(page), page);
>> +
>> +	if (likely(!compound))
>> +		nr_pages = 1;
>> +	else
>> +		nr_pages = folio_nr_pages(folio);
>> +
>> +	page_add_file_rmap_range(folio, page, nr_pages, vma, compound);
>> +}
>> +
>>  /**
>>   * page_remove_rmap - take down pte mapping from a page
>>   * @page:	page to remove mapping from