mm/rmap: Optimize anonymous large folio unmapping

Currently, nr_pages is defined as unsigned long. We use nr_pages to manipulate mm rss counters for lazyfree folios as follows:

	add_mm_counter(mm, MM_ANONPAGES, -nr_pages);

Suppose nr_pages = 3. -nr_pages underflows and becomes ULONG_MAX - 2. Then, since add_mm_counter() uses this -nr_pages as a long, ULONG_MAX - 2 does not fit into the positive range of long, and is converted to -3. Eventually all of this works out, but for keeping things simple, declare nr_pages as a signed variable.

Signed-off-by Dev Jain <[email protected]>

---
 mm/rmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

mm/rmap.c +0 -0

index 6398d7eef393f..087c9f5b884fe 100644

mm/rmap.c +0 -1

 	int ptes = 0;
 
 	/*
--

2.34.1

Quoted 21 lines · Dev Jain

Currently, nr_pages is defined as unsigned long. We use nr_pages to manipulate mm rss counters for lazyfree folios as follows:

	add_mm_counter(mm, MM_ANONPAGES, -nr_pages);

Suppose nr_pages = 3. -nr_pages underflows and becomes ULONG_MAX - 2. Then, since add_mm_counter() uses this -nr_pages as a long, ULONG_MAX - 2 does not fit into the positive range of long, and is converted to -3. Eventually all of this works out, but for keeping things simple, declare nr_pages as a signed variable.

Signed-off-by Dev Jain <[email protected]>

---
 mm/rmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

mm/rmap.c +0 -0

index 6398d7eef393f..087c9f5b884fe 100644

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 1985

Quoted 8 lines

mm/rmap.c +2 -1

@@ -1979,9 +1979,10 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 	struct page *subpage;
 	struct mmu_notifier_range range;
 	enum ttu_flags flags = (enum ttu_flags)(long)arg;
-	unsigned long nr_pages = 1, end_addr;
+	unsigned long end_addr;
 	unsigned long pfn;
 	unsigned long hsz = 0;
+	long nr_pages = 1;

This is a non-issue that makes the code confusing, so let's not?

The convention throughout the kernel is nr_pages generally is unsigned because you can't have negative nr_pages.

Quoted 33 lines · Lorenzo Stoakes (Oracle)

Quoted 21 lines · Dev Jain

Currently, nr_pages is defined as unsigned long. We use nr_pages to manipulate mm rss counters for lazyfree folios as follows:

	add_mm_counter(mm, MM_ANONPAGES, -nr_pages);

Suppose nr_pages = 3. -nr_pages underflows and becomes ULONG_MAX - 2. Then, since add_mm_counter() uses this -nr_pages as a long, ULONG_MAX - 2 does not fit into the positive range of long, and is converted to -3. Eventually all of this works out, but for keeping things simple, declare nr_pages as a signed variable.

Signed-off-by Dev Jain <[email protected]>

---
 mm/rmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

mm/rmap.c +0 -0

index 6398d7eef393f..087c9f5b884fe 100644

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 1985

Quoted 8 lines

mm/rmap.c +2 -1

@@ -1979,9 +1979,10 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 	struct page *subpage;
 	struct mmu_notifier_range range;
 	enum ttu_flags flags = (enum ttu_flags)(long)arg;
-	unsigned long nr_pages = 1, end_addr;
+	unsigned long end_addr;
 	unsigned long pfn;
 	unsigned long hsz = 0;
+	long nr_pages = 1;

This is a non-issue that makes the code confusing, so let's not?

Quoted 2 lines

The convention throughout the kernel is nr_pages generally is unsigned because you can't have negative nr_pages.

Indeed. Documented in:

commit fa17bcd5f65ed702df001579cca8c885fa6bf3e7

Author: Aristeu Rozanski <[email protected]>
Date:   Tue Aug 26 11:37:21 2025 -0400

mm: make folio page count functions return unsigned

As raised by Andrew [1], a folio/compound page never spans a negative number of pages. Consequently, let's use "unsigned long" instead of "long" consistently for folio_nr_pages(), folio_large_nr_pages() and compound_nr().

Using "unsigned long" as return value is fine, because even "(long)-folio_nr_pages()" will keep on working as expected. Using "unsigned int" instead would actually break these use cases.

This patch takes the first step changing these to return unsigned long (and making drm_gem_get_pages() use the new types instead of replacing min()).

In the future, we might want to make more callers of these functions to consistently use "unsigned long".

So when I was playing around with the code, I noticed that passing unsigned int nr_pages to add_mm_counter(-nr_pages) messes up things. Then

Quoted 2 lines · Dev Jain

So when I was playing around with the code, I noticed that passing unsigned int nr_pages to add_mm_counter(-nr_pages) messes up things. Then

Using int (whether signed or unsigned) to store nr_pages is a bad idea. Look how many people are asking about supporting PUD-sized folios. On an ARM 64k PAGE_SIZE machine, that's 2^26 pages which is uncomfortably close to 2^32. It'll only take one more level to exceed that, so, what, five to ten more years?

Just use unsigned long everywhere now and save ourselves the grief later.

Initialize nr_pages to 1 at the start of the loop, similar to what is being done in folio_referenced_one(). It may happen that the nr_pages computed from a previous call to folio_unmap_pte_batch gets reused without again going through folio_unmap_pte_batch, messing up things. Although, I don't

think there is any bug right now; a bug would have been there, if in the same instance of a call to try_to_unmap_one, we end up in the pte_present(pteval) branch, then in the else branch doing pte_clear() for device-exclusive ptes. This means that a lazyfree folio has some present entries and some device entries mapping it. Since a pte being device-exclusive means that a GUP reference on the underlying folio is held, the lazyfree unmapping path upon witnessing this will abort try_to_unmap_one.

Signed-off-by Dev Jain <[email protected]>

---
 mm/rmap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

mm/rmap.c +1 -1

index 087c9f5b884fe..1fa020edd954a 100644
@@ -1982,7 +1982,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 	unsigned long end_addr;
 	unsigned long pfn;
 	unsigned long hsz = 0;
-	long nr_pages = 1;
+	long nr_pages;
 	int ptes = 0;
 
 	/*

mm/rmap.c +0 -1

 		/*
 		 * If the folio is in an mlock()d vma, we must not swap it out.
 		 */
--

2.34.1

Quoted 4 lines · Dev Jain

Initialize nr_pages to 1 at the start of the loop, similar to what is being done in folio_referenced_one(). It may happen that the nr_pages computed from a previous call to folio_unmap_pte_batch gets reused without again going through folio_unmap_pte_batch, messing up things. Although, I don't

Quoted 8 lines

think there is any bug right now; a bug would have been there, if in the same instance of a call to try_to_unmap_one, we end up in the pte_present(pteval) branch, then in the else branch doing pte_clear() for device-exclusive ptes. This means that a lazyfree folio has some present entries and some device entries mapping it. Since a pte being device-exclusive means that a GUP reference on the underlying folio is held, the lazyfree unmapping path upon witnessing this will abort try_to_unmap_one.

Dude, paragraphs. PARAGRAPHS :) this is one dense set of words.

It's also a very compressed 'stream of consciousness' hard-to-read block here.

I'm not sure it's really worth having this as a separate commit either, it's pretty trivial.

Quoted 21 lines

Signed-off-by Dev Jain <[email protected]>

---
 mm/rmap.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

mm/rmap.c +1 -1

index 087c9f5b884fe..1fa020edd954a 100644
@@ -1982,7 +1982,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 	unsigned long end_addr;
 	unsigned long pfn;
 	unsigned long hsz = 0;
-	long nr_pages = 1;
+	long nr_pages;
 	int ptes = 0;
 
 	/*

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 2023

Quoted 5 lines

mm/rmap.c +2 -0

@@ -2019,6 +2019,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 	mmu_notifier_invalidate_range_start(&range);
 
 	while (page_vma_mapped_walk(&pvmw)) {
+		nr_pages = 1;
+

This seems valid but I really hate how we default-set it then in some branch set it to something else.

But I think fixing that would be part of a bigger cleanup...

Quoted 6 lines

mm/rmap.c +0 -1

 		/*
 		 * If the folio is in an mlock()d vma, we must not swap it out.
 		 */
--

2.34.1

Quoted 14 lines · Lorenzo Stoakes (Oracle)

Quoted 4 lines · Dev Jain

Initialize nr_pages to 1 at the start of the loop, similar to what is being done in folio_referenced_one(). It may happen that the nr_pages computed from a previous call to folio_unmap_pte_batch gets reused without again going through folio_unmap_pte_batch, messing up things. Although, I don't

Quoted 8 lines

think there is any bug right now; a bug would have been there, if in the same instance of a call to try_to_unmap_one, we end up in the pte_present(pteval) branch, then in the else branch doing pte_clear() for device-exclusive ptes. This means that a lazyfree folio has some present entries and some device entries mapping it. Since a pte being device-exclusive means that a GUP reference on the underlying folio is held, the lazyfree unmapping path upon witnessing this will abort try_to_unmap_one.

Quoted 3 lines

Dude, paragraphs. PARAGRAPHS :) this is one dense set of words.

It's also a very compressed 'stream of consciousness' hard-to-read block here.

Sure :) I'll try to break this down.

Quoted 1 line

Quoted 2 lines

I'm not sure it's really worth having this as a separate commit either, it's pretty trivial.

Hmm...well, as I explain above, it's not trivial for me :) it is difficult for me to reason here whether nr_pages can be reused without a reset in a future iteration.

Quoted 27 lines · Dev Jain

Quoted 14 lines · Lorenzo Stoakes (Oracle)

Quoted 4 lines · Dev Jain

Initialize nr_pages to 1 at the start of the loop, similar to what is being done in folio_referenced_one(). It may happen that the nr_pages computed from a previous call to folio_unmap_pte_batch gets reused without again going through folio_unmap_pte_batch, messing up things. Although, I don't

Quoted 8 lines

think there is any bug right now; a bug would have been there, if in the same instance of a call to try_to_unmap_one, we end up in the pte_present(pteval) branch, then in the else branch doing pte_clear() for device-exclusive ptes. This means that a lazyfree folio has some present entries and some device entries mapping it. Since a pte being device-exclusive means that a GUP reference on the underlying folio is held, the lazyfree unmapping path upon witnessing this will abort try_to_unmap_one.

Quoted 3 lines

Dude, paragraphs. PARAGRAPHS :) this is one dense set of words.

It's also a very compressed 'stream of consciousness' hard-to-read block here.

Sure :) I'll try to break this down.

Quoted 3 lines

I'm not sure it's really worth having this as a separate commit either, it's pretty trivial.

Quoted 3 lines

Hmm...well, as I explain above, it's not trivial for me :) it is difficult for me to reason here whether nr_pages can be reused without a reset in a future iteration.

OK you can have it be separate, let's just really clean up the commit message then please :)

Thanks, Lorenzo

Clean up the code by refactoring the post-pte-clearing path of lazyfree folio unmapping, into commit_ttu_lazyfree_folio().

No functional change is intended.

Signed-off-by Dev Jain <[email protected]>

mm/rmap.c +47 -18

