Optimize anonymous large folio unmapping

If any page of a folio is poisoned, try_to_unmap() is called with the intent to remove any mappings pointing to any part of the folio.

Therefore, an hwpoison check on a single page does not make sense (although everything works out because the memory-failure path sets hwpoison on the head page too). Convert these checks to check whether the folio has at least one hwpoisoned page.

While at it, convert VM_BUG_* to VM_WARN_*.

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

---
 mm/rmap.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

mm/rmap.c +0 -0

index fb3c351f8c458..430c91c8fe2ae 100644
@@ -2116,10 +2116,11 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 			bool anon = folio_test_anon(folio);

Quoted 7 lines · Dev Jain

If any page of a folio is poisoned, try_to_unmap() is called with the intent to remove any mappings pointing to any part of the folio.

Therefore, an hwpoison check on a single page does not make sense (although everything works out because the memory-failure path sets hwpoison on the head page too). Convert these checks to check whether the folio has at least one hwpoisoned page.

I think the reasoning is different and should better be described:

1) For hugetlb, we will always get subpage == head page (single PTE -> first PFN).

try_memory_failure_hugetlb()->try_memory_failure_hugetlb()->hugetlb_update_hwpoison() will do the folio_test_set_hwpoison().

folio_test_set_hwpoison() is the same as setting it on the head page.

For hugetlb, subpage==headpage

That's why PageHWPoison(subpage) used to work.

2) try_to_unmap() cannot handle large folios (besides hugetlb folios).

unmap_poisoned_folio() spells that out. That's why subpage==headpage right now.

That's why PageHWPoison(subpage) used to work.

Quoted 14 lines

While at it, convert VM_BUG_* to VM_WARN_*.

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

---
 mm/rmap.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

mm/rmap.c +0 -0

index fb3c351f8c458..430c91c8fe2ae 100644
@@ -2116,10 +2116,11 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 			bool anon = folio_test_anon(folio);

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

Quoted 10 lines

mm/rmap.c +4 -3

 
 			/*
-			 * The try_to_unmap() is only passed a hugetlb page
-			 * in the case where the hugetlb page is poisoned.
+			 * The try_to_unmap() is only passed a hugetlb folio
+			 * in the case where the hugetlb folio contains a
+			 * poisoned page.
 			 */
-			VM_BUG_ON_PAGE(!PageHWPoison(subpage), subpage);
+			VM_WARN_ON_FOLIO(!folio_contain_hwpoisoned_page(folio), folio);

I think we could also just use folio_test_hwpoison() here.

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

Quoted 12 lines

mm/rmap.c +1 -1

 			/*
 			 * huge_pmd_unshare may unmap an entire PMD page.
 			 * There is no way of knowing exactly which PMDs may

@@ -2198,7 +2199,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 		/* Update high watermark before we lower rss */
 		update_hiwater_rss(mm);
 
-		if (PageHWPoison(subpage) && (flags & TTU_HWPOISON)) {
+		if (folio_contain_hwpoisoned_page(folio) && (flags & TTU_HWPOISON)) {
 			pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
 			if (folio_test_hugetlb(folio)) {
 				hugetlb_count_sub(folio_nr_pages(folio), mm);

And here. Which might be cleaner, because folio_contain_hwpoisoned_page() is so far never called on hugetlb folios.

Because folio_set_has_hwpoisoned() is only ever used on THPs, not hugetlb. It would currently work, but the expectation so far is that these primitives are for small folios+thp, not hugetlb.

You could add a comment here, that with TTU_HWPOISON, we only ever expect small folios or hugetlb folios for now.

Quoted 23 lines · David Hildenbrand (Arm)

Quoted 7 lines · Dev Jain

If any page of a folio is poisoned, try_to_unmap() is called with the intent to remove any mappings pointing to any part of the folio.

Therefore, an hwpoison check on a single page does not make sense (although everything works out because the memory-failure path sets hwpoison on the head page too). Convert these checks to check whether the folio has at least one hwpoisoned page.

I think the reasoning is different and should better be described:

1) For hugetlb, we will always get subpage == head page (single PTE -> first PFN).

try_memory_failure_hugetlb()->try_memory_failure_hugetlb()->hugetlb_update_hwpoison() will do the folio_test_set_hwpoison().

folio_test_set_hwpoison() is the same as setting it on the head page.

For hugetlb, subpage==headpage

That's why PageHWPoison(subpage) used to work.

Quoted 3 lines

2) try_to_unmap() cannot handle large folios (besides hugetlb folios).

unmap_poisoned_folio() spells that out. That's why subpage==headpage right now.

Should have mentioned: subpage != headpage for contpte mapped hugetlb folios on arm64.

Simplify try_to_unmap_one by separating out the hugetlb parts into try_to_unmap_hugetlb_one.

To understand the correctness of the refactoring, the following points are noted:

1. try_to_unmap() is called for hugetlb folios only when they are hwpoisoned.

2. A hugetlb VMA cannot be mlocked.

3. The pvmw API sets pvmw.pte to the base of the hugetlb folio (pvmw.pmd is NULL).

4. We won't ever process a softleaf entry that encodes a hugetlb folio; hugetlb folios are never swapped out, migration entries will be skipped (PVMW_MIGRATION not passed) and device-exclusive does not work for hugetlb.

5. uffd-wp bit is lost when converting pvmw.pte to hwpoison softleaf (therefore no need to call pte_install_uffd_wp_if_needed after clearing pvmw.pte)

6. TTU_HWPOISON is always present; for it to not be present, either folio has to be in swapcache, or mapping_can_writeback() is true (see unmap_poisoned_folio), none of which is true for hugetlb folios.

7. hugetlb uses separate counters from normal rss counters, therefore update_highwater_rss() need not be called.

While at it:

- Change VM_BUG_* to VM_WARN_*.

- Do not declare variables which are only used once

- Assert that the subpage derived by the pvmw walk is exactly the head page. This is because try_to_unmap() does not remember the specific subpage which was hwpoisoned, and, since we cannot munmap/mremap across a hugetlb folio partially, the first pte mapping the hugetlb folio (in case of a contpte or contpmd mapped folio) cannot ever point to an intermediate page.

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

---
 mm/rmap.c | 203 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 121 insertions(+), 82 deletions(-)

mm/rmap.c +0 -0

index 430c91c8fe2ae..06ab1158d4cd1 100644

mm/rmap.c +32 -0

+
+	/*
+	 * huge_pmd_unshare may unmap an entire PMD page. There is no way of
+	 * knowing exactly which PMDs may be cached for this mm, so we must
+	 * flush them all. start/end were already adjusted above to cover this
+	 * range.
+	 */
+	flush_cache_range(vma, range->start, range->end);
+
+	/*
+	 * To call huge_pmd_unshare, i_mmap_rwsem must be held in write mode.
+	 * Caller needs to explicitly do this outside rmap routines.
+	 *
+	 * We also must hold hugetlb vma_lock in write mode. Lock order dictates
+	 * acquiring vma_lock BEFORE i_mmap_rwsem. We can only try lock here and
+	 * fail if unsuccessful.
+	 */
+	if (!folio_test_anon(folio)) {
+		struct mmu_gather tlb;
+
+		VM_WARN_ON(!(flags & TTU_RMAP_LOCKED));
+		if (!hugetlb_vma_trylock_write(vma)) {
+			ret = false;
+			goto walk_done;
+		}
+
+		tlb_gather_mmu_vma(&tlb, vma);
+		if (huge_pmd_unshare(&tlb, vma, address, pvmw->pte)) {
+			hugetlb_vma_unlock_write(vma);
+			huge_pmd_unshare_flush(&tlb, vma);
+			tlb_finish_mmu(&tlb);
+			/*

mm/rmap.c +14 -0

+	pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
+	hugetlb_count_sub(folio_nr_pages(folio), mm);
+	set_huge_pte_at(mm, address, pvmw->pte, pteval, hsz);
+	hugetlb_remove_rmap(folio);
+	folio_put_refs(folio, 1);
+
+walk_done:
+	page_vma_mapped_walk_done(pvmw);
+	return ret;
+}
+
+static bool try_to_unmap_hugetlb_one(struct folio *folio,
+		struct vm_area_struct *vma, unsigned long address, void *arg)
+{

mm/rmap.c +4 -0

+	mmu_notifier_invalidate_range_end(&range);
+	return ret;
+}
+

Quoted 9 lines

---
 mm/rmap.c | 203 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 121 insertions(+), 82 deletions(-)

mm/rmap.c +0 -0

index 430c91c8fe2ae..06ab1158d4cd1 100644

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

Quoted 8 lines

mm/rmap.c +5 -0

@@ -1978,6 +1978,121 @@ static inline unsigned int folio_unmap_pte_batch(struct folio *folio,
 				     FPB_RESPECT_WRITE | FPB_RESPECT_SOFT_DIRTY);
 }
 
+static bool __try_to_unmap_hugetlb_one(struct folio *folio,
+		struct vm_area_struct *vma, struct page_vma_mapped_walk *pvmw,
+		struct mmu_notifier_range *range, enum ttu_flags flags)
+{
+	unsigned long hsz = huge_page_size(hstate_vma(vma));

Can be const.

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

Quoted 1 line

mm/rmap.c +1 -0

+	unsigned long address = pvmw->address;

Can be const. But do we even need the temporary variable?

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

Quoted 2 lines

mm/rmap.c +2 -0

+	struct mm_struct *mm = vma->vm_mm;
+	struct page *subpage;

a) subpages do not exist. It's just a folio page.
b) subpages are irrelevant for hugetlb, just drop anything that deals with them.

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

Quoted 3 lines

mm/rmap.c +3 -0

+	bool ret = true;
+	pte_t pteval;
+

Worth adding a comment here, why we don't need a loop.

/* There is only a single mapping in a VMA. */

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

Quoted 4 lines

mm/rmap.c +4 -0

+	if (!page_vma_mapped_walk(pvmw))
+		return true;
+
+	pteval = ptep_get(pvmw->pte);

Should we now properly use huge_ptep_get()?

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

Quoted 2 lines

mm/rmap.c +2 -0

+	VM_WARN_ON(!pte_present(pteval));
+	subpage = folio_page(folio, pte_pfn(pteval) - folio_pfn(folio));

No need for subpages.

	return __rste_to_pte(pte_val(*ptep));
}

Quoted 4 lines

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

Quoted 2 lines

mm/rmap.c +2 -0

+	VM_WARN_ON(!pte_present(pteval));
+	subpage = folio_page(folio, pte_pfn(pteval) - folio_pfn(folio));

No need for subpages.

So plain ptep_get() can feed raw huge-entry bits into pte_pfn(), and the derived subpage can be wrong.

Quoted 8 lines

	return __rste_to_pte(pte_val(*ptep));
}

Quoted 4 lines

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

Quoted 2 lines

mm/rmap.c +2 -0

+	VM_WARN_ON(!pte_present(pteval));
+	subpage = folio_page(folio, pte_pfn(pteval) - folio_pfn(folio));

No need for subpages.

Quoted 2 lines

So plain ptep_get() can feed raw huge-entry bits into pte_pfn(), and the derived subpage can be wrong.

Good question which impact that might have in practice?

Quoted 8 lines · David Hildenbrand (Arm)

Quoted 6 lines

    return __rste_to_pte(pte_val(*ptep));
}

So plain ptep_get() can feed raw huge-entry bits into pte_pfn(), and the derived subpage can be wrong.

Good question which impact that might have in practice?

The subpage check can warn, but we still pass that subpage to make_hwpoison_entry(). So the hwpoison marker can end up with the wrong PFN?

+    subpage = folio_page(folio, pte_pfn(pteval) - folio_pfn(folio)); +    VM_WARN_ON(folio_page(folio, 0) != subpage);
[...]
+    pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));

Quoted 12 lines · Lance Yang

Quoted 8 lines · David Hildenbrand (Arm)

Quoted 6 lines

    return __rste_to_pte(pte_val(*ptep));
}

So plain ptep_get() can feed raw huge-entry bits into pte_pfn(), and the derived subpage can be wrong.

Good question which impact that might have in practice?

Quoted 8 lines

The subpage check can warn, but we still pass that subpage to make_hwpoison_entry(). So the hwpoison marker can end up with the wrong PFN?

+    subpage = folio_page(folio, pte_pfn(pteval) - folio_pfn(folio)); +    VM_WARN_ON(folio_page(folio, 0) != subpage);
[...]
+    pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));

My s390x page table knowledge is a bit rusty.

IIUC, it would be a problem if some PTE bits in segment/region entries (pmd/pud/ ...) would pass the

	 pte_pfn(x) -> (pte_val(x) >> PAGE_SHIFT)

check. I don't think this applies, because

While

	#define _SEGMENT_ENTRY_ORIGIN_LARGE ~0xfffffUL

We also have

	#define _SEGMENT_ENTRY_ORIGIN ~0x7ffUL

So these bits are not actually used.

What __rste_to_pte() primarily does is reshuffling present bits etc.

So using any other bits besides the PFN would be problematic I guess.

Am I wrong or isn't the present bit already at a different location? For prot-none hugetlb folios there might be a real issue, as the PTE present bit corresponds to the PMD/PUD read-permission bit.

Oh my :)

So yeah, we should probably fix that ahead of time unless I am missing something? Good that we separate that hugetlb crap out.

Quoted 49 lines · David Hildenbrand (Arm)

Quoted 12 lines · Lance Yang

Quoted 8 lines · David Hildenbrand (Arm)

Quoted 6 lines

     return __rste_to_pte(pte_val(*ptep));
}

So plain ptep_get() can feed raw huge-entry bits into pte_pfn(), and the derived subpage can be wrong.

Good question which impact that might have in practice?

Quoted 8 lines

The subpage check can warn, but we still pass that subpage to make_hwpoison_entry(). So the hwpoison marker can end up with the wrong PFN?

+    subpage = folio_page(folio, pte_pfn(pteval) - folio_pfn(folio)); +    VM_WARN_ON(folio_page(folio, 0) != subpage);
[...]
+    pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));

My s390x page table knowledge is a bit rusty.

IIUC, it would be a problem if some PTE bits in segment/region entries (pmd/pud/ ...) would pass the

     pte_pfn(x) -> (pte_val(x) >> PAGE_SHIFT)

check. I don't think this applies, because

While
    #define _SEGMENT_ENTRY_ORIGIN_LARGE ~0xfffffUL

We also have

    #define _SEGMENT_ENTRY_ORIGIN ~0x7ffUL

So these bits are not actually used.

What __rste_to_pte() primarily does is reshuffling present bits etc.

So using any other bits besides the PFN would be problematic I guess.

Am I wrong or isn't the present bit already at a different location? For prot-none hugetlb folios there might be a real issue, as the PTE present bit corresponds to the PMD/PUD read-permission bit.

Quoted 4 lines

Oh my :)

So yeah, we should probably fix that ahead of time unless I am missing something? Good that we separate that hugetlb crap out.

Yeah, looks like this was already there before the split. Should this be fixed separately?