+	struct mm_struct *mm = vma->vm_mm;
+	int ref_count, map_count;
+
+	/*
+	 * Synchronize with gup_pte_range():
+	 * - clear PTE; barrier; read refcount
+	 * - inc refcount; barrier; read PTE
+	 */
+	smp_mb();
+
+	ref_count = folio_ref_count(folio);
+	map_count = folio_mapcount(folio);
+
+	/*
+	 * Order reads for page refcount and dirty flag
+	 * (see comments in __remove_mapping()).
+	 */
+	smp_rmb();
+
+	if (folio_test_dirty(folio) && !(vma->vm_flags & VM_DROPPABLE)) {
+		/*
+		 * redirtied either using the page table or a previously
+		 * obtained GUP reference.
+		 */
+		set_ptes(mm, address, ptep, pteval, nr_pages);
+		folio_set_swapbacked(folio);
+		return 1;
+	}
+
+	if (ref_count != 1 + map_count) {
+		/*
+		 * Additional reference. Could be a GUP reference or any
+		 * speculative reference. GUP users must mark the folio
+		 * dirty if there was a modification. This folio cannot be
+		 * reclaimed right now either way, so act just like nothing
+		 * happened.
+		 * We'll come back here later and detect if the folio was
+		 * dirtied when the additional reference is gone.
+		 */
+		set_ptes(mm, address, ptep, pteval, nr_pages);
+		return 1;
+	}
+
+	add_mm_counter(mm, MM_ANONPAGES, -nr_pages);
+	return 0;
+}
+
 /*
  * @arg: enum ttu_flags will be passed to this argument
  */

@@ -2227,46 +2278,10 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 
 			/* MADV_FREE page check */
 			if (!folio_test_swapbacked(folio)) {
-				int ref_count, map_count;
-
-				/*
-				 * Synchronize with gup_pte_range():
-				 * - clear PTE; barrier; read refcount
-				 * - inc refcount; barrier; read PTE
-				 */
-				smp_mb();
-
-				ref_count = folio_ref_count(folio);
-				map_count = folio_mapcount(folio);
-
-				/*
-				 * Order reads for page refcount and dirty flag
-				 * (see comments in __remove_mapping()).
-				 */
-				smp_rmb();
-

mm/rmap.c +0 -15

 					goto walk_abort;
-				} else if (ref_count != 1 + map_count) {
-					/*
-					 * Additional reference. Could be a GUP reference or any
-					 * speculative reference. GUP users must mark the folio
-					 * dirty if there was a modification. This folio cannot be
-					 * reclaimed right now either way, so act just like nothing
-					 * happened.
-					 * We'll come back here later and detect if the folio was
-					 * dirtied when the additional reference is gone.
-					 */
-					set_ptes(mm, address, pvmw.pte, pteval, nr_pages);
-					goto walk_abort;
-				}
-				add_mm_counter(mm, MM_ANONPAGES, -nr_pages);
 				goto discard;
 			}
 
--

2.34.1

Quoted 6 lines · Dev Jain

Clean up the code by refactoring the post-pte-clearing path of lazyfree folio unmapping, into commit_ttu_lazyfree_folio().

No functional change is intended.

Signed-off-by Dev Jain <[email protected]>

This is a good idea, and we need more refactoring like this in the rmap code, but comments/nits below.

Quoted 12 lines

---
 mm/rmap.c | 93 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 39 deletions(-)

mm/rmap.c +0 -0

index 1fa020edd954a..a61978141ee3f 100644
@@ -1966,6 +1966,57 @@ static inline unsigned int folio_unmap_pte_batch(struct folio *folio,
 				     FPB_RESPECT_WRITE | FPB_RESPECT_SOFT_DIRTY);
 }
 