Quoted 54 lines · Lance Yang

Quoted 51 lines · David Hildenbrand (Arm)

Quoted 12 lines · Lance Yang

Quoted 8 lines · David Hildenbrand (Arm)

Quoted 6 lines

     return __rste_to_pte(pte_val(*ptep));
}

So plain ptep_get() can feed raw huge-entry bits into pte_pfn(), and the derived subpage can be wrong.

Good question which impact that might have in practice?

Quoted 8 lines

The subpage check can warn, but we still pass that subpage to make_hwpoison_entry(). So the hwpoison marker can end up with the wrong PFN?

+    subpage = folio_page(folio, pte_pfn(pteval) - folio_pfn(folio)); +    VM_WARN_ON(folio_page(folio, 0) != subpage);
[...]
+    pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));

My s390x page table knowledge is a bit rusty.

IIUC, it would be a problem if some PTE bits in segment/region entries (pmd/pud/ ...) would pass the

     pte_pfn(x) -> (pte_val(x) >> PAGE_SHIFT)

check. I don't think this applies, because

While
    #define _SEGMENT_ENTRY_ORIGIN_LARGE ~0xfffffUL

We also have

    #define _SEGMENT_ENTRY_ORIGIN ~0x7ffUL

So these bits are not actually used.

What __rste_to_pte() primarily does is reshuffling present bits etc.

So using any other bits besides the PFN would be problematic I guess.

Am I wrong or isn't the present bit already at a different location? For prot-none hugetlb folios there might be a real issue, as the PTE present bit corresponds to the PMD/PUD read-permission bit.

Oh my :)

Quoted 5 lines

Quoted 2 lines

So yeah, we should probably fix that ahead of time unless I am missing something? Good that we separate that hugetlb crap out.

Yeah, looks like this was already there before the split. Should this be fixed separately?

Same bug is there in try_to_migrate_one(), check_pte(), remove_migration_pte() and prot_none_hugetlb_entry() :)

Quoted 54 lines · Lance Yang

Quoted 51 lines · David Hildenbrand (Arm)

Quoted 12 lines · Lance Yang

Quoted 8 lines · David Hildenbrand (Arm)

Quoted 6 lines

     return __rste_to_pte(pte_val(*ptep));
}

So plain ptep_get() can feed raw huge-entry bits into pte_pfn(), and the derived subpage can be wrong.

Good question which impact that might have in practice?

Quoted 8 lines

The subpage check can warn, but we still pass that subpage to make_hwpoison_entry(). So the hwpoison marker can end up with the wrong PFN?

+    subpage = folio_page(folio, pte_pfn(pteval) - folio_pfn(folio)); +    VM_WARN_ON(folio_page(folio, 0) != subpage);
[...]
+    pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));

My s390x page table knowledge is a bit rusty.

IIUC, it would be a problem if some PTE bits in segment/region entries (pmd/pud/ ...) would pass the

     pte_pfn(x) -> (pte_val(x) >> PAGE_SHIFT)

check. I don't think this applies, because

While
    #define _SEGMENT_ENTRY_ORIGIN_LARGE ~0xfffffUL

We also have

    #define _SEGMENT_ENTRY_ORIGIN ~0x7ffUL

So these bits are not actually used.

What __rste_to_pte() primarily does is reshuffling present bits etc.

So using any other bits besides the PFN would be problematic I guess.

Am I wrong or isn't the present bit already at a different location? For prot-none hugetlb folios there might be a real issue, as the PTE present bit corresponds to the PMD/PUD read-permission bit.

Oh my :)

Quoted 5 lines

Quoted 2 lines

So yeah, we should probably fix that ahead of time unless I am missing something? Good that we separate that hugetlb crap out.

Yeah, looks like this was already there before the split. Should this be fixed separately?

Same bug is there in try_to_migrate_one(), check_pte(), remove_migration_pte() and prot_none_hugetlb_entry() :)

Quoted 6 lines · Dev Jain

Quoted 3 lines · Lance Yang

Quoted 6 lines

Quoted 2 lines

Yeah, looks like this was already there before the split. Should this be fixed separately?

Same bug is there in try_to_migrate_one(), check_pte(), remove_migration_pte() and prot_none_hugetlb_entry() :)

Lemme send a series for them. Main thing is identifying the fixes tag really.

Quoted 12 lines · Dev Jain

Quoted 9 lines · Dev Jain

Quoted 5 lines · Lance Yang

Yeah, looks like this was already there before the split. Should this be fixed separately?

Quoted 5 lines

Quoted 3 lines

Same bug is there in try_to_migrate_one(), check_pte(), remove_migration_pte() and prot_none_hugetlb_entry() :)

Lemme send a series for them. Main thing is identifying the fixes tag really.

Thanks!

Quoted 13 lines · David Hildenbrand (Arm)

Quoted 12 lines · Dev Jain

Quoted 9 lines · Dev Jain

Quoted 5 lines · Lance Yang

Yeah, looks like this was already there before the split. Should this be fixed separately?

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Same bug is there in try_to_migrate_one(), check_pte(), remove_migration_pte() and prot_none_hugetlb_entry() :)

Lemme send a series for them. Main thing is identifying the fixes tag really.

Thanks!

I am confused w.r.t backport. So currently we just have to do this:

(quoted diff) +10 -5

-               /*
-                * Handle PFN swap PTEs, such as device-exclusive ones, that
-                * actually map pages.
-                */
-               pteval = ptep_get(pvmw.pte);
+               address = pvmw.address;
+               if (folio_test_hugetlb(folio)) {
+                       pteval = huge_ptep_get(mm, address, pvmw.pte);
+               } else {
+                       /*
+                        * Handle PFN swap PTEs, such as device-exclusive ones,
+                        * that actually map pages.
+                        */
+                       pteval = ptep_get(pvmw.pte);
+               }

At commit c7ab0d2fdc84, try_to_unmap_one() was converted to use the pvmw API. The code was:

	while (page_vma_mapped_walk(&pvmw)) {
		subpage = page - page_to_pfn(page) + pte_pfn(*pvmw.pte);

Doing a plain dereference. Before this commit, the code was:

pte_t *__page_check_address(struct page *page, struct mm_struct *mm,
unsigned long address, spinlock_t **ptlp, int sync)

{
	pmd_t *pmd;
	pte_t *pte;
	spinlock_t *ptl;

	if (unlikely(PageHuge(page))) {
		/* when pud is not present, pte will be NULL */
		pte = huge_pte_offset(mm, address);
		if (!pte)
			return NULL;

		ptl = huge_pte_lockptr(page_hstate(page), mm, pte);
		goto check;
	}

	pmd = mm_find_pmd(mm, address);
	if (!pmd)
		return NULL;

	pte = pte_offset_map(pmd, address);
	/* Make a quick check before getting the lock */
	if (!sync && !pte_present(*pte)) {
		pte_unmap(pte);
		return NULL;
	}

	ptl = pte_lockptr(mm, pmd);
check:
	spin_lock(ptl);
	if (pte_present(*pte) && page_to_pfn(page) == pte_pfn(*pte)) {
		*ptlp = ptl;
		return pte;
	}
	pte_unmap_unlock(pte, ptl);
	return NULL;
}

This does pte_pfn(*pte), a plain dereference. I am not sure how back I need to go for the backport and do I need to post multiple patches for different stable versions.

And, it is crazy that the bug has stayed for this long and testing didn't find this. Perhaps no one bothered running hugetlb-read-hwpoison on PPC or s390. And for some reason those tests are in a category of "destructive" in run_vmtests.sh, requiring a command line option - and even that option was fixed just now in 3432cbb291aa!

Quoted 8 lines · Dev Jain

Simplify try_to_unmap_one by separating out the hugetlb parts into try_to_unmap_hugetlb_one.

To understand the correctness of the refactoring, the following points are noted:

1. try_to_unmap() is called for hugetlb folios only when they are hwpoisoned.

Ack

Quoted 2 lines

2. A hugetlb VMA cannot be mlocked.

Ack

Quoted 3 lines

3. The pvmw API sets pvmw.pte to the base of the hugetlb folio (pvmw.pmd is NULL).

Ack

Quoted 5 lines

4. We won't ever process a softleaf entry that encodes a hugetlb folio; hugetlb folios are never swapped out, migration entries will be skipped (PVMW_MIGRATION not passed) and device-exclusive does not work for hugetlb.

Ack, we should always find present entries.

Quoted 4 lines

5. uffd-wp bit is lost when converting pvmw.pte to hwpoison softleaf (therefore no need to call pte_install_uffd_wp_if_needed after clearing pvmw.pte)

Are you sure? And if so, can you elaborate why + document?

Quoted 6 lines

6. TTU_HWPOISON is always present; for it to not be present, either folio has to be in swapcache, or mapping_can_writeback() is true (see unmap_poisoned_folio), none of which is true for hugetlb folios.

Agreed. That's one very confusing bit in the existing code.

Quoted 3 lines

7. hugetlb uses separate counters from normal rss counters, therefore update_highwater_rss() need not be called.

Ack.

Quoted 7 lines

While at it:

- Change VM_BUG_* to VM_WARN_*.

- Do not declare variables which are only used once

Quoted 6 lines

- Assert that the subpage derived by the pvmw walk is exactly the head page. This is because try_to_unmap() does not remember the specific subpage which was hwpoisoned, and, since we cannot munmap/mremap across a hugetlb folio partially, the first pte mapping the hugetlb folio (in case of a contpte or contpmd mapped folio) cannot ever point to an intermediate page.

Might want to add a Suggested-by:, so people know directly whom to blame :)

I am very much in favor of decoupling both things cleanly, instead of hacking it together and make the code all big and confusing.

Quoted 14 lines

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

---
 mm/rmap.c | 203 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 121 insertions(+), 82 deletions(-)

mm/rmap.c +0 -0

index 430c91c8fe2ae..06ab1158d4cd1 100644
@@ -1978,6 +1978,121 @@ 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 1985

Quoted 5 lines

mm/rmap.c +5 -0

+static bool __try_to_unmap_hugetlb_one(struct folio *folio,
+		struct vm_area_struct *vma, struct page_vma_mapped_walk *pvmw,
+		struct mmu_notifier_range *range, enum ttu_flags flags)
+{
+	unsigned long hsz = huge_page_size(hstate_vma(vma));

Can be const.

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

Quoted 2 lines

mm/rmap.c +1 -0

+	unsigned long address = pvmw->address;

Can be const. But do we even need the temporary variable?

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

Quoted 3 lines

mm/rmap.c +2 -0

+	struct mm_struct *mm = vma->vm_mm;
+	struct page *subpage;

a) subpages do not exist. It's just a folio page.
b) subpages are irrelevant for hugetlb, just drop anything that deals with them.

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

Quoted 4 lines

mm/rmap.c +3 -0

+	bool ret = true;
+	pte_t pteval;
+

Worth adding a comment here, why we don't need a loop.

/* There is only a single mapping in a VMA. */

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

Quoted 5 lines

mm/rmap.c +4 -0

+	if (!page_vma_mapped_walk(pvmw))
+		return true;
+
+	pteval = ptep_get(pvmw->pte);

Should we now properly use huge_ptep_get()?

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

Quoted 3 lines

mm/rmap.c +2 -0

+	VM_WARN_ON(!pte_present(pteval));
+	subpage = folio_page(folio, pte_pfn(pteval) - folio_pfn(folio));

No need for subpages.

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

Quoted 2 lines

mm/rmap.c +1 -0

+	VM_WARN_ON(folio_page(folio, 0) != subpage);

	VM_WARN_ON(pte_pfn(pteval) != folio_pfn(folio));

I assume both can be VM_WARN_ON_ONCE()

Quoted 37 lines

mm/rmap.c +36 -0

+
+	/*
+	 * huge_pmd_unshare may unmap an entire PMD page. There is no way of
+	 * knowing exactly which PMDs may be cached for this mm, so we must
+	 * flush them all. start/end were already adjusted above to cover this
+	 * range.
+	 */
+	flush_cache_range(vma, range->start, range->end);
+
+	/*
+	 * To call huge_pmd_unshare, i_mmap_rwsem must be held in write mode.
+	 * Caller needs to explicitly do this outside rmap routines.
+	 *
+	 * We also must hold hugetlb vma_lock in write mode. Lock order dictates
+	 * acquiring vma_lock BEFORE i_mmap_rwsem. We can only try lock here and
+	 * fail if unsuccessful.
+	 */
+	if (!folio_test_anon(folio)) {
+		struct mmu_gather tlb;
+
+		VM_WARN_ON(!(flags & TTU_RMAP_LOCKED));
+		if (!hugetlb_vma_trylock_write(vma)) {
+			ret = false;
+			goto walk_done;
+		}
+
+		tlb_gather_mmu_vma(&tlb, vma);
+		if (huge_pmd_unshare(&tlb, vma, address, pvmw->pte)) {
+			hugetlb_vma_unlock_write(vma);
+			huge_pmd_unshare_flush(&tlb, vma);
+			tlb_finish_mmu(&tlb);
+			/*
+			 * The PMD table was unmapped, consequently unmapping
+			 * the folio.
+			 */
+			goto walk_done;

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

Quoted 6 lines

mm/rmap.c +6 -0

+		}
+		hugetlb_vma_unlock_write(vma);
+		tlb_finish_mmu(&tlb);
+	}
+	pteval = huge_ptep_clear_flush(vma, address, pvmw->pte);
+	if (pte_dirty(pteval))

Should we now use huge_pte_dirty() for consistency?

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

Quoted 4 lines

mm/rmap.c +3 -0

+		folio_mark_dirty(folio);
+
+	VM_WARN_ON(!(flags & TTU_HWPOISON));

This should be checked right at the very beginning of try_to_unmap_hugetlb_one().

Quoted 19 lines

mm/rmap.c +18 -0