mm/rmap.c static inline unsigned int folio_unmap_pte_batch(struct folio *folio, · line 1969

Quoted 1 line

mm/rmap.c +1 -0

+static inline int commit_ttu_lazyfree_folio(struct vm_area_struct *vma,

Strange name, maybe lazyfree_range()? Not sure what ttu has to do with

anything...

mm/rmap.c static inline unsigned int folio_unmap_pte_batch(struct folio *folio, · line 1971

Quoted 2 lines

mm/rmap.c +2 -0

+		struct folio *folio, unsigned long address, pte_t *ptep,
+		pte_t pteval, long nr_pages)

That long nr_pages is really grating now...

mm/rmap.c static inline unsigned int folio_unmap_pte_batch(struct folio *folio, · line 1972

Quoted 2 lines

mm/rmap.c +1 -0

+{

Come on Dev, it's 2026, why on earth are you returning an integer and not a bool?

Also it would make sense for this to return false if something breaks, otherwise true.

Quoted 73 lines

mm/rmap.c +47 -18

+	struct mm_struct *mm = vma->vm_mm;
+	int ref_count, map_count;
+
+	/*
+	 * Synchronize with gup_pte_range():
+	 * - clear PTE; barrier; read refcount
+	 * - inc refcount; barrier; read PTE
+	 */
+	smp_mb();
+
+	ref_count = folio_ref_count(folio);
+	map_count = folio_mapcount(folio);
+
+	/*
+	 * Order reads for page refcount and dirty flag
+	 * (see comments in __remove_mapping()).
+	 */
+	smp_rmb();
+
+	if (folio_test_dirty(folio) && !(vma->vm_flags & VM_DROPPABLE)) {
+		/*
+		 * redirtied either using the page table or a previously
+		 * obtained GUP reference.
+		 */
+		set_ptes(mm, address, ptep, pteval, nr_pages);
+		folio_set_swapbacked(folio);
+		return 1;
+	}
+
+	if (ref_count != 1 + map_count) {
+		/*
+		 * Additional reference. Could be a GUP reference or any
+		 * speculative reference. GUP users must mark the folio
+		 * dirty if there was a modification. This folio cannot be
+		 * reclaimed right now either way, so act just like nothing
+		 * happened.
+		 * We'll come back here later and detect if the folio was
+		 * dirtied when the additional reference is gone.
+		 */
+		set_ptes(mm, address, ptep, pteval, nr_pages);
+		return 1;
+	}
+
+	add_mm_counter(mm, MM_ANONPAGES, -nr_pages);
+	return 0;
+}
+
 /*
  * @arg: enum ttu_flags will be passed to this argument
  */

@@ -2227,46 +2278,10 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 
 			/* MADV_FREE page check */
 			if (!folio_test_swapbacked(folio)) {
-				int ref_count, map_count;
-
-				/*
-				 * Synchronize with gup_pte_range():
-				 * - clear PTE; barrier; read refcount
-				 * - inc refcount; barrier; read PTE
-				 */
-				smp_mb();
-
-				ref_count = folio_ref_count(folio);
-				map_count = folio_mapcount(folio);
-
-				/*
-				 * Order reads for page refcount and dirty flag
-				 * (see comments in __remove_mapping()).
-				 */
-				smp_rmb();
-

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 2283

Quoted 10 lines

mm/rmap.c +3 -7

-				if (folio_test_dirty(folio) && !(vma->vm_flags & VM_DROPPABLE)) {
-					/*
-					 * redirtied either using the page table or a previously
-					 * obtained GUP reference.
-					 */
-					set_ptes(mm, address, pvmw.pte, pteval, nr_pages);
-					folio_set_swapbacked(folio);
+				if (commit_ttu_lazyfree_folio(vma, folio, address,
+							      pvmw.pte, pteval,
+							      nr_pages))

With above corrections this would be:

	if (!lazyfree_range(vma, folio, address, pvme.pte, pteval, nr_pages))
	   ...

Quoted 19 lines

mm/rmap.c +0 -15

 					goto walk_abort;
-				} else if (ref_count != 1 + map_count) {
-					/*
-					 * Additional reference. Could be a GUP reference or any
-					 * speculative reference. GUP users must mark the folio
-					 * dirty if there was a modification. This folio cannot be
-					 * reclaimed right now either way, so act just like nothing
-					 * happened.
-					 * We'll come back here later and detect if the folio was
-					 * dirtied when the additional reference is gone.
-					 */
-					set_ptes(mm, address, pvmw.pte, pteval, nr_pages);
-					goto walk_abort;
-				}
-				add_mm_counter(mm, MM_ANONPAGES, -nr_pages);
 				goto discard;
 			}
 
--

Quoted 2 lines

2.34.1

Thanks, Lorenzo

Quoted 23 lines · Lorenzo Stoakes (Oracle)

Quoted 6 lines · Dev Jain

Clean up the code by refactoring the post-pte-clearing path of lazyfree folio unmapping, into commit_ttu_lazyfree_folio().

No functional change is intended.

Signed-off-by Dev Jain <[email protected]>

This is a good idea, and we need more refactoring like this in the rmap code, but comments/nits below.

Quoted 12 lines

---
 mm/rmap.c | 93 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 54 insertions(+), 39 deletions(-)

mm/rmap.c +0 -0

index 1fa020edd954a..a61978141ee3f 100644
@@ -1966,6 +1966,57 @@ static inline unsigned int folio_unmap_pte_batch(struct folio *folio,
 				     FPB_RESPECT_WRITE | FPB_RESPECT_SOFT_DIRTY);
 }
 

Quoted 3 lines

mm/rmap.c static inline unsigned int folio_unmap_pte_batch(struct folio *folio, · line 1969

Quoted 1 line

mm/rmap.c +1 -0

+static inline int commit_ttu_lazyfree_folio(struct vm_area_struct *vma,

Strange name, maybe lazyfree_range()? Not sure what ttu has to do with

ttu means try_to_unmap, just like it is used in TTU_SYNC, TTU_SPLIT_HUGE_PMD, etc. So personally I really like the name, it reads "commit the try-to-unmap of a lazyfree folio". The "commit" comes because the pte clearing has already happened, so now we are deciding if at all to back-off and restore the ptes.

Quoted 7 lines

anything...

mm/rmap.c static inline unsigned int folio_unmap_pte_batch(struct folio *folio, · line 1971

Quoted 2 lines

mm/rmap.c +2 -0

+		struct folio *folio, unsigned long address, pte_t *ptep,
+		pte_t pteval, long nr_pages)

That long nr_pages is really grating now...

Reading the discussion on patch 1, I'll convert this to unsigned long.

Quoted 7 lines

mm/rmap.c static inline unsigned int folio_unmap_pte_batch(struct folio *folio, · line 1972

Quoted 3 lines

mm/rmap.c +1 -0

+{

Come on Dev, it's 2026, why on earth are you returning an integer and not a bool?

Quoted 2 lines

Also it would make sense for this to return false if something breaks, otherwise true.

Yes I was confused on which one of the options to choose :). Since the function does a lot more than just test some functionality (which is what boolean functions usually do) I was feeling weird when returning bool. But yeah alright, I'll convert this to bool.

Quoted 114 lines

Quoted 74 lines

mm/rmap.c +47 -18

+	struct mm_struct *mm = vma->vm_mm;
+	int ref_count, map_count;
+
+	/*
+	 * Synchronize with gup_pte_range():
+	 * - clear PTE; barrier; read refcount
+	 * - inc refcount; barrier; read PTE
+	 */
+	smp_mb();
+
+	ref_count = folio_ref_count(folio);
+	map_count = folio_mapcount(folio);
+
+	/*
+	 * Order reads for page refcount and dirty flag
+	 * (see comments in __remove_mapping()).
+	 */
+	smp_rmb();
+
+	if (folio_test_dirty(folio) && !(vma->vm_flags & VM_DROPPABLE)) {
+		/*
+		 * redirtied either using the page table or a previously
+		 * obtained GUP reference.
+		 */
+		set_ptes(mm, address, ptep, pteval, nr_pages);
+		folio_set_swapbacked(folio);
+		return 1;
+	}
+
+	if (ref_count != 1 + map_count) {
+		/*
+		 * Additional reference. Could be a GUP reference or any
+		 * speculative reference. GUP users must mark the folio
+		 * dirty if there was a modification. This folio cannot be
+		 * reclaimed right now either way, so act just like nothing
+		 * happened.
+		 * We'll come back here later and detect if the folio was
+		 * dirtied when the additional reference is gone.
+		 */
+		set_ptes(mm, address, ptep, pteval, nr_pages);
+		return 1;
+	}
+
+	add_mm_counter(mm, MM_ANONPAGES, -nr_pages);
+	return 0;
+}
+
 /*
  * @arg: enum ttu_flags will be passed to this argument
  */

@@ -2227,46 +2278,10 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 
 			/* MADV_FREE page check */
 			if (!folio_test_swapbacked(folio)) {
-				int ref_count, map_count;
-
-				/*
-				 * Synchronize with gup_pte_range():
-				 * - clear PTE; barrier; read refcount
-				 * - inc refcount; barrier; read PTE
-				 */
-				smp_mb();
-
-				ref_count = folio_ref_count(folio);
-				map_count = folio_mapcount(folio);
-
-				/*
-				 * Order reads for page refcount and dirty flag
-				 * (see comments in __remove_mapping()).
-				 */
-				smp_rmb();
-

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 2283

Quoted 10 lines

mm/rmap.c +3 -7

-				if (folio_test_dirty(folio) && !(vma->vm_flags & VM_DROPPABLE)) {
-					/*
-					 * redirtied either using the page table or a previously
-					 * obtained GUP reference.
-					 */
-					set_ptes(mm, address, pvmw.pte, pteval, nr_pages);
-					folio_set_swapbacked(folio);
+				if (commit_ttu_lazyfree_folio(vma, folio, address,
+							      pvmw.pte, pteval,
+							      nr_pages))

With above corrections this would be:

	if (!lazyfree_range(vma, folio, address, pvme.pte, pteval, nr_pages))
	   ...

Quoted 19 lines

mm/rmap.c +0 -15

 					goto walk_abort;
-				} else if (ref_count != 1 + map_count) {
-					/*
-					 * Additional reference. Could be a GUP reference or any
-					 * speculative reference. GUP users must mark the folio
-					 * dirty if there was a modification. This folio cannot be
-					 * reclaimed right now either way, so act just like nothing
-					 * happened.
-					 * We'll come back here later and detect if the folio was
-					 * dirtied when the additional reference is gone.
-					 */
-					set_ptes(mm, address, pvmw.pte, pteval, nr_pages);
-					goto walk_abort;
-				}
-				add_mm_counter(mm, MM_ANONPAGES, -nr_pages);
 				goto discard;
 			}
 
--

Quoted 2 lines

2.34.1

Thanks, Lorenzo

The ptes are all the same w.r.t belonging to the same type of VMA, and being marked with uffd-wp or all being not marked. Therefore we can batch set uffd-wp markers through install_uffd_wp_ptes_if_needed, and enable batched unmapping of folios belonging to uffd-wp VMAs by dropping that condition from folio_unmap_pte_batch.

It may happen that we don't batch over the entire folio in one go, in which case, we must skip over the current batch. Add a helper to do that - page_vma_mapped_walk_jump() will increment the relevant fields of pvmw by nr pages.

I think that we can get away with just incrementing pvmw->pte and pvmw->address, since looking at the code in page_vma_mapped.c, pvmw->pfn and pvmw->nr_pages are used in conjunction, and pvmw->pgoff and pvmw->nr_pages (in vma_address_end()) are used in conjunction, cancelling out the increment and decrement in the respective fields. But let us not rely on the pvmw implementation and keep this simple.

Export this function to rmap.h to enable future reuse.

Signed-off-by Dev Jain <[email protected]>

---
 include/linux/rmap.h | 10 ++++++++++
 mm/rmap.c            |  8 +++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

include/linux/rmap.h +0 -0

index 8dc0871e5f001..1b7720c66ac87 100644

include/linux/rmap.h +1 -0

+
 /**
  * page_vma_mapped_walk_restart - Restart the page table walk.
  * @pvmw: Pointer to struct page_vma_mapped_walk.

mm/rmap.c +1 -4

index a7570cd037344..dd638429c963e 100644
@@ -1953,9 +1953,6 @@ static inline unsigned int folio_unmap_pte_batch(struct folio *folio,
 	if (pte_unused(pte))
 		return 1;
 
-	if (userfaultfd_wp(vma))
-		return 1;
-
 	/*
 	 * If unmap fails, we need to restore the ptes. To avoid accidentally
 	 * upgrading write permissions for ptes that were not originally

@@ -2235,7 +2232,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 		 * we may want to replace a none pte with a marker pte if
 		 * it's file-backed, so we don't lose the tracking info.
 		 */
-		install_uffd_wp_ptes_if_needed(vma, address, pvmw.pte, pteval, 1);
+		install_uffd_wp_ptes_if_needed(vma, address, pvmw.pte, pteval, nr_pages);
 
 		/* Update high watermark before we lower rss */
 		update_hiwater_rss(mm);

mm/rmap.c +0 -1

 		continue;
 walk_abort:
 		ret = false;
--

2.34.1

Quoted 11 lines · Dev Jain

The ptes are all the same w.r.t belonging to the same type of VMA, and being marked with uffd-wp or all being not marked. Therefore we can batch set uffd-wp markers through install_uffd_wp_ptes_if_needed, and enable batched unmapping of folios belonging to uffd-wp VMAs by dropping that condition from folio_unmap_pte_batch.

It may happen that we don't batch over the entire folio in one go, in which case, we must skip over the current batch. Add a helper to do that - page_vma_mapped_walk_jump() will increment the relevant fields of pvmw by nr pages.

Quoted 6 lines

I think that we can get away with just incrementing pvmw->pte and pvmw->address, since looking at the code in page_vma_mapped.c, pvmw->pfn and pvmw->nr_pages are used in conjunction, and pvmw->pgoff and pvmw->nr_pages (in vma_address_end()) are used in conjunction, cancelling out the increment and decrement in the respective fields. But let us not rely on the pvmw implementation and keep this simple.

This isn't simple...

Quoted 14 lines

Export this function to rmap.h to enable future reuse.

Signed-off-by Dev Jain <[email protected]>

---
 include/linux/rmap.h | 10 ++++++++++
 mm/rmap.c            |  8 +++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

include/linux/rmap.h +0 -0

index 8dc0871e5f001..1b7720c66ac87 100644

include/linux/rmap.h static inline void page_vma_mapped_walk_done(struct page_vma_mapped_walk *pvmw) · line 896

Quoted 5 lines

include/linux/rmap.h +2 -0

@@ -892,6 +892,16 @@ static inline void page_vma_mapped_walk_done(struct page_vma_mapped_walk *pvmw)
              spin_unlock(pvmw->ptl);
 }
 
+static inline void page_vma_mapped_walk_jump(struct page_vma_mapped_walk *pvmw,
+             unsigned int nr)

unsigned long nr_pages... 'nr' is meaningless and you're mixing + matching types for no reason.

include/linux/rmap.h static inline void page_vma_mapped_walk_done(struct page_vma_mapped_walk *pvmw) · line 903

Quoted 7 lines

include/linux/rmap.h +7 -0

+{
+     pvmw->pfn += nr;
+     pvmw->nr_pages -= nr;
+     pvmw->pgoff += nr;
+     pvmw->pte += nr;
+     pvmw->address += nr * PAGE_SIZE;
+}

I absolutely hate this. It's extremely confusing, especially since you're now going from looking at 1 page to nr_pages - 1, jump doesn't really mean anything here, you're losing sight of the batch size and exposing a silly detail to the caller, and I really don't want to 'export' this at this time.

If we must have this, can you please make it static in rmap.c at least for the time being.

Or perhaps instead, have a batched variant of page_vma_mapped_walk(), like page_vma_mapped_walk_batch()?

Quoted 45 lines · Lorenzo Stoakes (Oracle)

Quoted 11 lines · Dev Jain

The ptes are all the same w.r.t belonging to the same type of VMA, and being marked with uffd-wp or all being not marked. Therefore we can batch set uffd-wp markers through install_uffd_wp_ptes_if_needed, and enable batched unmapping of folios belonging to uffd-wp VMAs by dropping that condition from folio_unmap_pte_batch.

It may happen that we don't batch over the entire folio in one go, in which case, we must skip over the current batch. Add a helper to do that - page_vma_mapped_walk_jump() will increment the relevant fields of pvmw by nr pages.

Quoted 6 lines

I think that we can get away with just incrementing pvmw->pte and pvmw->address, since looking at the code in page_vma_mapped.c, pvmw->pfn and pvmw->nr_pages are used in conjunction, and pvmw->pgoff and pvmw->nr_pages (in vma_address_end()) are used in conjunction, cancelling out the increment and decrement in the respective fields. But let us not rely on the pvmw implementation and keep this simple.

This isn't simple...

Quoted 14 lines

Export this function to rmap.h to enable future reuse.

Signed-off-by Dev Jain <[email protected]>

---
 include/linux/rmap.h | 10 ++++++++++
 mm/rmap.c            |  8 +++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

include/linux/rmap.h +0 -0

index 8dc0871e5f001..1b7720c66ac87 100644

include/linux/rmap.h static inline void page_vma_mapped_walk_done(struct page_vma_mapped_walk *pvmw) · line 896

Quoted 5 lines

include/linux/rmap.h +2 -0

@@ -892,6 +892,16 @@ static inline void page_vma_mapped_walk_done(struct page_vma_mapped_walk *pvmw)
              spin_unlock(pvmw->ptl);
 }
 
+static inline void page_vma_mapped_walk_jump(struct page_vma_mapped_walk *pvmw,
+             unsigned int nr)

unsigned long nr_pages... 'nr' is meaningless and you're mixing + matching types for no reason.

Quoted 12 lines

include/linux/rmap.h static inline void page_vma_mapped_walk_done(struct page_vma_mapped_walk *pvmw) · line 903

Quoted 7 lines

include/linux/rmap.h +7 -0

+{
+     pvmw->pfn += nr;
+     pvmw->nr_pages -= nr;
+     pvmw->pgoff += nr;
+     pvmw->pte += nr;
+     pvmw->address += nr * PAGE_SIZE;
+}

I absolutely hate this. It's extremely confusing, especially since you're now going from looking at 1 page to nr_pages - 1, jump doesn't really mean anything here, you're losing sight of the batch size and exposing a silly detail to the caller, and I really don't want to 'export' this at this time.

I’m fairly sure I raised the same concern when Dev first suggested this, but somehow it seems my comment was completely overlooked. :-)

Quoted 6 lines

If we must have this, can you please make it static in rmap.c at least for the time being.

Or perhaps instead, have a batched variant of page_vma_mapped_walk(), like page_vma_mapped_walk_batch()?

Right now, for non-anon pages we face the same issues, but page_vma_mapped_walk() can skip those PTEs once it finds that nr - 1 PTEs are none.

next_pte:

                do {
                        pvmw->address += PAGE_SIZE;
                        if (pvmw->address >= end)
                                return not_found(pvmw);
                        /* Did we cross page table boundary? */
                        if ((pvmw->address & (PMD_SIZE - PAGE_SIZE)) == 0) {
                                if (pvmw->ptl) {
                                        spin_unlock(pvmw->ptl);
                                        pvmw->ptl = NULL;
                                }
                                pte_unmap(pvmw->pte);
                                pvmw->pte = NULL;
                                pvmw->flags |= PVMW_PGTABLE_CROSSED;
                                goto restart;
                        }
                        pvmw->pte++;
                } while (pte_none(ptep_get(pvmw->pte)));

The difference now is that swap entries cannot be skipped.

If we're trying to find page_vma_mapped_walk_batch(), I suppose it could be like this?

bool page_vma_mapped_walk_batch(struct page_vma_mapped_walk *pvmw, unsigned long nr)
{
...
}

static inline bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)