+	pteval = swp_entry_to_pte(make_hwpoison_entry(subpage));
+	hugetlb_count_sub(folio_nr_pages(folio), mm);
+	set_huge_pte_at(mm, address, pvmw->pte, pteval, hsz);
+	hugetlb_remove_rmap(folio);
+	folio_put_refs(folio, 1);
+
+walk_done:
+	page_vma_mapped_walk_done(pvmw);
+	return ret;
+}
+
+static bool try_to_unmap_hugetlb_one(struct folio *folio,
+		struct vm_area_struct *vma, unsigned long address, void *arg)
+{
+	DEFINE_FOLIO_VMA_WALK(pvmw, folio, vma, address, 0);
+	struct mmu_notifier_range range;
+	enum ttu_flags flags = (enum ttu_flags)(long)arg;
+	bool ret;

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

Quoted 6 lines

mm/rmap.c +6 -0

+
+	/*
+	 * The try_to_unmap() is only passed a hugetlb folio in the case
+	 * where the hugetlb folio contains a poisoned page.
+	 */
+	VM_WARN_ON_FOLIO(!folio_contain_hwpoisoned_page(folio), folio);

As discussed, I'd prefer to just have folio_test_hwpoison() here.

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

Quoted 8 lines

mm/rmap.c +7 -0

+
+	/*
+	 * When racing against e.g. zap_pte_range() on another cpu,
+	 * in between its ptep_get_and_clear_full() and folio_remove_rmap_*(),
+	 * try_to_unmap() may return before folio_mapped() has become false,
+	 * if page table locking is skipped: use TTU_SYNC to wait for that.
+	 */

The comment should be updated to mention hugetlb primitives ... but

... it's a good question whether that is required *at all* ...

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

Quoted 2 lines

mm/rmap.c +2 -0

+	if (flags & TTU_SYNC)
+		pvmw.flags = PVMW_SYNC;

... and in fact, looking at page_vma_mapped_walk(), PVMW_SYNC is irrelevant.

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

Quoted 5 lines

mm/rmap.c +4 -0

+
+	/*
+	 * For hugetlb, it could be much worse than THP if we need pud
+	 * invalidation in the case of pmd sharing.

I don't understand that comment. Is it really helpful?

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

Quoted 2 lines

mm/rmap.c +2 -0

+	 *
+	 * Note that the folio can not be freed in this function as call of

"cannot" ?

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

Quoted 2 lines

mm/rmap.c +2 -0

+	 * try_to_unmap() must hold a reference on the folio.
+	 */

Do we really need this comment entirely? We don't even look at the folio after unmapping it.

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

Quoted 8 lines

mm/rmap.c +7 -0

+	range.end = vma_address_end(&pvmw);
+	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
+				address, range.end);
+	adjust_range_if_pmd_sharing_possible(vma, &range.start, &range.end);
+	mmu_notifier_invalidate_range_start(&range);
+	ret = __try_to_unmap_hugetlb_one(folio, vma, &pvmw, &range,
+					 flags);

Is it really worth it moving that stuff to a separate function?

Quoted 6 lines

mm/rmap.c +4 -0

+	mmu_notifier_invalidate_range_end(&range);
+	return ret;
+}
+

Quoted 36 lines · David Hildenbrand (Arm)

Quoted 8 lines · Dev Jain

Simplify try_to_unmap_one by separating out the hugetlb parts into try_to_unmap_hugetlb_one.

To understand the correctness of the refactoring, the following points are noted:

1. try_to_unmap() is called for hugetlb folios only when they are hwpoisoned.

Ack

Quoted 2 lines

2. A hugetlb VMA cannot be mlocked.

Ack

Quoted 3 lines

3. The pvmw API sets pvmw.pte to the base of the hugetlb folio (pvmw.pmd is NULL).

Ack

Quoted 5 lines

4. We won't ever process a softleaf entry that encodes a hugetlb folio; hugetlb folios are never swapped out, migration entries will be skipped (PVMW_MIGRATION not passed) and device-exclusive does not work for hugetlb.

Ack, we should always find present entries.

Quoted 4 lines

5. uffd-wp bit is lost when converting pvmw.pte to hwpoison softleaf (therefore no need to call pte_install_uffd_wp_if_needed after clearing pvmw.pte)

Quoted 1 line

Are you sure? And if so, can you elaborate why + document?

If you see the current code, we construct pteval from the subpage, losing any bit state of the previous entry. Also it doesn't make sense anyways to keep uffd-wp for an hwpoisoned entry.

Simplify try_to_unmap_one by separating out the hugetlb parts into try_to_unmap_hugetlb_one.

Quoted 2 lines · Dev Jain

Simplify try_to_unmap_one by separating out the hugetlb parts into try_to_unmap_hugetlb_one.

[snip]

mm/rmap.c static int folio_not_mapped(struct folio *folio) · line 2438

Quoted 10 lines

mm/rmap.c +2 -1

@@ -2393,7 +2431,8 @@ static int folio_not_mapped(struct folio *folio)
 void try_to_unmap(struct folio *folio, enum ttu_flags flags)
 {
 	struct rmap_walk_control rwc = {
-		.rmap_one = try_to_unmap_one,
+		.rmap_one = folio_test_hugetlb(folio) ?
+				try_to_unmap_hugetlb_one : try_to_unmap_one,
 		.arg = (void *)flags,
 		.done = folio_not_mapped,
 		.anon_lock = folio_lock_anon_vma_read,

Now that try_to_unmap_hugetlb_one() is split out, should we wrap it in #ifdef CONFIG_HUGETLB_PAGE and a stub function for !HUGETLB case?

I'm working on similar change for try_to_migrate() and had this thought.

I think either is fine, but wanted to check the preference.

Thanks,
Shivank

Quoted 8 lines · Shivank

Quoted 2 lines · Dev Jain

Simplify try_to_unmap_one by separating out the hugetlb parts into try_to_unmap_hugetlb_one.

[snip]

Quoted 13 lines

mm/rmap.c static int folio_not_mapped(struct folio *folio) · line 2438

Quoted 10 lines

mm/rmap.c +2 -1

@@ -2393,7 +2431,8 @@ static int folio_not_mapped(struct folio *folio)
 void try_to_unmap(struct folio *folio, enum ttu_flags flags)
 {
 	struct rmap_walk_control rwc = {
-		.rmap_one = try_to_unmap_one,
+		.rmap_one = folio_test_hugetlb(folio) ?
+				try_to_unmap_hugetlb_one : try_to_unmap_one,
 		.arg = (void *)flags,
 		.done = folio_not_mapped,
 		.anon_lock = folio_lock_anon_vma_read,

Now that try_to_unmap_hugetlb_one() is split out, should we wrap it in #ifdef CONFIG_HUGETLB_PAGE and a stub function for !HUGETLB case?

I'd rather not, if everything builds correctly?

Quoted 1 line

Quoted 1 line

I'm working on similar change for try_to_migrate() and had this thought.

Wait, are you doing the batching work for try_to_migrate()? I had that as an obvious follow up to this series so if you are already on it then great!

One other function to batch is page_vma_mkclean_one, but we do not already have the folio there, so have to be careful.

Quoted 29 lines · Dev Jain

Quoted 8 lines · Shivank

Quoted 2 lines · Dev Jain

Simplify try_to_unmap_one by separating out the hugetlb parts into try_to_unmap_hugetlb_one.

[snip]

Quoted 13 lines

mm/rmap.c static int folio_not_mapped(struct folio *folio) · line 2438

Quoted 10 lines

mm/rmap.c +2 -1

@@ -2393,7 +2431,8 @@ static int folio_not_mapped(struct folio *folio)
 void try_to_unmap(struct folio *folio, enum ttu_flags flags)
 {
 	struct rmap_walk_control rwc = {
-		.rmap_one = try_to_unmap_one,
+		.rmap_one = folio_test_hugetlb(folio) ?
+				try_to_unmap_hugetlb_one : try_to_unmap_one,
 		.arg = (void *)flags,
 		.done = folio_not_mapped,
 		.anon_lock = folio_lock_anon_vma_read,

Now that try_to_unmap_hugetlb_one() is split out, should we wrap it in #ifdef CONFIG_HUGETLB_PAGE and a stub function for !HUGETLB case?

I'd rather not, if everything builds correctly?

Quoted 1 line

Quoted 6 lines

Quoted 1 line

I'm working on similar change for try_to_migrate() and had this thought.

Wait, are you doing the batching work for try_to_migrate()? I had that as an obvious follow up to this series so if you are already on it then great!

Yes,
I'm seeing significant throughput boost when combining it with my batch copy offloading series [1] Once batch copying removes the copy bottleneck, the rmap walk becomes the dominant cost for PTE-mapped large folios.

My experiment with move_pages() on two-socket EPYC Zen 3 to move 1GB anon memory from node 0 -> node 1:

  folio   vanilla     DMA (dcbm, 16 chan)
  -----   --------    -------------------
  2MB     10.7 GB/s   64.3  (6.0x) -> no change for PMD mapped
  1MB      7.0 GB/s   14.3  (2.0x) copy offlaod
                      39.0  (5.6x) + rmap batching
  256KB    6.7 GB/s   13.0  (1.9x) copy offload
                      32.3  (4.8x) + rmap batching

[1] https://lore.kernel.org/linux-mm/[email protected]

Quoted 2 lines

One other function to batch is page_vma_mkclean_one, but we do not already have the folio there, so have to be careful.

Thanks,
Shivank

For lazyfree folio unmapping, after clearing the ptes we must abort the operation if the folio got dirtied or it has unexpected references.

Refactor this logic into a function which will return whether we need to abort or not.

If we abort, we restore the ptes and bail out of try_to_unmap_one. Otherwise adjust the rss stats of the mm and jump to a label.

Also rename that label from "discard" to "finish_unmap"; the former is appropriate in the lazyfree context, but the code following the label is executed for other successful unmap code paths too, so 'discard' does not sound correct for them.

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

---
 mm/rmap.c | 88 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 50 insertions(+), 38 deletions(-)

mm/rmap.c +0 -0

index 06ab1158d4cd1..12bbee57f20da 100644

Quoted 27 lines · Dev Jain

For lazyfree folio unmapping, after clearing the ptes we must abort the operation if the folio got dirtied or it has unexpected references.

Refactor this logic into a function which will return whether we need to abort or not.

If we abort, we restore the ptes and bail out of try_to_unmap_one. Otherwise adjust the rss stats of the mm and jump to a label.

Also rename that label from "discard" to "finish_unmap"; the former is appropriate in the lazyfree context, but the code following the label is executed for other successful unmap code paths too, so 'discard' does not sound correct for them.

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

---
 mm/rmap.c | 88 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 50 insertions(+), 38 deletions(-)

mm/rmap.c +0 -0

index 06ab1158d4cd1..12bbee57f20da 100644
@@ -2093,6 +2093,52 @@ static bool try_to_unmap_hugetlb_one(struct folio *folio,
 	return ret;
 }
 

mm/rmap.c static bool try_to_unmap_hugetlb_one(struct folio *folio, · line 2097

Quoted 2 lines

mm/rmap.c +2 -0

+static inline bool ttu_lazyfree_folio(struct vm_area_struct *vma,
+		struct folio *folio)

Nitty, but I wonder whether we want to return an int with 0 vs. -EBUSY instead.

Reading "!ttu_lazyfree_folio" naturally makes me assume that this would be a success path (which it is not).

Apart from that LGTM.

Quoted 33 lines · David Hildenbrand (Arm)

Quoted 24 lines · Dev Jain

For lazyfree folio unmapping, after clearing the ptes we must abort the operation if the folio got dirtied or it has unexpected references.

Refactor this logic into a function which will return whether we need to abort or not.

If we abort, we restore the ptes and bail out of try_to_unmap_one. Otherwise adjust the rss stats of the mm and jump to a label.

Also rename that label from "discard" to "finish_unmap"; the former is appropriate in the lazyfree context, but the code following the label is executed for other successful unmap code paths too, so 'discard' does not sound correct for them.

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

---
 mm/rmap.c | 88 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 50 insertions(+), 38 deletions(-)

mm/rmap.c +0 -0

index 06ab1158d4cd1..12bbee57f20da 100644

mm/rmap.c static bool try_to_unmap_hugetlb_one(struct folio *folio, · line 2097

Quoted 5 lines

mm/rmap.c +2 -0

@@ -2093,6 +2093,52 @@ static bool try_to_unmap_hugetlb_one(struct folio *folio,
 	return ret;
 }
 
+static inline bool ttu_lazyfree_folio(struct vm_area_struct *vma,
+		struct folio *folio)

Nitty, but I wonder whether we want to return an int with 0 vs. -EBUSY instead.

Quoted 2 lines

Reading "!ttu_lazyfree_folio" naturally makes me assume that this would be a success path (which it is not).

You have a point but if you don't have a strong opinion I'll prefer to keep as is. -EBUSY won't be consumed by the callers anyways : )

In preparation for the next patch, enable batch setting of uffd-wp ptes.

The code paths passing nr > 1 to zap_install_uffd_wp_if_needed() produce that nr through either folio_pte_batch or swap_pte_batch, therefore batching is correct:

1) all ptes belong to the same type of VMA (anonymous or non-anonymous, wp-armed or non-wp-armed)

2) all ptes being marked with uffd-wp or all being not marked (same is the case with the pte_swp_uffd_wp_any check)

3) uffd_supports_wp_marker() is independent of the function parameters

Note that we will have to use set_pte_at() in a loop instead of set_ptes() since the latter cannot handle present->non-present conversion for nr_pages > 1.

Rename the function to cond_install_uffd_wp_ptes, and convert the documentation to kerneldoc format.

Move the function to memory.c since this has grown too long to be kept in mm_inline.h, while retaining the inline hint.

Quoted 1 line · Dev Jain

In preparation for the next patch, enable batch setting of uffd-wp ptes.

Avoid talking about "next patch". Phrase it as "In preparation for batching...".

Quoted 8 lines

The code paths passing nr > 1 to zap_install_uffd_wp_if_needed() produce that nr through either folio_pte_batch or swap_pte_batch, therefore batching is correct:

1) all ptes belong to the same type of VMA (anonymous or non-anonymous, wp-armed or non-wp-armed)

Quoted 8 lines

2) all ptes being marked with uffd-wp or all being not marked (same is the case with the pte_swp_uffd_wp_any check)

3) uffd_supports_wp_marker() is independent of the function parameters

Note that we will have to use set_pte_at() in a loop instead of set_ptes() since the latter cannot handle present->non-present conversion for nr_pages > 1.

Below you have

mm/memory.c static inline bool zap_drop_markers(struct zap_details *details) · line 1632

Quoted 2 lines

mm/memory.c +2 -0

+	/* The current status of the pte should be "cleared" before calling */
+	WARN_ON_ONCE(!pte_none(ptep_get(ptep)));

Could we easily extend set_ptes() to allow for non-present -> non-present with nr > 1?

Quoted 7 lines

Rename the function to cond_install_uffd_wp_ptes, and convert the documentation to kerneldoc format.

Move the function to memory.c since this has grown too long to be kept in mm_inline.h, while retaining the inline hint.

Well, for functions in the header you really need "static inline", so it's not a hint and I would just drop that comment.

However, the move while modifying a larger function is rather suboptimal for reviewing (more like "spot the difference between these two pictures"). I'd suggest moving it in one patch, and thenb modifying it in a second (this) patch instead. You can perform the rename already while moving I think.

Nothing else jumped at me.

Quoted 22 lines · David Hildenbrand (Arm)

Quoted 1 line · Dev Jain

In preparation for the next patch, enable batch setting of uffd-wp ptes.

Avoid talking about "next patch". Phrase it as "In preparation for batching...".

Quoted 8 lines

The code paths passing nr > 1 to zap_install_uffd_wp_if_needed() produce that nr through either folio_pte_batch or swap_pte_batch, therefore batching is correct:

1) all ptes belong to the same type of VMA (anonymous or non-anonymous, wp-armed or non-wp-armed)

Quoted 8 lines

2) all ptes being marked with uffd-wp or all being not marked (same is the case with the pte_swp_uffd_wp_any check)