{
        return page_vma_mapped_walk_batch(pvmw, 1);
}

Quoted 60 lines · Barry Song

Quoted 57 lines · Lorenzo Stoakes (Oracle)

Quoted 11 lines · Dev Jain

The ptes are all the same w.r.t belonging to the same type of VMA, and being marked with uffd-wp or all being not marked. Therefore we can batch set uffd-wp markers through install_uffd_wp_ptes_if_needed, and enable batched unmapping of folios belonging to uffd-wp VMAs by dropping that condition from folio_unmap_pte_batch.

It may happen that we don't batch over the entire folio in one go, in which case, we must skip over the current batch. Add a helper to do that - page_vma_mapped_walk_jump() will increment the relevant fields of pvmw by nr pages.

Quoted 6 lines

I think that we can get away with just incrementing pvmw->pte and pvmw->address, since looking at the code in page_vma_mapped.c, pvmw->pfn and pvmw->nr_pages are used in conjunction, and pvmw->pgoff and pvmw->nr_pages (in vma_address_end()) are used in conjunction, cancelling out the increment and decrement in the respective fields. But let us not rely on the pvmw implementation and keep this simple.

This isn't simple...

Quoted 14 lines

Export this function to rmap.h to enable future reuse.

Signed-off-by Dev Jain <[email protected]>

---
 include/linux/rmap.h | 10 ++++++++++
 mm/rmap.c            |  8 +++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

include/linux/rmap.h +0 -0

index 8dc0871e5f001..1b7720c66ac87 100644

include/linux/rmap.h static inline void page_vma_mapped_walk_done(struct page_vma_mapped_walk *pvmw) · line 896

Quoted 5 lines

include/linux/rmap.h +2 -0

@@ -892,6 +892,16 @@ static inline void page_vma_mapped_walk_done(struct page_vma_mapped_walk *pvmw)
              spin_unlock(pvmw->ptl);
 }
 
+static inline void page_vma_mapped_walk_jump(struct page_vma_mapped_walk *pvmw,
+             unsigned int nr)

unsigned long nr_pages... 'nr' is meaningless and you're mixing + matching types for no reason.

include/linux/rmap.h static inline void page_vma_mapped_walk_done(struct page_vma_mapped_walk *pvmw) · line 903

Quoted 7 lines

include/linux/rmap.h +7 -0

+{
+     pvmw->pfn += nr;
+     pvmw->nr_pages -= nr;
+     pvmw->pgoff += nr;
+     pvmw->pte += nr;
+     pvmw->address += nr * PAGE_SIZE;
+}

I absolutely hate this. It's extremely confusing, especially since you're now going from looking at 1 page to nr_pages - 1, jump doesn't really mean anything here, you're losing sight of the batch size and exposing a silly detail to the caller, and I really don't want to 'export' this at this time.

Quoted 2 lines

I’m fairly sure I raised the same concern when Dev first suggested this, but somehow it seems my comment was completely overlooked. :-)

Quoted 42 lines

Quoted 7 lines

If we must have this, can you please make it static in rmap.c at least for the time being.

Or perhaps instead, have a batched variant of page_vma_mapped_walk(), like page_vma_mapped_walk_batch()?

Right now, for non-anon pages we face the same issues, but page_vma_mapped_walk() can skip those PTEs once it finds that nr - 1 PTEs are none.

next_pte:

                do {
                        pvmw->address += PAGE_SIZE;
                        if (pvmw->address >= end)
                                return not_found(pvmw);
                        /* Did we cross page table boundary? */
                        if ((pvmw->address & (PMD_SIZE - PAGE_SIZE)) == 0) {
                                if (pvmw->ptl) {
                                        spin_unlock(pvmw->ptl);
                                        pvmw->ptl = NULL;
                                }
                                pte_unmap(pvmw->pte);
                                pvmw->pte = NULL;
                                pvmw->flags |= PVMW_PGTABLE_CROSSED;
                                goto restart;
                        }
                        pvmw->pte++;
                } while (pte_none(ptep_get(pvmw->pte)));

The difference now is that swap entries cannot be skipped.

If we're trying to find page_vma_mapped_walk_batch(), I suppose it could be like this?

bool page_vma_mapped_walk_batch(struct page_vma_mapped_walk *pvmw, unsigned long nr)
{
...
}

Quoted 4 lines

static inline bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)

{
        return page_vma_mapped_walk_batch(pvmw, 1);
}

Another approach might be to introduce a flag so that page_vma_mapped_walk() knows we are doing batched unmaps and can skip nr - 1 swap entries.

include/linux/rmap.h +0 -0

index 8dc0871e5f00..bf03ae006366 100644

include/linux/rmap.h +1 -0

+
 /* Result flags */
 

/* The page is mapped across page table boundary */

Thanks
Barry

Quoted 11 lines · Dev Jain

The ptes are all the same w.r.t belonging to the same type of VMA, and being marked with uffd-wp or all being not marked. Therefore we can batch set uffd-wp markers through install_uffd_wp_ptes_if_needed, and enable batched unmapping of folios belonging to uffd-wp VMAs by dropping that condition from folio_unmap_pte_batch.

It may happen that we don't batch over the entire folio in one go, in which case, we must skip over the current batch. Add a helper to do that - page_vma_mapped_walk_jump() will increment the relevant fields of pvmw by nr pages.

Quoted 6 lines

I think that we can get away with just incrementing pvmw->pte and pvmw->address, since looking at the code in page_vma_mapped.c, pvmw->pfn and pvmw->nr_pages are used in conjunction, and pvmw->pgoff and pvmw->nr_pages (in vma_address_end()) are used in conjunction, cancelling out the increment and decrement in the respective fields. But let us not rely on the pvmw implementation and keep this simple.

This isn't simple...

Quoted 14 lines

Export this function to rmap.h to enable future reuse.

Signed-off-by Dev Jain <[email protected]>

---
 include/linux/rmap.h | 10 ++++++++++
 mm/rmap.c            |  8 +++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

include/linux/rmap.h +0 -0

index 8dc0871e5f001..1b7720c66ac87 100644

include/linux/rmap.h static inline void page_vma_mapped_walk_done(struct page_vma_mapped_walk *pvmw) · line 896

Quoted 5 lines

include/linux/rmap.h +2 -0

@@ -892,6 +892,16 @@ static inline void page_vma_mapped_walk_done(struct page_vma_mapped_walk *pvmw)
 		spin_unlock(pvmw->ptl);
 }
 
+static inline void page_vma_mapped_walk_jump(struct page_vma_mapped_walk *pvmw,
+		unsigned int nr)

unsigned long nr_pages... 'nr' is meaningless and you're mixing + matching types for no reason.

include/linux/rmap.h static inline void page_vma_mapped_walk_done(struct page_vma_mapped_walk *pvmw) · line 903

Quoted 7 lines

include/linux/rmap.h +7 -0

+{
+	pvmw->pfn += nr;
+	pvmw->nr_pages -= nr;
+	pvmw->pgoff += nr;
+	pvmw->pte += nr;
+	pvmw->address += nr * PAGE_SIZE;
+}

I absolutely hate this. It's extremely confusing, especially since you're now going from looking at 1 page to nr_pages - 1, jump doesn't really mean anything here, you're losing sight of the batch size and exposing a silly detail to the caller, and I really don't want to 'export' this at this time.

If we must have this, can you please make it static in rmap.c at least for the time being.

Or perhaps instead, have a batched variant of page_vma_mapped_walk(), like page_vma_mapped_walk_batch()?

I think that makes a lot more sense...

I mean I kind of hate the pvmw interface in general, this is a hack to handle batching clamped on to the side of it, let's figure out how to do this sensibly and do what's needed rather than adding yet more hacks-on-hacks please.

Quoted 25 lines

include/linux/rmap.h +1 -0

+
 /**
  * page_vma_mapped_walk_restart - Restart the page table walk.
  * @pvmw: Pointer to struct page_vma_mapped_walk.

mm/rmap.c +1 -4

index a7570cd037344..dd638429c963e 100644
@@ -1953,9 +1953,6 @@ static inline unsigned int folio_unmap_pte_batch(struct folio *folio,
 	if (pte_unused(pte))
 		return 1;
 
-	if (userfaultfd_wp(vma))
-		return 1;
-
 	/*
 	 * If unmap fails, we need to restore the ptes. To avoid accidentally
 	 * upgrading write permissions for ptes that were not originally

@@ -2235,7 +2232,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 		 * we may want to replace a none pte with a marker pte if
 		 * it's file-backed, so we don't lose the tracking info.
 		 */
-		install_uffd_wp_ptes_if_needed(vma, address, pvmw.pte, pteval, 1);
+		install_uffd_wp_ptes_if_needed(vma, address, pvmw.pte, pteval, nr_pages);
 

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 2359

Quoted 8 lines

mm/rmap.c +1 -1

 		/* Update high watermark before we lower rss */
 		update_hiwater_rss(mm);

@@ -2359,8 +2356,9 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 		 * If we are sure that we batched the entire folio and cleared
 		 * all PTEs, we can just optimize and stop right here.
 		 */
-		if (nr_pages == folio_nr_pages(folio))
+		if (likely(nr_pages == folio_nr_pages(folio)))

Please don't add random likely()'s based on what you think is likely(). This kind of thing should only be done based on profiling.

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 2361

Quoted 3 lines

mm/rmap.c +1 -0

 			goto walk_done;
+		page_vma_mapped_walk_jump(&pvmw, nr_pages - 1);

(You're now passing a signed long to an unsigned int...!)

Quoted 8 lines

mm/rmap.c +0 -1

 		continue;
 walk_abort:
 		ret = false;
--

2.34.1

Thanks, Lorenzo

Teach folio_dup_swap to handle a batch of consecutive pages. Note that folio_dup_swap already can handle a subset of this: nr_pages == 1 and nr_pages == folio_nr_pages(folio). Generalize this to any nr_pages.

Currently we have a not-so-nice logic of passing in subpage == NULL if we mean to exercise the logic on the entire folio, and subpage != NULL if we want to exercise the logic on only that subpage. Remove this indirection, and explicitly pass subpage != NULL, and the number of pages required.

Quoted 4 lines · Dev Jain

Teach folio_dup_swap to handle a batch of consecutive pages. Note that folio_dup_swap already can handle a subset of this: nr_pages == 1 and nr_pages == folio_nr_pages(folio). Generalize this to any nr_pages.

Thanks a lot for doing this. I was thinking it's about time we respin the batch unmapping of anon folios idea. Barry tried that before with an RFC, and now batching from swap side is easier, so some parts can be done cleaner.

Quoted 5 lines

Currently we have a not-so-nice logic of passing in subpage == NULL if we mean to exercise the logic on the entire folio, and subpage != NULL if we want to exercise the logic on only that subpage. Remove this indirection, and explicitly pass subpage != NULL, and the number of pages required.

I was hoping most callers will just use the whole folio, but after checking your code, yeah, using explicit subpage and nr does fit the other parts better.

mm/swapfile.c int folio_alloc_swap(struct folio *folio) · line 1742

Quoted 11 lines

mm/swapfile.c +2 -1

index 915bc93964dbd..eaf61ae6c3817 100644
@@ -1738,7 +1738,8 @@ int folio_alloc_swap(struct folio *folio)
 /**
  * folio_dup_swap() - Increase swap count of swap entries of a folio.
  * @folio: folio with swap entries bounded.
- * @subpage: if not NULL, only increase the swap count of this subpage.
+ * @subpage: Increase the swap count of this subpage till nr number of
+ * pages forward.

The new nr_pages parameter isn't documented in the kernel-doc comment?

Teach folio_dup_swap to handle a batch of consecutive pages. Note that folio_dup_swap already can handle a subset of this: nr_pages == 1 and nr_pages == folio_nr_pages(folio). Generalize this to any nr_pages.

Currently we have a not-so-nice logic of passing in subpage == NULL if we mean to exercise the logic on the entire folio, and subpage != NULL if we want to exercise the logic on only that subpage. Remove this indirection, and explicitly pass subpage != NULL, and the number of pages required.

Signed-off-by Dev Jain <[email protected]>

---
 mm/rmap.c     |  2 +-
 mm/shmem.c    |  2 +-
 mm/swap.h     |  5 +++--
 mm/swapfile.c | 12 +++++-------
 4 files changed, 10 insertions(+), 11 deletions(-)

mm/rmap.c +1 -1

index dd638429c963e..f6d5b187cf09b 100644
@@ -2282,7 +2282,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 				goto discard;
 			}
 
-			if (folio_dup_swap(folio, subpage) < 0) {
+			if (folio_dup_swap(folio, subpage, 1) < 0) {
 				set_pte_at(mm, address, pvmw.pte, pteval);
 				goto walk_abort;
 			}

mm/shmem.c +1 -1

index 5e7dcf5bc5d3c..86ee34c9b40b3 100644
@@ -1695,7 +1695,7 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
 			spin_unlock(&shmem_swaplist_lock);
 		}
 
-		folio_dup_swap(folio, NULL);
+		folio_dup_swap(folio, folio_page(folio, 0), folio_nr_pages(folio));
 		shmem_delete_from_page_cache(folio, swp_to_radix_entry(folio->swap));
 
 		BUG_ON(folio_mapped(folio));

mm/swap.h +3 -2

index a77016f2423b9..d9cb58ebbddd1 100644
@@ -206,7 +206,7 @@ extern int swap_retry_table_alloc(swp_entry_t entry, gfp_t gfp);
  * folio_put_swap(): does the opposite thing of folio_dup_swap().
  */
 int folio_alloc_swap(struct folio *folio);
-int folio_dup_swap(struct folio *folio, struct page *subpage);
+int folio_dup_swap(struct folio *folio, struct page *subpage, unsigned int nr_pages);
 void folio_put_swap(struct folio *folio, struct page *subpage);
 
 /* For internal use */

@@ -390,7 +390,8 @@ static inline int folio_alloc_swap(struct folio *folio)
 	return -EINVAL;
 }
 
-static inline int folio_dup_swap(struct folio *folio, struct page *page)
+static inline int folio_dup_swap(struct folio *folio, struct page *page,
+				 unsigned int nr_pages)
 {
 	return -EINVAL;
 }

mm/swapfile.c +0 -0

index 915bc93964dbd..eaf61ae6c3817 100644

mm/swapfile.c +3 -7

  *
  * Typically called when the folio is unmapped and have its swap entry to
  * take its place: Swap entries allocated to a folio has count == 0 and pinned

@@ -1752,18 +1753,15 @@ int folio_alloc_swap(struct folio *folio)
  * swap_put_entries_direct on its swap entry before this helper returns, or
  * the swap count may underflow.
  */
-int folio_dup_swap(struct folio *folio, struct page *subpage)
+int folio_dup_swap(struct folio *folio, struct page *subpage,
+		   unsigned int nr_pages)
 {
 	swp_entry_t entry = folio->swap;
-	unsigned long nr_pages = folio_nr_pages(folio);
 
 	VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
 	VM_WARN_ON_FOLIO(!folio_test_swapcache(folio), folio);
 
-	if (subpage) {
-		entry.val += folio_page_idx(folio, subpage);
-		nr_pages = 1;
-	}
+	entry.val += folio_page_idx(folio, subpage);
 
 	return swap_dup_entries_cluster(swap_entry_to_info(entry),
 					swp_offset(entry), nr_pages);
--

2.34.1

Quoted 9 lines · Dev Jain

Teach folio_dup_swap to handle a batch of consecutive pages. Note that folio_dup_swap already can handle a subset of this: nr_pages == 1 and nr_pages == folio_nr_pages(folio). Generalize this to any nr_pages.

Currently we have a not-so-nice logic of passing in subpage == NULL if we mean to exercise the logic on the entire folio, and subpage != NULL if we want to exercise the logic on only that subpage. Remove this indirection, and explicitly pass subpage != NULL, and the number of pages required.

You've made the interface more confusing? Now we can update multiple subpages but specify only one? :)

Let's try to actually refactor this into something sane... see below.

Quoted 63 lines

Signed-off-by Dev Jain <[email protected]>

---
 mm/rmap.c     |  2 +-
 mm/shmem.c    |  2 +-
 mm/swap.h     |  5 +++--
 mm/swapfile.c | 12 +++++-------
 4 files changed, 10 insertions(+), 11 deletions(-)

mm/rmap.c +1 -1

index dd638429c963e..f6d5b187cf09b 100644
@@ -2282,7 +2282,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 				goto discard;
 			}
 
-			if (folio_dup_swap(folio, subpage) < 0) {
+			if (folio_dup_swap(folio, subpage, 1) < 0) {
 				set_pte_at(mm, address, pvmw.pte, pteval);
 				goto walk_abort;
 			}

mm/shmem.c +1 -1

index 5e7dcf5bc5d3c..86ee34c9b40b3 100644
@@ -1695,7 +1695,7 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
 			spin_unlock(&shmem_swaplist_lock);
 		}
 
-		folio_dup_swap(folio, NULL);
+		folio_dup_swap(folio, folio_page(folio, 0), folio_nr_pages(folio));
 		shmem_delete_from_page_cache(folio, swp_to_radix_entry(folio->swap));
 
 		BUG_ON(folio_mapped(folio));

mm/swap.h +3 -2

index a77016f2423b9..d9cb58ebbddd1 100644
@@ -206,7 +206,7 @@ extern int swap_retry_table_alloc(swp_entry_t entry, gfp_t gfp);
  * folio_put_swap(): does the opposite thing of folio_dup_swap().
  */
 int folio_alloc_swap(struct folio *folio);
-int folio_dup_swap(struct folio *folio, struct page *subpage);
+int folio_dup_swap(struct folio *folio, struct page *subpage, unsigned int nr_pages);
 void folio_put_swap(struct folio *folio, struct page *subpage);
 
 /* For internal use */

@@ -390,7 +390,8 @@ static inline int folio_alloc_swap(struct folio *folio)
 	return -EINVAL;
 }
 
-static inline int folio_dup_swap(struct folio *folio, struct page *page)
+static inline int folio_dup_swap(struct folio *folio, struct page *page,
+				 unsigned int nr_pages)
 {
 	return -EINVAL;
 }

mm/swapfile.c +0 -0

index 915bc93964dbd..eaf61ae6c3817 100644

mm/swapfile.c int folio_alloc_swap(struct folio *folio) · line 1742

Quoted 6 lines

mm/swapfile.c +2 -1

@@ -1738,7 +1738,8 @@ int folio_alloc_swap(struct folio *folio)
 /**
  * folio_dup_swap() - Increase swap count of swap entries of a folio.
  * @folio: folio with swap entries bounded.
- * @subpage: if not NULL, only increase the swap count of this subpage.
+ * @subpage: Increase the swap count of this subpage till nr number of
+ * pages forward.

(Obviously also Kairui's point about missing entry in kdoc)

This is REALLY confusing sorry. And this interface is just a horror show.

Before we had subpage == only increase the swap count of the subpage.

Now subpage = the first subpage at which we do that? Please, no.

You just need to rework this interface in general, this is a hack.

Something like:

int __folio_dup_swap(struct folio *folio, unsigned int subpage_start_index,
unsigned int nr_subpages)

{
	...
}

...

int folio_dup_swap_subpage(struct folio *folio, struct page *subpage)

{
	return __folio_dup_swap(folio, folio_page_idx(folio, subpage), 1);
}

int folio_dup_swap(struct folio *folio)

{
	return __folio_dup_swap(folio, 0, folio_nr_pages(folio));
}

Or something like that.

We're definitely _not_ keeping the subpage parameter like that and hacking on batching, PLEASE.

Quoted 26 lines

mm/swapfile.c +3 -7

  *
  * Typically called when the folio is unmapped and have its swap entry to
  * take its place: Swap entries allocated to a folio has count == 0 and pinned

@@ -1752,18 +1753,15 @@ int folio_alloc_swap(struct folio *folio)
  * swap_put_entries_direct on its swap entry before this helper returns, or
  * the swap count may underflow.
  */