3) uffd_supports_wp_marker() is independent of the function parameters

Note that we will have to use set_pte_at() in a loop instead of set_ptes() since the latter cannot handle present->non-present conversion for nr_pages > 1.

Quoted 7 lines

Below you have

mm/memory.c static inline bool zap_drop_markers(struct zap_details *details) · line 1632

Quoted 2 lines

mm/memory.c +2 -0

+	/* The current status of the pte should be "cleared" before calling */
+	WARN_ON_ONCE(!pte_none(ptep_get(ptep)));

Could we easily extend set_ptes() to allow for non-present -> non-present with nr > 1?

We could have a set_nonpresent_ptes() in general, but then for some non-present configs like in this patch, we don't need to change the pfn, while in some configs like setting swp ptes in one go (like I do in later patches) we have an underlying pfn. So I don't see an obvious simplification with a common helper for non-present -> non-present for nr > 1.

Quoted 10 lines

Quoted 7 lines

Rename the function to cond_install_uffd_wp_ptes, and convert the documentation to kerneldoc format.

Move the function to memory.c since this has grown too long to be kept in mm_inline.h, while retaining the inline hint.

Well, for functions in the header you really need "static inline", so it's not a hint and I would just drop that comment.

Ok.

Quoted 5 lines

However, the move while modifying a larger function is rather suboptimal for reviewing (more like "spot the difference between these two pictures"). I'd suggest moving it in one patch, and thenb modifying it in a second (this) patch instead. You can perform the rename already while moving I think.

So I'll move the function, do the renaming of pte and pteval, do the kerneldoc conversion change in one patch. Then batch and rename in the next.

Quoted 4 lines

Nothing else jumped at me.

Quoted 13 lines · Dev Jain

Quoted 9 lines · David Hildenbrand (Arm)

Avoid talking about "next patch". Phrase it as "In preparation for batching...".

Below you have

Could we easily extend set_ptes() to allow for non-present -> non-present with nr > 1?

Quoted 5 lines

We could have a set_nonpresent_ptes() in general, but then for some non-present configs like in this patch, we don't need to change the pfn, while in some configs like setting swp ptes in one go (like I do in later patches) we have an underlying pfn. So I don't see an obvious simplification with a common helper for non-present -> non-present for nr > 1.

I think a "non-present" entry always means a softleaf? In that case, we can simply branch on softleaf_has_pfn().

Folios have pages, not subpages. Rename 'subpage' parameters to 'page'.

Add folio_dup_swap_pages 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: the caller invokes folio_dup_swap_pages() if it wants to operate on a range of pages in the folio (i.e nr_pages may be anything between 1 till folio_nr_pages()), and invokes folio_dup_swap() if it wants to operate on the entire folio.

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

---

Add folio_dup_swap_pages 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: the caller invokes folio_dup_swap_pages() if it wants to operate on a range of pages in the folio (i.e nr_pages may be anything between 1 till folio_nr_pages()), and invokes folio_dup_swap() if it wants to operate on the entire folio.

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

---
 mm/rmap.c     |  2 +-
 mm/shmem.c    |  2 +-
 mm/swap.h     | 19 +++++++++++++++++--
 mm/swapfile.c | 14 ++++++--------
 4 files changed, 25 insertions(+), 12 deletions(-)

mm/rmap.c +1 -1

index ead031361bc61..3065d384cadf8 100644
@@ -2331,7 +2331,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 				goto finish_unmap;
 			}
 
-			if (folio_dup_swap(folio, subpage) < 0) {
+			if (folio_dup_swap_pages(folio, subpage, 1) < 0) {
 				set_pte_at(mm, address, pvmw.pte, pteval);
 				goto walk_abort;
 			}

mm/shmem.c +1 -1

index bab3529af23c5..5e4f521399847 100644
@@ -1698,7 +1698,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);
 		shmem_delete_from_page_cache(folio, swp_to_radix_entry(folio->swap));
 
 		BUG_ON(folio_mapped(folio));

mm/swap.h +0 -0

index b57547c25ffc9..36c69b2788c77 100644

Add folio_put_swap_pages 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: the caller invokes folio_put_swap_pages() if it wants to operate on a range of pages in the folio (i.e nr_pages may be anything between 1 till folio_nr_pages()), and invokes folio_put_swap() if it

wants to operate on the entire folio.

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

---

mm/swap.h +0 -0

 /* For internal use */
 extern void __swap_cluster_free_entries(struct swap_info_struct *si,

@@ -397,7 +398,8 @@ static inline int folio_dup_swap_pages(struct folio *folio, struct page *page,
 	return -EINVAL;
 }
 

Quoted 4 lines · Dev Jain

Add folio_put_swap_pages 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.

Quoted 6 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: the caller invokes folio_put_swap_pages() if it wants to operate on a range of pages in the folio (i.e nr_pages may be anything between 1 till folio_nr_pages()), and invokes folio_put_swap() if it

"between 1 and folio_nr_pages()" ?

Quoted 5 lines

wants to operate on the entire folio.

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

---

[...]

mm/swap.h extern int swap_retry_table_alloc(swp_entry_t entry, gfp_t gfp); · line 212

Quoted 8 lines

+++ b/mm/swap.h

mm/swap.h +2 -1

@@ -208,7 +208,8 @@ extern int swap_retry_table_alloc(swp_entry_t entry, gfp_t gfp);
 int folio_alloc_swap(struct folio *folio);
 int folio_dup_swap_pages(struct folio *folio, struct page *page,
 			 unsigned long nr_pages);
-void folio_put_swap(struct folio *folio, struct page *page);
+void folio_put_swap_pages(struct folio *folio, struct page *page,
+			  unsigned long nr_pages);

Nit \tab\tab (same below)

Quoted 7 lines

mm/swap.h +0 -0

 /* For internal use */
 extern void __swap_cluster_free_entries(struct swap_info_struct *si,

@@ -397,7 +398,8 @@ static inline int folio_dup_swap_pages(struct folio *folio, struct page *page,
 	return -EINVAL;
 }
 

[...]

Quoted 1 line

mm/swapfile.c static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd, · line 2504

Quoted 12 lines

mm/swapfile.c +2 -1

 	swap_put_entries_cluster(si, swp_offset(entry), nr_pages, false);
 }

@@ -2499,7 +2497,8 @@ 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_pages(swapcache,
+			     folio_file_page(swapcache, swp_offset(entry)), 1);
 out:
 	if (pte)
 		pte_unmap_unlock(pte, ptl);

We could consider having a simple folio_put_swap_page() helper. But maybe most of these will be converted to batching in the long run either way.

Acked-by David Hildenbrand (Arm) <[email protected]>

To enable 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. Now, clear the PageAnonExclusive bit on a batch of nr_pages. Refactor the function such that the clearing of the bit can be done at one place without duplication.

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.

While at it, convert nr_pages to unsigned long to future-proof from overflow in case P4D-huge mappings etc get supported down the road. I haven't made such a change in each function receiving nr_pages in try_to_unmap_one - perhaps this can be done incrementally.

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

---
 include/linux/rmap.h | 52 +++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 17 deletions(-)

include/linux/rmap.h +0 -0

index 8dc0871e5f001..64929490a7cfc 100644

include/linux/rmap.h +4 -3

 /**
- * folio_try_share_anon_rmap_pte - try marking an exclusive anonymous page
- *				   mapped by a PTE possibly shared to prepare
+ * folio_try_share_anon_rmap_ptes - try marking exclusive anonymous pages
+ *				   mapped by PTEs possibly shared to prepare
  *				   for KSM or temporary unmapping
  * @folio:	The folio to share a mapping of
- * @page:	The mapped exclusive page
+ * @page:	The first mapped exclusive page of the batch in the folio
+ * @nr_pages:	The number of pages to share in the folio (batch size)
  *
  * The caller needs to hold the page table lock and has to have the page table
  * entries cleared/invalidated.

include/linux/rmap.h +4 -0

  */
+static inline int folio_try_share_anon_rmap_ptes(struct folio *folio,
+		struct page *page, unsigned long nr_pages)
+{
+	return __folio_try_share_anon_rmap(folio, page, nr_pages, PGTABLE_LEVEL_PTE);

Quoted 29 lines · Dev Jain

To enable 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. Now, clear the PageAnonExclusive bit on a batch of nr_pages. Refactor the function such that the clearing of the bit can be done at one place without duplication.

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.

While at it, convert nr_pages to unsigned long to future-proof from overflow in case P4D-huge mappings etc get supported down the road. I haven't made such a change in each function receiving nr_pages in try_to_unmap_one - perhaps this can be done incrementally.

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

---
 include/linux/rmap.h | 52 +++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 17 deletions(-)

include/linux/rmap.h +0 -0

index 8dc0871e5f001..64929490a7cfc 100644

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

Quoted 8 lines

include/linux/rmap.h +3 -1

@@ -706,17 +706,18 @@ static inline int folio_try_dup_anon_rmap_pmd(struct folio *folio,
 }
 
 static __always_inline int __folio_try_share_anon_rmap(struct folio *folio,
-		struct page *page, int nr_pages, enum pgtable_level level)
+		struct page *page, unsigned long nr_pages, enum pgtable_level level)
 {
+	/* device private folios cannot get pinned via GUP. */
+	const bool pinnable = likely(!folio_is_device_private(folio));

We don't seem to have a lot of users of likely/unlikely when assigning variables, so I wonder if the compiler really uses that information then.

git grep " = " | grep "likely(" | grep ";$" | wc -l 32

Maybe clearer to just push the "likely" into the actual two condition below.

[...]

Quoted 13 lines

/**
- * folio_try_share_anon_rmap_pte - try marking an exclusive anonymous page
- * mapped by a PTE possibly shared to prepare
+ * folio_try_share_anon_rmap_ptes - try marking exclusive anonymous pages
+ * mapped by PTEs possibly shared to prepare
* for KSM or temporary unmapping
* @folio: The folio to share a mapping of
- * @page: The mapped exclusive page
+ * @page: The first mapped exclusive page of the batch in the folio
+ * @nr_pages: The number of pages to share in the folio (batch size)
*
* The caller needs to hold the page table lock and has to have the page table
* entries cleared/invalidated.

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

Quoted 6 lines

include/linux/rmap.h +2 -0

@@ -797,11 +807,19 @@ static __always_inline int __folio_try_share_anon_rmap(struct folio *folio,
  *
  * Returns 0 if marking the mapped page possibly shared succeeded. Returns
  * -EBUSY otherwise.
+ *
+ * The caller needs to hold the page table lock.

Isn't that stated further above?

Quoted 6 lines

include/linux/rmap.h +4 -0

  */
+static inline int folio_try_share_anon_rmap_ptes(struct folio *folio,
+		struct page *page, unsigned long nr_pages)
+{
+	return __folio_try_share_anon_rmap(folio, page, nr_pages, PGTABLE_LEVEL_PTE);

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

Quoted 10 lines

include/linux/rmap.h +3 -1

+}
+
 static inline int folio_try_share_anon_rmap_pte(struct folio *folio,
 		struct page *page)
 {
-	return __folio_try_share_anon_rmap(folio, page, 1, PGTABLE_LEVEL_PTE);
+	return folio_try_share_anon_rmap_ptes(folio, page, 1);
 }
 
 /**

In general, LGTM.

Refactor anonymous folio unmap to ttu_anon_folio.

No functional change intended.

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

---
 mm/rmap.c | 116 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 68 insertions(+), 48 deletions(-)

mm/rmap.c +19 -0

index c0e385882f562..b1639bad8e27f 100644
@@ -2136,6 +2136,70 @@ static inline bool ttu_lazyfree_folio(struct vm_area_struct *vma,
 	return true;
 }
 
+static inline void set_swp_pte_at(struct mm_struct *mm, unsigned long address,
+		pte_t *ptep, swp_entry_t entry, pte_t pteval, bool anon_exclusive)
+{
+	pte_t swp_pte = swp_entry_to_pte(entry);
+
+	if (anon_exclusive)
+		swp_pte = pte_swp_mkexclusive(swp_pte);
+
+	if (likely(pte_present(pteval))) {
+		if (pte_soft_dirty(pteval))
+			swp_pte = pte_swp_mksoft_dirty(swp_pte);
+		if (pte_uffd_wp(pteval))
+			swp_pte = pte_swp_mkuffd_wp(swp_pte);
+	} else {
+		/* Device-exclusive entry */
+		if (pte_swp_soft_dirty(pteval))
+			swp_pte = pte_swp_mksoft_dirty(swp_pte);
+		if (pte_swp_uffd_wp(pteval))
+			swp_pte = pte_swp_mkuffd_wp(swp_pte);

Quoted 1 line · Dev Jain

Refactor anonymous folio unmap to ttu_anon_folio.

Given that the !folio_test_swapbacked() case remains in the caller, should we make it clearer and call it

ttu_anon_swapbacked_folio()

Or alternatively, factor complete anon folio handling out (incl. lazyfree handling)?

Quoted 40 lines

No functional change intended.

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

---
 mm/rmap.c | 116 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 68 insertions(+), 48 deletions(-)

mm/rmap.c +23 -0

index c0e385882f562..b1639bad8e27f 100644
@@ -2136,6 +2136,70 @@ static inline bool ttu_lazyfree_folio(struct vm_area_struct *vma,
 	return true;
 }
 
+static inline void set_swp_pte_at(struct mm_struct *mm, unsigned long address,
+		pte_t *ptep, swp_entry_t entry, pte_t pteval, bool anon_exclusive)
+{
+	pte_t swp_pte = swp_entry_to_pte(entry);
+
+	if (anon_exclusive)
+		swp_pte = pte_swp_mkexclusive(swp_pte);
+
+	if (likely(pte_present(pteval))) {
+		if (pte_soft_dirty(pteval))
+			swp_pte = pte_swp_mksoft_dirty(swp_pte);
+		if (pte_uffd_wp(pteval))
+			swp_pte = pte_swp_mkuffd_wp(swp_pte);
+	} else {
+		/* Device-exclusive entry */
+		if (pte_swp_soft_dirty(pteval))
+			swp_pte = pte_swp_mksoft_dirty(swp_pte);
+		if (pte_swp_uffd_wp(pteval))
+			swp_pte = pte_swp_mkuffd_wp(swp_pte);
+	}
+
+	set_pte_at(mm, address, ptep, swp_pte);
+}

mm/rmap.c static inline bool ttu_lazyfree_folio(struct vm_area_struct *vma, · line 2167

Quoted 6 lines

mm/rmap.c +6 -0

+
+static inline bool ttu_anon_folio(struct vm_area_struct *vma, struct folio *folio,
+		struct page *subpage, unsigned long address, pte_t *ptep,
+		pte_t pteval)
+{
+	bool anon_exclusive = folio_test_anon(folio) && PageAnonExclusive(subpage);

"subpage" ;)

Can be const.

[...]

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

Quoted 3 lines

mm/rmap.c +0 -0

 		} else {
 			/*
 			 * This is a locked file-backed folio,

Happy to see that function shrink a bit ...