-int folio_dup_swap(struct folio *folio, struct page *subpage)
+int folio_dup_swap(struct folio *folio, struct page *subpage,
+		   unsigned int nr_pages)
 {
 	swp_entry_t entry = folio->swap;
-	unsigned long nr_pages = folio_nr_pages(folio);
 
 	VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
 	VM_WARN_ON_FOLIO(!folio_test_swapcache(folio), folio);
 
-	if (subpage) {
-		entry.val += folio_page_idx(folio, subpage);
-		nr_pages = 1;
-	}
+	entry.val += folio_page_idx(folio, subpage);
 
 	return swap_dup_entries_cluster(swap_entry_to_info(entry),
 					swp_offset(entry), nr_pages);
--

Quoted 2 lines

2.34.1

Thanks, Lorenzo

Quoted 111 lines · Lorenzo Stoakes (Oracle)

Quoted 9 lines · Dev Jain

Teach folio_dup_swap to handle a batch of consecutive pages. Note that folio_dup_swap already can handle a subset of this: nr_pages == 1 and nr_pages == folio_nr_pages(folio). Generalize this to any nr_pages.

Currently we have a not-so-nice logic of passing in subpage == NULL if we mean to exercise the logic on the entire folio, and subpage != NULL if we want to exercise the logic on only that subpage. Remove this indirection, and explicitly pass subpage != NULL, and the number of pages required.

You've made the interface more confusing? Now we can update multiple subpages but specify only one? :)

Let's try to actually refactor this into something sane... see below.

Quoted 63 lines

Signed-off-by Dev Jain <[email protected]>

---
 mm/rmap.c     |  2 +-
 mm/shmem.c    |  2 +-
 mm/swap.h     |  5 +++--
 mm/swapfile.c | 12 +++++-------
 4 files changed, 10 insertions(+), 11 deletions(-)

mm/rmap.c +1 -1

index dd638429c963e..f6d5b187cf09b 100644
@@ -2282,7 +2282,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 				goto discard;
 			}
 
-			if (folio_dup_swap(folio, subpage) < 0) {
+			if (folio_dup_swap(folio, subpage, 1) < 0) {
 				set_pte_at(mm, address, pvmw.pte, pteval);
 				goto walk_abort;
 			}

mm/shmem.c +1 -1

index 5e7dcf5bc5d3c..86ee34c9b40b3 100644
@@ -1695,7 +1695,7 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
 			spin_unlock(&shmem_swaplist_lock);
 		}
 
-		folio_dup_swap(folio, NULL);
+		folio_dup_swap(folio, folio_page(folio, 0), folio_nr_pages(folio));
 		shmem_delete_from_page_cache(folio, swp_to_radix_entry(folio->swap));
 
 		BUG_ON(folio_mapped(folio));

mm/swap.h +3 -2

index a77016f2423b9..d9cb58ebbddd1 100644
@@ -206,7 +206,7 @@ extern int swap_retry_table_alloc(swp_entry_t entry, gfp_t gfp);
  * folio_put_swap(): does the opposite thing of folio_dup_swap().
  */
 int folio_alloc_swap(struct folio *folio);
-int folio_dup_swap(struct folio *folio, struct page *subpage);
+int folio_dup_swap(struct folio *folio, struct page *subpage, unsigned int nr_pages);
 void folio_put_swap(struct folio *folio, struct page *subpage);
 
 /* For internal use */

@@ -390,7 +390,8 @@ static inline int folio_alloc_swap(struct folio *folio)
 	return -EINVAL;
 }
 
-static inline int folio_dup_swap(struct folio *folio, struct page *page)
+static inline int folio_dup_swap(struct folio *folio, struct page *page,
+				 unsigned int nr_pages)
 {
 	return -EINVAL;
 }

mm/swapfile.c +0 -0

index 915bc93964dbd..eaf61ae6c3817 100644

mm/swapfile.c int folio_alloc_swap(struct folio *folio) · line 1742

Quoted 6 lines

mm/swapfile.c +2 -1

@@ -1738,7 +1738,8 @@ int folio_alloc_swap(struct folio *folio)
 /**
  * folio_dup_swap() - Increase swap count of swap entries of a folio.
  * @folio: folio with swap entries bounded.
- * @subpage: if not NULL, only increase the swap count of this subpage.
+ * @subpage: Increase the swap count of this subpage till nr number of
+ * pages forward.

(Obviously also Kairui's point about missing entry in kdoc)

This is REALLY confusing sorry. And this interface is just a horror show.

Before we had subpage == only increase the swap count of the subpage.

Now subpage = the first subpage at which we do that? Please, no.

You just need to rework this interface in general, this is a hack.

Something like:

int __folio_dup_swap(struct folio *folio, unsigned int subpage_start_index,
unsigned int nr_subpages)

{
	...
}

...

int folio_dup_swap_subpage(struct folio *folio, struct page *subpage)

{
	return __folio_dup_swap(folio, folio_page_idx(folio, subpage), 1);
}

Quoted 6 lines

int folio_dup_swap(struct folio *folio)

{
	return __folio_dup_swap(folio, 0, folio_nr_pages(folio));
}

Or something like that.

I get the essence of the point you are making.

Since most callers of folio_put_swap mean it for entire folio, perhaps we can have folio_put_swap for these callers, and the ones which are not sure can call folio_put_swap_subpages? Same for folio_dup_swap. And since we are calling it folio_put_swap_subpages, we can retain the subpage parameter?

Quoted 28 lines

We're definitely _not_ keeping the subpage parameter like that and hacking on batching, PLEASE.

Quoted 23 lines

mm/swapfile.c +3 -6

  *
  * Typically called when the folio is unmapped and have its swap entry to
  * take its place: Swap entries allocated to a folio has count == 0 and pinned

@@ -1752,18 +1753,15 @@ int folio_alloc_swap(struct folio *folio)
  * swap_put_entries_direct on its swap entry before this helper returns, or
  * the swap count may underflow.
  */
-int folio_dup_swap(struct folio *folio, struct page *subpage)
+int folio_dup_swap(struct folio *folio, struct page *subpage,
+		   unsigned int nr_pages)
 {
 	swp_entry_t entry = folio->swap;
-	unsigned long nr_pages = folio_nr_pages(folio);
 
 	VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
 	VM_WARN_ON_FOLIO(!folio_test_swapcache(folio), folio);
 
-	if (subpage) {
-		entry.val += folio_page_idx(folio, subpage);
-		nr_pages = 1;
-	}
+	entry.val += folio_page_idx(folio, subpage);
 

Quoted 7 lines

Quoted 5 lines

mm/swapfile.c +0 -1

 	return swap_dup_entries_cluster(swap_entry_to_info(entry),
 					swp_offset(entry), nr_pages);
--

2.34.1

Thanks, Lorenzo

Quoted 5 lines · Lorenzo Stoakes (Oracle)

Quoted 4 lines · Dev Jain

Teach folio_dup_swap to handle a batch of consecutive pages. Note that folio_dup_swap already can handle a subset of this: nr_pages == 1 and nr_pages == folio_nr_pages(folio). Generalize this to any nr_pages.

Quoted 8 lines

Quoted 5 lines

Currently we have a not-so-nice logic of passing in subpage == NULL if we mean to exercise the logic on the entire folio, and subpage != NULL if we want to exercise the logic on only that subpage. Remove this indirection, and explicitly pass subpage != NULL, and the number of pages required.

You've made the interface more confusing? Now we can update multiple subpages but specify only one? :)

Teach folio_put_swap to handle a batch of consecutive pages. Note that folio_put_swap already can handle a subset of this: nr_pages == 1 and nr_pages == folio_nr_pages(folio). Generalize this to any nr_pages.

Currently we have a not-so-nice logic of passing in subpage == NULL if we mean to exercise the logic on the entire folio, and subpage != NULL if we want to exercise the logic on only that subpage. Remove this indirection, and explicitly pass subpage != NULL, and the number of pages required.

Signed-off-by Dev Jain <[email protected]>

---
 mm/memory.c   |  6 +++---
 mm/rmap.c     |  4 ++--
 mm/shmem.c    |  6 +++---
 mm/swap.h     |  5 +++--
 mm/swapfile.c | 13 +++++--------
 5 files changed, 16 insertions(+), 18 deletions(-)

mm/memory.c +3 -3

index 768646c0b3b6a..8249a9b7083ab 100644
@@ -5002,7 +5002,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
        if (unlikely(folio != swapcache)) {
                folio_add_new_anon_rmap(folio, vma, address, RMAP_EXCLUSIVE);
                folio_add_lru_vma(folio, vma);
-               folio_put_swap(swapcache, NULL);
+               folio_put_swap(swapcache, folio_page(swapcache, 0), folio_nr_pages(swapcache));
        } else if (!folio_test_anon(folio)) {
                /*
                 * We currently only expect !anon folios that are fully

@@ -5011,12 +5011,12 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
                VM_WARN_ON_ONCE_FOLIO(folio_nr_pages(folio) != nr_pages, folio);
                VM_WARN_ON_ONCE_FOLIO(folio_mapped(folio), folio);
                folio_add_new_anon_rmap(folio, vma, address, rmap_flags);
-               folio_put_swap(folio, NULL);
+               folio_put_swap(folio, folio_page(folio, 0), folio_nr_pages(folio));
        } else {
                VM_WARN_ON_ONCE(nr_pages != 1 && nr_pages != folio_nr_pages(folio));
                folio_add_anon_rmap_ptes(folio, page, nr_pages, vma, address,
                                         rmap_flags);
-               folio_put_swap(folio, nr_pages == 1 ? page : NULL);
+               folio_put_swap(folio, page, nr_pages);
        }
 
        VM_BUG_ON(!folio_test_anon(folio) ||

mm/rmap.c +2 -2

index f6d5b187cf09b..42f6b00cced01 100644
@@ -2293,7 +2293,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
                         * so we'll not check/care.
                         */
                        if (arch_unmap_one(mm, vma, address, pteval) < 0) {
-                               folio_put_swap(folio, subpage);
+                               folio_put_swap(folio, subpage, 1);
                                set_pte_at(mm, address, pvmw.pte, pteval);
                                goto walk_abort;
                        }

@@ -2301,7 +2301,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
                        /* See folio_try_share_anon_rmap(): clear PTE first. */
                        if (anon_exclusive &&
                            folio_try_share_anon_rmap_pte(folio, subpage)) {
-                               folio_put_swap(folio, subpage);
+                               folio_put_swap(folio, subpage, 1);
                                set_pte_at(mm, address, pvmw.pte, pteval);
                                goto walk_abort;
                        }

mm/shmem.c +0 -0

index 86ee34c9b40b3..d9d216ea28ecb 100644

mm/shmem.c +1 -1

                }
 
                /*

@@ -2196,7 +2196,7 @@ static void shmem_set_folio_swapin_error(struct inode *inode, pgoff_t index,
 
        nr_pages = folio_nr_pages(folio);
        folio_wait_writeback(folio);
-       folio_put_swap(folio, NULL);
+       folio_put_swap(folio, folio_page(folio, 0), folio_nr_pages(folio));
        swap_cache_del_folio(folio);
        /*
         * Don't treat swapin error folio as alloced. Otherwise inode->i_blocks

Quoted 75 lines · Dev Jain

Teach folio_put_swap to handle a batch of consecutive pages. Note that folio_put_swap already can handle a subset of this: nr_pages == 1 and nr_pages == folio_nr_pages(folio). Generalize this to any nr_pages.

Currently we have a not-so-nice logic of passing in subpage == NULL if we mean to exercise the logic on the entire folio, and subpage != NULL if we want to exercise the logic on only that subpage. Remove this indirection, and explicitly pass subpage != NULL, and the number of pages required.

Signed-off-by Dev Jain <[email protected]>

---
 mm/memory.c   |  6 +++---
 mm/rmap.c     |  4 ++--
 mm/shmem.c    |  6 +++---
 mm/swap.h     |  5 +++--
 mm/swapfile.c | 13 +++++--------
 5 files changed, 16 insertions(+), 18 deletions(-)

mm/memory.c +3 -3

index 768646c0b3b6a..8249a9b7083ab 100644
@@ -5002,7 +5002,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
        if (unlikely(folio != swapcache)) {
                folio_add_new_anon_rmap(folio, vma, address, RMAP_EXCLUSIVE);
                folio_add_lru_vma(folio, vma);
-               folio_put_swap(swapcache, NULL);
+               folio_put_swap(swapcache, folio_page(swapcache, 0), folio_nr_pages(swapcache));
        } else if (!folio_test_anon(folio)) {
                /*
                 * We currently only expect !anon folios that are fully

@@ -5011,12 +5011,12 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
                VM_WARN_ON_ONCE_FOLIO(folio_nr_pages(folio) != nr_pages, folio);
                VM_WARN_ON_ONCE_FOLIO(folio_mapped(folio), folio);
                folio_add_new_anon_rmap(folio, vma, address, rmap_flags);
-               folio_put_swap(folio, NULL);
+               folio_put_swap(folio, folio_page(folio, 0), folio_nr_pages(folio));
        } else {
                VM_WARN_ON_ONCE(nr_pages != 1 && nr_pages != folio_nr_pages(folio));
                folio_add_anon_rmap_ptes(folio, page, nr_pages, vma, address,
                                         rmap_flags);
-               folio_put_swap(folio, nr_pages == 1 ? page : NULL);
+               folio_put_swap(folio, page, nr_pages);
        }
 
        VM_BUG_ON(!folio_test_anon(folio) ||

mm/rmap.c +2 -2

index f6d5b187cf09b..42f6b00cced01 100644
@@ -2293,7 +2293,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
                         * so we'll not check/care.
                         */
                        if (arch_unmap_one(mm, vma, address, pteval) < 0) {
-                               folio_put_swap(folio, subpage);
+                               folio_put_swap(folio, subpage, 1);
                                set_pte_at(mm, address, pvmw.pte, pteval);
                                goto walk_abort;
                        }

@@ -2301,7 +2301,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
                        /* See folio_try_share_anon_rmap(): clear PTE first. */
                        if (anon_exclusive &&
                            folio_try_share_anon_rmap_pte(folio, subpage)) {
-                               folio_put_swap(folio, subpage);
+                               folio_put_swap(folio, subpage, 1);
                                set_pte_at(mm, address, pvmw.pte, pteval);
                                goto walk_abort;
                        }

mm/shmem.c +0 -0

index 86ee34c9b40b3..d9d216ea28ecb 100644

mm/shmem.c int shmem_writeout(struct folio *folio, struct swap_iocb **plug, · line 1719

Quoted 5 lines

mm/shmem.c +1 -1

@@ -1716,7 +1716,7 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
                /* Swap entry might be erased by racing shmem_free_swap() */
                if (!error) {
                        shmem_recalc_inode(inode, 0, -nr_pages);
-                       folio_put_swap(folio, NULL);
+                       folio_put_swap(folio, folio_page(folio, 0), folio_nr_pages(folio));

I just realized that we already have a nr_pages variable available here, maybe you can just use that?

Feel free to ignore this if it might touch more code.

Quoted 16 lines

mm/shmem.c +1 -1

                }
 
                /*

@@ -2196,7 +2196,7 @@ static void shmem_set_folio_swapin_error(struct inode *inode, pgoff_t index,
 
        nr_pages = folio_nr_pages(folio);
        folio_wait_writeback(folio);
-       folio_put_swap(folio, NULL);
+       folio_put_swap(folio, folio_page(folio, 0), folio_nr_pages(folio));
        swap_cache_del_folio(folio);
        /*
         * Don't treat swapin error folio as alloced. Otherwise inode->i_blocks

@@ -2426,7 +2426,7 @@ static int shmem_swapin_folio(struct inode *inode, pgoff_t index,
        if (sgp == SGP_WRITE)
                folio_mark_accessed(folio);
 

mm/shmem.c static int shmem_swapin_folio(struct inode *inode, pgoff_t index, · line 2429

Quoted 2 lines

mm/shmem.c +1 -1

-       folio_put_swap(folio, NULL);
+       folio_put_swap(folio, folio_page(folio, 0), folio_nr_pages(folio));

Same here, nr_pages seems good enough?

Teach folio_put_swap to handle a batch of consecutive pages. Note that folio_put_swap already can handle a subset of this: nr_pages == 1 and nr_pages == folio_nr_pages(folio). Generalize this to any nr_pages.

Currently we have a not-so-nice logic of passing in subpage == NULL if we mean to exercise the logic on the entire folio, and subpage != NULL if we want to exercise the logic on only that subpage. Remove this indirection, and explicitly pass subpage != NULL, and the number of pages required.

Signed-off-by Dev Jain <[email protected]>

---
 mm/memory.c   |  6 +++---
 mm/rmap.c     |  4 ++--
 mm/shmem.c    |  6 +++---
 mm/swap.h     |  5 +++--
 mm/swapfile.c | 13 +++++--------
 5 files changed, 16 insertions(+), 18 deletions(-)

mm/memory.c +0 -0

index 768646c0b3b6a..8249a9b7083ab 100644

mm/memory.c +0 -0

 	} else if (!folio_test_anon(folio)) {
 		/*
 		 * We currently only expect !anon folios that are fully

@@ -5011,12 +5011,12 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 		VM_WARN_ON_ONCE_FOLIO(folio_nr_pages(folio) != nr_pages, folio);

mm/rmap.c +1 -1

 				set_pte_at(mm, address, pvmw.pte, pteval);
 				goto walk_abort;
 			}

@@ -2301,7 +2301,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 			/* See folio_try_share_anon_rmap(): clear PTE first. */
 			if (anon_exclusive &&
 			    folio_try_share_anon_rmap_pte(folio, subpage)) {
-				folio_put_swap(folio, subpage);
+				folio_put_swap(folio, subpage, 1);
 				set_pte_at(mm, address, pvmw.pte, pteval);
 				goto walk_abort;
 			}

mm/shmem.c +2 -2

index 86ee34c9b40b3..d9d216ea28ecb 100644
@@ -1716,7 +1716,7 @@ int shmem_writeout(struct folio *folio, struct swap_iocb **plug,
 		/* Swap entry might be erased by racing shmem_free_swap() */
 		if (!error) {
 			shmem_recalc_inode(inode, 0, -nr_pages);
-			folio_put_swap(folio, NULL);
+			folio_put_swap(folio, folio_page(folio, 0), folio_nr_pages(folio));
 		}
 
 		/*

@@ -2196,7 +2196,7 @@ static void shmem_set_folio_swapin_error(struct inode *inode, pgoff_t index,
 
 	nr_pages = folio_nr_pages(folio);
 	folio_wait_writeback(folio);
-	folio_put_swap(folio, NULL);
+	folio_put_swap(folio, folio_page(folio, 0), folio_nr_pages(folio));
 	swap_cache_del_folio(folio);
 	/*
 	 * Don't treat swapin error folio as alloced. Otherwise inode->i_blocks

mm/shmem.c +0 -0

 	swap_cache_del_folio(folio);
 	folio_mark_dirty(folio);
 	put_swap_device(si);

mm/swap.h +3 -2

index d9cb58ebbddd1..73fd9faa67608 100644
@@ -207,7 +207,7 @@ extern int swap_retry_table_alloc(swp_entry_t entry, gfp_t gfp);
  */
 int folio_alloc_swap(struct folio *folio);
 int folio_dup_swap(struct folio *folio, struct page *subpage, unsigned int nr_pages);
-void folio_put_swap(struct folio *folio, struct page *subpage);
+void folio_put_swap(struct folio *folio, struct page *subpage, unsigned int nr_pages);
 
 /* For internal use */
 extern void __swap_cluster_free_entries(struct swap_info_struct *si,

@@ -396,7 +396,8 @@ static inline int folio_dup_swap(struct folio *folio, struct page *page,
 	return -EINVAL;
 }
 
-static inline void folio_put_swap(struct folio *folio, struct page *page)
+static inline void folio_put_swap(struct folio *folio, struct page *page,
+				  unsigned int nr_pages)
 {
 }
 

mm/swapfile.c +0 -0

index eaf61ae6c3817..c66aa6d15d479 100644

mm/swapfile.c +2 -2

  *
  * This won't free the swap slots even if swap count drops to zero, they are
  * still pinned by the swap cache. User may call folio_free_swap to free them.
  * Context: Caller must ensure the folio is locked and in the swap cache.
  */
-void folio_put_swap(struct folio *folio, struct page *subpage)
+void folio_put_swap(struct folio *folio, struct page *subpage,
+		    unsigned int nr_pages)
 {
 	swp_entry_t entry = folio->swap;
-	unsigned long nr_pages = folio_nr_pages(folio);
 	struct swap_info_struct *si = __swap_entry_to_info(entry);
 

mm/swapfile.c +1 -2

 }

@@ -2334,7 +2331,7 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
 		new_pte = pte_mkuffd_wp(new_pte);
 setpte:
 	set_pte_at(vma->vm_mm, addr, pte, new_pte);
-	folio_put_swap(swapcache, folio_file_page(swapcache, swp_offset(entry)));
+	folio_put_swap(swapcache, folio_file_page(swapcache, swp_offset(entry)), 1);
 out:
 	if (pte)
 		pte_unmap_unlock(pte, ptl);
--

2.34.1

In the quest of enabling batched unmapping of anonymous folios, we need to handle the sharing of exclusive pages. Hence, a batched version of folio_try_share_anon_rmap_pte is required.

Currently, the sole purpose of nr_pages in __folio_try_share_anon_rmap is to do some rmap sanity checks. Add helpers to set and clear the PageAnonExclusive bit on a batch of nr_pages. Note that __folio_try_share_anon_rmap can receive nr_pages == HPAGE_PMD_NR from the PMD path, but currently we only clear the bit on the head page. Retain this behaviour by setting nr_pages = 1 in case the caller is folio_try_share_anon_rmap_pmd.

Signed-off-by Dev Jain <[email protected]>

---
 include/linux/page-flags.h | 11 +++++++++++
 include/linux/rmap.h       | 28 ++++++++++++++++++++++++++--
 mm/rmap.c                  |  2 +-
 3 files changed, 38 insertions(+), 3 deletions(-)

include/linux/page-flags.h +0 -0

index 0e03d816e8b9d..1d74ed9a28c41 100644

include/linux/rmap.h +1 -1

 		return 0;
 	}
 

@@ -766,7 +770,7 @@ static __always_inline int __folio_try_share_anon_rmap(struct folio *folio,
 
 	if (unlikely(folio_maybe_dma_pinned(folio)))
 		return -EBUSY;
-	ClearPageAnonExclusive(page);
+	ClearPagesAnonExclusive(page, nr_pages);
 
 	/*
 	 * This is conceptually a smp_wmb() paired with the smp_rmb() in

include/linux/rmap.h +7 -0

+ */
+static inline int folio_try_share_anon_rmap_ptes(struct folio *folio,
+		struct page *page, unsigned int nr)
+{
+	return __folio_try_share_anon_rmap(folio, page, nr, PGTABLE_LEVEL_PTE);
+}
+
 /**
  * folio_try_share_anon_rmap_pmd - try marking an exclusive anonymous page
  *				   range mapped by a PMD possibly shared to

mm/rmap.c +0 -0

index 42f6b00cced01..bba5b571946d8 100644

mm/rmap.c +0 -1

 				folio_put_swap(folio, subpage, 1);
 				set_pte_at(mm, address, pvmw.pte, pteval);
 				goto walk_abort;
--

2.34.1

Quoted 27 lines · Dev Jain

In the quest of enabling batched unmapping of anonymous folios, we need to handle the sharing of exclusive pages. Hence, a batched version of folio_try_share_anon_rmap_pte is required.

Currently, the sole purpose of nr_pages in __folio_try_share_anon_rmap is to do some rmap sanity checks. Add helpers to set and clear the PageAnonExclusive bit on a batch of nr_pages. Note that __folio_try_share_anon_rmap can receive nr_pages == HPAGE_PMD_NR from the PMD path, but currently we only clear the bit on the head page. Retain this behaviour by setting nr_pages = 1 in case the caller is folio_try_share_anon_rmap_pmd.

Signed-off-by Dev Jain <[email protected]>

---
 include/linux/page-flags.h | 11 +++++++++++
 include/linux/rmap.h       | 28 ++++++++++++++++++++++++++--
 mm/rmap.c                  |  2 +-
 3 files changed, 38 insertions(+), 3 deletions(-)

include/linux/page-flags.h +0 -0

index 0e03d816e8b9d..1d74ed9a28c41 100644
@@ -1178,6 +1178,17 @@ static __always_inline void __ClearPageAnonExclusive(struct page *page)
 	__clear_bit(PG_anon_exclusive, &PF_ANY(page, 1)->flags.f);
 }
 

include/linux/page-flags.h static __always_inline void __ClearPageAnonExclusive(struct page *page) · line 1182

Quoted 2 lines

include/linux/page-flags.h +2 -0

+static __always_inline void ClearPagesAnonExclusive(struct page *page,
+		unsigned int nr)

You're randomly moving between nr and nr_pages, can we just consistently use nr_pages please.

include/linux/page-flags.h static __always_inline void __ClearPageAnonExclusive(struct page *page) · line 1186

Quoted 5 lines

include/linux/page-flags.h +4 -0

+{
+	for (;;) {
+		ClearPageAnonExclusive(page);
+		if (--nr == 0)

You really require nr to != 0 here or otherwise you're going to be clearing 4 billion pages :)

include/linux/page-flags.h static __always_inline void __ClearPageAnonExclusive(struct page *page) · line 1190

Quoted 5 lines

include/linux/page-flags.h +4 -0

+			break;
+		++page;
+	}
+}

Can we put this in mm.h or somewhere else please, and can we do away with this

HorribleNamingConvention, this is new, we can 'get away' with making it something sensible :)

I wonder if we shouldn't also add a folio pointer here, and some VM_WARN_ON_ONCE()'s. Like:

static inline void folio_clear_page_batch(struct folio *folio,
struct page *first_subpage, unsigned int nr_pages)

{
	struct page *subpage = first_subpage;

	VM_WARN_ON_ONCE(!nr_pages);
	VM_WARN_ON_ONCE(... check first_subpage in folio ...);
	VM_WARN_ON_ONCE(... check first_subpage -> first_subpage + nr_pages in folio ...);

	while (nr_pages--)
		ClearPageAnonExclusive(subpage++);
}

Quoted 12 lines

include/linux/page-flags.h +1 -0

+
 #ifdef CONFIG_MMU
 #define __PG_MLOCKED		(1UL << PG_mlocked)
 #else

include/linux/rmap.h +0 -0

index 1b7720c66ac87..7a67776dca3fe 100644
@@ -712,9 +712,13 @@ static __always_inline int __folio_try_share_anon_rmap(struct folio *folio,
 	VM_WARN_ON_FOLIO(!PageAnonExclusive(page), folio);
 	__folio_rmap_sanity_checks(folio, page, nr_pages, level);
 

include/linux/rmap.h static __always_inline int __folio_try_share_anon_rmap(struct folio *folio, · line 715

Quoted 1 line

include/linux/rmap.h +1 -0

+	/* We only clear anon-exclusive from head page of PMD folio */

Is this accurate? David? I thought anon exclusive was per-subpage for any large folio...?

If we're explicitly doing this for some reason here, then why introduce it?

include/linux/rmap.h static __always_inline int __folio_try_share_anon_rmap(struct folio *folio, · line 721

Quoted 7 lines

include/linux/rmap.h +4 -1

+	if (level == PGTABLE_LEVEL_PMD)
+		nr_pages = 1;
+
 	/* device private folios cannot get pinned via GUP. */
 	if (unlikely(folio_is_device_private(folio))) {
-		ClearPageAnonExclusive(page);
+		ClearPagesAnonExclusive(page, nr_pages);

I really kind of hate this 'we are looking at subpage X with variable page in folio Y, but we don't mention Y' thing. It's super confusing that we have a pointer to a thing which sometimes we deref and treat as a value we care about and sometimes treat as an array.

This pattern exists throughout all the batched stuff and I kind of hate it everywhere.

I guess the batching means that we are looking at a sub-folio range.

If C had a better type system we could somehow have a type that encoded this, but it doesn't :>)

But I wonder if we shouldn't just go ahead and rename page -> pages and be consistent about this?

Quoted 14 lines

include/linux/rmap.h +1 -1

 		return 0;
 	}
 

@@ -766,7 +770,7 @@ static __always_inline int __folio_try_share_anon_rmap(struct folio *folio,
 
 	if (unlikely(folio_maybe_dma_pinned(folio)))
 		return -EBUSY;
-	ClearPageAnonExclusive(page);
+	ClearPagesAnonExclusive(page, nr_pages);
 
 	/*
 	 * This is conceptually a smp_wmb() paired with the smp_rmb() in

include/linux/rmap.h static inline int folio_try_share_anon_rmap_pte(struct folio *folio, · line 814

Quoted 7 lines

include/linux/rmap.h +4 -0

@@ -804,6 +808,26 @@ static inline int folio_try_share_anon_rmap_pte(struct folio *folio,
 	return __folio_try_share_anon_rmap(folio, page, 1, PGTABLE_LEVEL_PTE);
 }
 
+/**
+ * folio_try_share_anon_rmap_ptes - try marking exclusive anonymous pages
+ *				    mapped by PTEs possibly shared to prepare
+ *				   for KSM or temporary unmapping

This description is very confusing. 'Try marking exclusive anonymous pages [... marking them as what?] mapped by PTEs[, or (]possibly shared[, or )] to prepare for KSM[under what circumstances? Why mention KSM here?] or temporary unmapping [why temporary?]

OK I think you mean to say 'marking' them as 'possibly' shared.

But really by 'shared' you mean clearing anon exclusive right? So maybe the function name and description should reference that instead.

But this needs clarifying. This isn't an exercise in minimum number of words to describe the function.

Ohhh now I see this is what the comment is in folio_try_share_anon_rmap_pte() :P

Well, I wish we could update the original too ;) but OK this is fine as-is to matc that then.

include/linux/rmap.h static inline int folio_try_share_anon_rmap_pte(struct folio *folio, · line 823

Quoted 9 lines

include/linux/rmap.h +9 -0

+ * @folio:	The folio to share a mapping of
+ * @page:	The first mapped exclusive page of the batch
+ * @nr_pages:	The number of pages to share (batch size)
+ *
+ * See folio_try_share_anon_rmap_pte for full description.
+ *
+ * Context: The caller needs to hold the page table lock and has to have the
+ * page table entries cleared/invalidated. Those PTEs used to map consecutive
+ * pages of the folio passed here. The PTEs are all in the same PMD and VMA.

Can we VM_WARN_ON_ONCE() any of this? Not completely a necessity.

Quoted 16 lines

include/linux/rmap.h +7 -0

+ */
+static inline int folio_try_share_anon_rmap_ptes(struct folio *folio,
+		struct page *page, unsigned int nr)
+{
+	return __folio_try_share_anon_rmap(folio, page, nr, PGTABLE_LEVEL_PTE);
+}
+
 /**
  * folio_try_share_anon_rmap_pmd - try marking an exclusive anonymous page
  *				   range mapped by a PMD possibly shared to

mm/rmap.c +0 -0

index 42f6b00cced01..bba5b571946d8 100644

mm/rmap.c static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma, · line 2303

Quoted 5 lines

mm/rmap.c +1 -1

@@ -2300,7 +2300,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 
 			/* See folio_try_share_anon_rmap(): clear PTE first. */
 			if (anon_exclusive &&
-			    folio_try_share_anon_rmap_pte(folio, subpage)) {
+			    folio_try_share_anon_rmap_ptes(folio, subpage, 1)) {

I guess this is because you intend to make this batched later with >1, but I don't see the point of adding this since folio_try_share_anon_rmap_pte() already does what you're doing here.

So why not just change this when you actually batch?

Buuuut.... haven't you not already changed this whole function to now 'jump' ahead if batched, so why are we only specifying nr_pages = 1 here?

Honestly I think this function needs to be fully refactored away from the appalling giant-ball-of-string mess it is now before we try to add in batching to be honest.

Let's not accumulate more technical debt.

Quoted 8 lines

mm/rmap.c +0 -1

 				folio_put_swap(folio, subpage, 1);
 				set_pte_at(mm, address, pvmw.pte, pteval);
 				goto walk_abort;
--

2.34.1

Thanks, Lorenzo

Quoted 8 lines

Quoted 8 lines

include/linux/rmap.h +0 -0

index 1b7720c66ac87..7a67776dca3fe 100644
@@ -712,9 +712,13 @@ static __always_inline int __folio_try_share_anon_rmap(struct folio *folio,
 	VM_WARN_ON_FOLIO(!PageAnonExclusive(page), folio);
 	__folio_rmap_sanity_checks(folio, page, nr_pages, level);
 

Quoted 4 lines

include/linux/rmap.h static __always_inline int __folio_try_share_anon_rmap(struct folio *folio, · line 715

Quoted 1 line

include/linux/rmap.h +1 -0

+	/* We only clear anon-exclusive from head page of PMD folio */

Is this accurate? David? I thought anon exclusive was per-subpage for any large folio...?

The current behaviour is to do this only. I was also surprised with this, so I had dug in and found out:

https://lore.kernel.org/all/[email protected]/

where David says:

"Long story short: once
PTE-mapped, we have to track information about exclusivity per sub-page, but until then, we can just track it for the compound page in the head page and not having to update a whole bunch of subpages all of the time for a simple PMD mapping of a THP."