[PATCH v4 0/3] Reclaim lazyfree THP without splitting

[PATCH v4 0/3] Reclaim lazyfree THP without splitting

[Lance Yang]

Hi all,

This series adds support for reclaiming PMD-mapped THP marked as lazyfree
without needing to first split the large folio via split_huge_pmd_address().

When the user no longer requires the pages, they would use madvise(MADV_FREE)
to mark the pages as lazy free. Subsequently, they typically would not re-write
to that memory again.

During memory reclaim, if we detect that the large folio and its PMD are both
still marked as clean and there are no unexpected references(such as GUP), so we
can just discard the memory lazily, improving the efficiency of memory
reclamation in this case.

Performance Testing
===================

On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
mem_cgroup_force_empty() results in the following runtimes in seconds
(shorter is better):

--------------------------------------------
|     Old       |      New       |  Change  |
--------------------------------------------
|   0.683426    |    0.049197    |  -92.80% |
--------------------------------------------

---

Changes since v3 [3]
====================
 - mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop
    - Resolve compilation errors by handling the case where
      CONFIG_PGTABLE_HAS_HUGE_LEAVES is undefined (thanks to SeongJae Park)
 - mm/vmscan: avoid split lazyfree THP during shrink_folio_list()
    - Remove the unnecessary conditional compilation directives
      (thanks to Barry Song)
    - Resolve compilation errors due to undefined references to
      unmap_huge_pmd_locked and split_huge_pmd_locked (thanks to Barry)

Changes since v2 [2]
====================
 - Update the changelog (thanks to David Hildenbrand)
 - Support try_to_unmap_one() to unmap PMD-mapped folios
   (thanks a lot to David Hildenbrand and Zi Yan)

Changes since v1 [1]
====================
 - Update the changelog
 - Follow the exact same logic as in try_to_unmap_one() (per David Hildenbrand)
 - Remove the extra code from rmap.c (per Matthew Wilcox)

[1] https://lore.kernel.org/linux-mm/20240417141111.77855-1-ioworker0@gmail.com
[2] https://lore.kernel.org/linux-mm/20240422055213.60231-1-ioworker0@gmail.com
[3] https://lore.kernel.org/linux-mm/20240429132308.38794-1-ioworker0@gmail.com

Lance Yang (3):
  mm/rmap: remove duplicated exit code in pagewalk loop
  mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop
  mm/vmscan: avoid split lazyfree THP during shrink_folio_list()

 include/linux/huge_mm.h |  29 ++++++++++
 mm/huge_memory.c        | 115 +++++++++++++++++++++++++++++++++-------
 mm/rmap.c               |  67 ++++++++++++-----------
 3 files changed, 160 insertions(+), 51 deletions(-)

-- 
2.33.1

[PATCH v4 1/3] mm/rmap: remove duplicated exit code in pagewalk loop

[Lance Yang]

Introduce the labels walk_done and walk_done_err as exit points to
eliminate duplicated exit code in the pagewalk loop.

Signed-off-by: Lance Yang <ioworker0@gmail.com>
---
 mm/rmap.c | 40 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 7faa60bc3e4d..7e2575d669a9 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1675,9 +1675,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 			/* Restore the mlock which got missed */
 			if (!folio_test_large(folio))
 				mlock_vma_folio(folio, vma);
-			page_vma_mapped_walk_done(&pvmw);
-			ret = false;
-			break;
+			goto walk_done_err;
 		}
 
 		pfn = pte_pfn(ptep_get(pvmw.pte));
@@ -1715,11 +1713,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 			 */
 			if (!anon) {
 				VM_BUG_ON(!(flags & TTU_RMAP_LOCKED));
-				if (!hugetlb_vma_trylock_write(vma)) {
-					page_vma_mapped_walk_done(&pvmw);
-					ret = false;
-					break;
-				}
+				if (!hugetlb_vma_trylock_write(vma))
+					goto walk_done_err;
 				if (huge_pmd_unshare(mm, vma, address, pvmw.pte)) {
 					hugetlb_vma_unlock_write(vma);
 					flush_tlb_range(vma,
@@ -1734,8 +1729,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 					 * actual page and drop map count
 					 * to zero.
 					 */
-					page_vma_mapped_walk_done(&pvmw);
-					break;
+					goto walk_done;
 				}
 				hugetlb_vma_unlock_write(vma);
 			}
@@ -1807,9 +1801,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 			if (unlikely(folio_test_swapbacked(folio) !=
 					folio_test_swapcache(folio))) {
 				WARN_ON_ONCE(1);
-				ret = false;
-				page_vma_mapped_walk_done(&pvmw);
-				break;
+				goto walk_done_err;
 			}
 
 			/* MADV_FREE page check */
@@ -1848,23 +1840,17 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 				 */
 				set_pte_at(mm, address, pvmw.pte, pteval);
 				folio_set_swapbacked(folio);
-				ret = false;
-				page_vma_mapped_walk_done(&pvmw);
-				break;
+				goto walk_done_err;
 			}
 
 			if (swap_duplicate(entry) < 0) {
 				set_pte_at(mm, address, pvmw.pte, pteval);
-				ret = false;
-				page_vma_mapped_walk_done(&pvmw);
-				break;
+				goto walk_done_err;
 			}
 			if (arch_unmap_one(mm, vma, address, pteval) < 0) {
 				swap_free(entry);
 				set_pte_at(mm, address, pvmw.pte, pteval);
-				ret = false;
-				page_vma_mapped_walk_done(&pvmw);
-				break;
+				goto walk_done_err;
 			}
 
 			/* See folio_try_share_anon_rmap(): clear PTE first. */
@@ -1872,9 +1858,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 			    folio_try_share_anon_rmap_pte(folio, subpage)) {
 				swap_free(entry);
 				set_pte_at(mm, address, pvmw.pte, pteval);
-				ret = false;
-				page_vma_mapped_walk_done(&pvmw);
-				break;
+				goto walk_done_err;
 			}
 			if (list_empty(&mm->mmlist)) {
 				spin_lock(&mmlist_lock);
@@ -1914,6 +1898,12 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 		if (vma->vm_flags & VM_LOCKED)
 			mlock_drain_local();
 		folio_put(folio);
+		continue;
+walk_done_err:
+		ret = false;
+walk_done:
+		page_vma_mapped_walk_done(&pvmw);
+		break;
 	}
 
 	mmu_notifier_invalidate_range_end(&range);
-- 
2.33.1

Re: [PATCH v4 1/3] mm/rmap: remove duplicated exit code in pagewalk loop

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 406 bytes --]

On 1 May 2024, at 0:26, Lance Yang wrote:

> Introduce the labels walk_done and walk_done_err as exit points to
> eliminate duplicated exit code in the pagewalk loop.
>
> Signed-off-by: Lance Yang <ioworker0@gmail.com>
> ---
>  mm/rmap.c | 40 +++++++++++++++-------------------------
>  1 file changed, 15 insertions(+), 25 deletions(-)

LGTM. Reviewed-by: Zi Yan <ziy@nvidia.com>
--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH v4 1/3] mm/rmap: remove duplicated exit code in pagewalk loop

[Lance Yang]

On Tue, May 7, 2024 at 10:51 PM Zi Yan <ziy@nvidia.com> wrote:
>
> On 1 May 2024, at 0:26, Lance Yang wrote:
>
> > Introduce the labels walk_done and walk_done_err as exit points to
> > eliminate duplicated exit code in the pagewalk loop.
> >
> > Signed-off-by: Lance Yang <ioworker0@gmail.com>
> > ---
> >  mm/rmap.c | 40 +++++++++++++++-------------------------
> >  1 file changed, 15 insertions(+), 25 deletions(-)
>

Hey Zi,

> LGTM. Reviewed-by: Zi Yan <ziy@nvidia.com>

Thanks for taking time to review!
Lance

> --
> Best Regards,
> Yan, Zi

[PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Lance Yang]

In preparation for supporting try_to_unmap_one() to unmap PMD-mapped
folios, start the pagewalk first, then call split_huge_pmd_address()
to split the folio.

Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Lance Yang <ioworker0@gmail.com>
---
 include/linux/huge_mm.h | 20 ++++++++++++++++++++
 mm/huge_memory.c        | 42 +++++++++++++++++++++--------------------
 mm/rmap.c               | 24 +++++++++++++++++------
 3 files changed, 60 insertions(+), 26 deletions(-)

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index c8d3ec116e29..38c4b5537715 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void)
 	return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
 }
 
+void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
+			   pmd_t *pmd, bool freeze, struct folio *folio);
+
+static inline void align_huge_pmd_range(struct vm_area_struct *vma,
+					unsigned long *start,
+					unsigned long *end)
+{
+	*start = ALIGN(*start, HPAGE_PMD_SIZE);
+	*end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE);
+
+	VM_WARN_ON_ONCE(vma->vm_start > *start);
+	VM_WARN_ON_ONCE(vma->vm_end < *end);
+}
+
 #else /* CONFIG_TRANSPARENT_HUGEPAGE */
 
 static inline bool folio_test_pmd_mappable(struct folio *folio)
@@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 		unsigned long address, bool freeze, struct folio *folio) {}
 static inline void split_huge_pmd_address(struct vm_area_struct *vma,
 		unsigned long address, bool freeze, struct folio *folio) {}
+static inline void split_huge_pmd_locked(struct vm_area_struct *vma,
+					 unsigned long address, pmd_t *pmd,
+					 bool freeze, struct folio *folio) {}
+static inline void align_huge_pmd_range(struct vm_area_struct *vma,
+					unsigned long *start,
+					unsigned long *end) {}
 
 #define split_huge_pud(__vma, __pmd, __address)	\
 	do { } while (0)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 8261b5669397..145505a1dd05 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 	pmd_populate(mm, pmd, pgtable);
 }
 
+void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
+			   pmd_t *pmd, bool freeze, struct folio *folio)
+{
+	VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio));
+	VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE));
+	VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
+	VM_BUG_ON(freeze && !folio);
+
+	/*
+	 * When the caller requests to set up a migration entry, we
+	 * require a folio to check the PMD against. Otherwise, there
+	 * is a risk of replacing the wrong folio.
+	 */
+	if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
+	    is_pmd_migration_entry(*pmd)) {
+		if (folio && folio != pmd_folio(*pmd))
+			return;
+		__split_huge_pmd_locked(vma, pmd, address, freeze);
+	}
+}
+
 void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 		unsigned long address, bool freeze, struct folio *folio)
 {
@@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 				(address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE);
 	mmu_notifier_invalidate_range_start(&range);
 	ptl = pmd_lock(vma->vm_mm, pmd);
-
-	/*
-	 * If caller asks to setup a migration entry, we need a folio to check
-	 * pmd against. Otherwise we can end up replacing wrong folio.
-	 */
-	VM_BUG_ON(freeze && !folio);
-	VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
-
-	if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
-	    is_pmd_migration_entry(*pmd)) {
-		/*
-		 * It's safe to call pmd_page when folio is set because it's
-		 * guaranteed that pmd is present.
-		 */
-		if (folio && folio != pmd_folio(*pmd))
-			goto out;
-		__split_huge_pmd_locked(vma, pmd, range.start, freeze);
-	}
-
-out:
+	split_huge_pmd_locked(vma, range.start, pmd, freeze, folio);
 	spin_unlock(ptl);
 	mmu_notifier_invalidate_range_end(&range);
 }
diff --git a/mm/rmap.c b/mm/rmap.c
index 7e2575d669a9..432601154583 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 	if (flags & TTU_SYNC)
 		pvmw.flags = PVMW_SYNC;
 
-	if (flags & TTU_SPLIT_HUGE_PMD)
-		split_huge_pmd_address(vma, address, false, folio);
-
 	/*
 	 * For THP, we have to assume the worse case ie pmd for invalidation.
 	 * For hugetlb, it could be much worse if we need to do pud
@@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 	range.end = vma_address_end(&pvmw);
 	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
 				address, range.end);
+	if (flags & TTU_SPLIT_HUGE_PMD)
+		align_huge_pmd_range(vma, &range.start, &range.end);
 	if (folio_test_hugetlb(folio)) {
 		/*
 		 * If sharing is possible, start and end will be adjusted
@@ -1664,9 +1663,6 @@ 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)) {
-		/* Unexpected PMD-mapped THP? */
-		VM_BUG_ON_FOLIO(!pvmw.pte, folio);
-
 		/*
 		 * If the folio is in an mlock()d vma, we must not swap it out.
 		 */
@@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 			goto walk_done_err;
 		}
 
+		if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
+			/*
+			 * We temporarily have to drop the PTL and start once
+			 * again from that now-PTE-mapped page table.
+			 */
+			split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
+					      folio);
+			pvmw.pmd = NULL;
+			spin_unlock(pvmw.ptl);
+			flags &= ~TTU_SPLIT_HUGE_PMD;
+			continue;
+		}
+
+		/* Unexpected PMD-mapped THP? */
+		VM_BUG_ON_FOLIO(!pvmw.pte, folio);
+
 		pfn = pte_pfn(ptep_get(pvmw.pte));
 		subpage = folio_page(folio, pfn - folio_pfn(folio));
 		address = pvmw.address;
-- 
2.33.1

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Baolin Wang]

On 2024/5/1 12:26, Lance Yang wrote:
> In preparation for supporting try_to_unmap_one() to unmap PMD-mapped
> folios, start the pagewalk first, then call split_huge_pmd_address()
> to split the folio.
> 
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Lance Yang <ioworker0@gmail.com>
> ---
>   include/linux/huge_mm.h | 20 ++++++++++++++++++++
>   mm/huge_memory.c        | 42 +++++++++++++++++++++--------------------
>   mm/rmap.c               | 24 +++++++++++++++++------
>   3 files changed, 60 insertions(+), 26 deletions(-)
> 
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index c8d3ec116e29..38c4b5537715 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void)
>   	return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
>   }
>   
> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> +			   pmd_t *pmd, bool freeze, struct folio *folio);
> +
> +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> +					unsigned long *start,
> +					unsigned long *end)
> +{
> +	*start = ALIGN(*start, HPAGE_PMD_SIZE);
> +	*end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE);
> +
> +	VM_WARN_ON_ONCE(vma->vm_start > *start);
> +	VM_WARN_ON_ONCE(vma->vm_end < *end);
> +}
> +
>   #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>   
>   static inline bool folio_test_pmd_mappable(struct folio *folio)
> @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>   		unsigned long address, bool freeze, struct folio *folio) {}
>   static inline void split_huge_pmd_address(struct vm_area_struct *vma,
>   		unsigned long address, bool freeze, struct folio *folio) {}
> +static inline void split_huge_pmd_locked(struct vm_area_struct *vma,
> +					 unsigned long address, pmd_t *pmd,
> +					 bool freeze, struct folio *folio) {}
> +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> +					unsigned long *start,
> +					unsigned long *end) {}
>   
>   #define split_huge_pud(__vma, __pmd, __address)	\
>   	do { } while (0)
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 8261b5669397..145505a1dd05 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>   	pmd_populate(mm, pmd, pgtable);
>   }
>   
> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> +			   pmd_t *pmd, bool freeze, struct folio *folio)
> +{
> +	VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio));
> +	VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE));
> +	VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
> +	VM_BUG_ON(freeze && !folio);
> +
> +	/*
> +	 * When the caller requests to set up a migration entry, we
> +	 * require a folio to check the PMD against. Otherwise, there
> +	 * is a risk of replacing the wrong folio.
> +	 */
> +	if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
> +	    is_pmd_migration_entry(*pmd)) {
> +		if (folio && folio != pmd_folio(*pmd))
> +			return;
> +		__split_huge_pmd_locked(vma, pmd, address, freeze);
> +	}
> +}
> +
>   void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>   		unsigned long address, bool freeze, struct folio *folio)
>   {
> @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>   				(address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE);
>   	mmu_notifier_invalidate_range_start(&range);
>   	ptl = pmd_lock(vma->vm_mm, pmd);
> -
> -	/*
> -	 * If caller asks to setup a migration entry, we need a folio to check
> -	 * pmd against. Otherwise we can end up replacing wrong folio.
> -	 */
> -	VM_BUG_ON(freeze && !folio);
> -	VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
> -
> -	if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
> -	    is_pmd_migration_entry(*pmd)) {
> -		/*
> -		 * It's safe to call pmd_page when folio is set because it's
> -		 * guaranteed that pmd is present.
> -		 */
> -		if (folio && folio != pmd_folio(*pmd))
> -			goto out;
> -		__split_huge_pmd_locked(vma, pmd, range.start, freeze);
> -	}
> -
> -out:
> +	split_huge_pmd_locked(vma, range.start, pmd, freeze, folio);
>   	spin_unlock(ptl);
>   	mmu_notifier_invalidate_range_end(&range);
>   }
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 7e2575d669a9..432601154583 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>   	if (flags & TTU_SYNC)
>   		pvmw.flags = PVMW_SYNC;
>   
> -	if (flags & TTU_SPLIT_HUGE_PMD)
> -		split_huge_pmd_address(vma, address, false, folio);
> -
>   	/*
>   	 * For THP, we have to assume the worse case ie pmd for invalidation.
>   	 * For hugetlb, it could be much worse if we need to do pud
> @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>   	range.end = vma_address_end(&pvmw);
>   	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
>   				address, range.end);
> +	if (flags & TTU_SPLIT_HUGE_PMD)
> +		align_huge_pmd_range(vma, &range.start, &range.end);

I am not sure why need this alignment?
(1) For a partially mapped THP, 'range.start' and 'range.end' can beyond 
the VMA limits. For a PMD mapped THP, I think the address is already THP 
size alignment returned from vma_address(&folio->page, vma).
(2) The range.end is not used.

>   	if (folio_test_hugetlb(folio)) {
>   		/*
>   		 * If sharing is possible, start and end will be adjusted
> @@ -1664,9 +1663,6 @@ 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)) {
> -		/* Unexpected PMD-mapped THP? */
> -		VM_BUG_ON_FOLIO(!pvmw.pte, folio);
> -
>   		/*
>   		 * If the folio is in an mlock()d vma, we must not swap it out.
>   		 */
> @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>   			goto walk_done_err;
>   		}
>   
> +		if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
> +			/*
> +			 * We temporarily have to drop the PTL and start once
> +			 * again from that now-PTE-mapped page table.
> +			 */
> +			split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
> +					      folio);
> +			pvmw.pmd = NULL;
> +			spin_unlock(pvmw.ptl);

IMO, you should also make the 'pvmw.ptl = NULL;' after unlocking as 
page_vma_mapped_walk() did, in case some corner case met.

> +			flags &= ~TTU_SPLIT_HUGE_PMD;
> +			continue;
> +		}
> +
> +		/* Unexpected PMD-mapped THP? */
> +		VM_BUG_ON_FOLIO(!pvmw.pte, folio);
> +
>   		pfn = pte_pfn(ptep_get(pvmw.pte));
>   		subpage = folio_page(folio, pfn - folio_pfn(folio));
>   		address = pvmw.address;

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Lance Yang]

Hey Baolin,

Thanks for taking time to review!

On Tue, May 7, 2024 at 11:40 AM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 2024/5/1 12:26, Lance Yang wrote:
> > In preparation for supporting try_to_unmap_one() to unmap PMD-mapped
> > folios, start the pagewalk first, then call split_huge_pmd_address()
> > to split the folio.
> >
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Lance Yang <ioworker0@gmail.com>
> > ---
> >   include/linux/huge_mm.h | 20 ++++++++++++++++++++
> >   mm/huge_memory.c        | 42 +++++++++++++++++++++--------------------
> >   mm/rmap.c               | 24 +++++++++++++++++------
> >   3 files changed, 60 insertions(+), 26 deletions(-)
> >
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index c8d3ec116e29..38c4b5537715 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void)
> >       return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
> >   }
> >
> > +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> > +                        pmd_t *pmd, bool freeze, struct folio *folio);
> > +
> > +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> > +                                     unsigned long *start,
> > +                                     unsigned long *end)
> > +{
> > +     *start = ALIGN(*start, HPAGE_PMD_SIZE);
> > +     *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE);
> > +
> > +     VM_WARN_ON_ONCE(vma->vm_start > *start);
> > +     VM_WARN_ON_ONCE(vma->vm_end < *end);
> > +}
> > +
> >   #else /* CONFIG_TRANSPARENT_HUGEPAGE */
> >
> >   static inline bool folio_test_pmd_mappable(struct folio *folio)
> > @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> >               unsigned long address, bool freeze, struct folio *folio) {}
> >   static inline void split_huge_pmd_address(struct vm_area_struct *vma,
> >               unsigned long address, bool freeze, struct folio *folio) {}
> > +static inline void split_huge_pmd_locked(struct vm_area_struct *vma,
> > +                                      unsigned long address, pmd_t *pmd,
> > +                                      bool freeze, struct folio *folio) {}
> > +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> > +                                     unsigned long *start,
> > +                                     unsigned long *end) {}
> >
> >   #define split_huge_pud(__vma, __pmd, __address)     \
> >       do { } while (0)
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 8261b5669397..145505a1dd05 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >       pmd_populate(mm, pmd, pgtable);
> >   }
> >
> > +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> > +                        pmd_t *pmd, bool freeze, struct folio *folio)
> > +{
> > +     VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio));
> > +     VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE));
> > +     VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
> > +     VM_BUG_ON(freeze && !folio);
> > +
> > +     /*
> > +      * When the caller requests to set up a migration entry, we
> > +      * require a folio to check the PMD against. Otherwise, there
> > +      * is a risk of replacing the wrong folio.
> > +      */
> > +     if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
> > +         is_pmd_migration_entry(*pmd)) {
> > +             if (folio && folio != pmd_folio(*pmd))
> > +                     return;
> > +             __split_huge_pmd_locked(vma, pmd, address, freeze);
> > +     }
> > +}
> > +
> >   void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> >               unsigned long address, bool freeze, struct folio *folio)
> >   {
> > @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> >                               (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE);
> >       mmu_notifier_invalidate_range_start(&range);
> >       ptl = pmd_lock(vma->vm_mm, pmd);
> > -
> > -     /*
> > -      * If caller asks to setup a migration entry, we need a folio to check
> > -      * pmd against. Otherwise we can end up replacing wrong folio.
> > -      */
> > -     VM_BUG_ON(freeze && !folio);
> > -     VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
> > -
> > -     if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
> > -         is_pmd_migration_entry(*pmd)) {
> > -             /*
> > -              * It's safe to call pmd_page when folio is set because it's
> > -              * guaranteed that pmd is present.
> > -              */
> > -             if (folio && folio != pmd_folio(*pmd))
> > -                     goto out;
> > -             __split_huge_pmd_locked(vma, pmd, range.start, freeze);
> > -     }
> > -
> > -out:
> > +     split_huge_pmd_locked(vma, range.start, pmd, freeze, folio);
> >       spin_unlock(ptl);
> >       mmu_notifier_invalidate_range_end(&range);
> >   }
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index 7e2575d669a9..432601154583 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >       if (flags & TTU_SYNC)
> >               pvmw.flags = PVMW_SYNC;
> >
> > -     if (flags & TTU_SPLIT_HUGE_PMD)
> > -             split_huge_pmd_address(vma, address, false, folio);
> > -
> >       /*
> >        * For THP, we have to assume the worse case ie pmd for invalidation.
> >        * For hugetlb, it could be much worse if we need to do pud
> > @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >       range.end = vma_address_end(&pvmw);
> >       mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
> >                               address, range.end);
> > +     if (flags & TTU_SPLIT_HUGE_PMD)
> > +             align_huge_pmd_range(vma, &range.start, &range.end);
>
> I am not sure why need this alignment?
> (1) For a partially mapped THP, 'range.start' and 'range.end' can beyond
> the VMA limits. For a PMD mapped THP, I think the address is already THP
> size alignment returned from vma_address(&folio->page, vma).
> (2) The range.end is not used.

Thanks for pointing that out!

I agree that this alignment is indeed redundant, and we should remove it.
Especially for a partially mapped THP, it could cause 'range.start' and
'range.end' beyond the VMA limits, which could lead us into trouble.

>
> >       if (folio_test_hugetlb(folio)) {
> >               /*
> >                * If sharing is possible, start and end will be adjusted
> > @@ -1664,9 +1663,6 @@ 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)) {
> > -             /* Unexpected PMD-mapped THP? */
> > -             VM_BUG_ON_FOLIO(!pvmw.pte, folio);
> > -
> >               /*
> >                * If the folio is in an mlock()d vma, we must not swap it out.
> >                */
> > @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >                       goto walk_done_err;
> >               }
> >
> > +             if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
> > +                     /*
> > +                      * We temporarily have to drop the PTL and start once
> > +                      * again from that now-PTE-mapped page table.
> > +                      */
> > +                     split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
> > +                                           folio);
> > +                     pvmw.pmd = NULL;
> > +                     spin_unlock(pvmw.ptl);
>
> IMO, you should also make the 'pvmw.ptl = NULL;' after unlocking as
> page_vma_mapped_walk() did, in case some corner case met.

Yep, I'll also set pvmw.ptl to NULL here if any corner cases arise.

Thanks again for the review!
Lance

>
> > +                     flags &= ~TTU_SPLIT_HUGE_PMD;
> > +                     continue;
> > +             }
> > +
> > +             /* Unexpected PMD-mapped THP? */
> > +             VM_BUG_ON_FOLIO(!pvmw.pte, folio);
> > +
> >               pfn = pte_pfn(ptep_get(pvmw.pte));
> >               subpage = folio_page(folio, pfn - folio_pfn(folio));
> >               address = pvmw.address;

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[David Hildenbrand]

>>>
>>> +             if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
>>> +                     /*
>>> +                      * We temporarily have to drop the PTL and start once
>>> +                      * again from that now-PTE-mapped page table.
>>> +                      */
>>> +                     split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
>>> +                                           folio);
>>> +                     pvmw.pmd = NULL;
>>> +                     spin_unlock(pvmw.ptl);
>>
>> IMO, you should also make the 'pvmw.ptl = NULL;' after unlocking as
>> page_vma_mapped_walk() did, in case some corner case met.
> 
> Yep, I'll also set pvmw.ptl to NULL here if any corner cases arise.
> 

This series already resides in mm-stable. I asked Andrew to remove it 
for now. If that doesn't work, we'll need fixup patches to address any 
review feedback.

-- 
Cheers,

David / dhildenb

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Lance Yang]

Hey David,

Thanks for reaching out!

On Tue, May 7, 2024 at 4:17 PM David Hildenbrand <david@redhat.com> wrote:
>
> >>>
> >>> +             if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
> >>> +                     /*
> >>> +                      * We temporarily have to drop the PTL and start once
> >>> +                      * again from that now-PTE-mapped page table.
> >>> +                      */
> >>> +                     split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
> >>> +                                           folio);
> >>> +                     pvmw.pmd = NULL;
> >>> +                     spin_unlock(pvmw.ptl);
> >>
> >> IMO, you should also make the 'pvmw.ptl = NULL;' after unlocking as
> >> page_vma_mapped_walk() did, in case some corner case met.
> >
> > Yep, I'll also set pvmw.ptl to NULL here if any corner cases arise.
> >
>
> This series already resides in mm-stable. I asked Andrew to remove it
> for now. If that doesn't work, we'll need fixup patches to address any
> review feedback.

I'll patiently wait Andrew's response, and then submit the next version or
fixup patches accordingly.

Thanks,
Lance

>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Andrew Morton]

On Tue, 7 May 2024 16:38:07 +0800 Lance Yang <ioworker0@gmail.com> wrote:

> > > Yep, I'll also set pvmw.ptl to NULL here if any corner cases arise.
> > >
> >
> > This series already resides in mm-stable. I asked Andrew to remove it
> > for now. If that doesn't work, we'll need fixup patches to address any
> > review feedback.
> 
> I'll patiently wait Andrew's response, and then submit the next version or
> fixup patches accordingly.

Well, which series are we talking about?  "mm/madvise: enhance
lazyfreeing with mTHP in madvise_free v10" or ""Reclaim lazyfree THP
without splitting v4" or both?

And how significant are the needed fixup patches?

And what is our confidence level after those fixups are in place?

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[David Hildenbrand]

On 07.05.24 19:22, Andrew Morton wrote:
> On Tue, 7 May 2024 16:38:07 +0800 Lance Yang <ioworker0@gmail.com> wrote:
> 
>>>> Yep, I'll also set pvmw.ptl to NULL here if any corner cases arise.
>>>>
>>>
>>> This series already resides in mm-stable. I asked Andrew to remove it
>>> for now. If that doesn't work, we'll need fixup patches to address any
>>> review feedback.
>>
>> I'll patiently wait Andrew's response, and then submit the next version or
>> fixup patches accordingly.
> 
> Well, which series are we talking about?  "mm/madvise: enhance
> lazyfreeing with mTHP in madvise_free v10" or ""Reclaim lazyfree THP
> without splitting v4" or both?

See my other mail, "mm/madvise: enhance lazyfreeing with mTHP in 
madvise_free v10" is all acked/reviewed and good to go.

> 
> And how significant are the needed fixup patches?
> 
> And what is our confidence level after those fixups are in place?

I'm afraid I won't have time to review this series this/next week, so I 
cannot tell. I already assumed this would not be 6.10 material.

-- 
Cheers,

David / dhildenb

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Andrew Morton]

On Tue, 7 May 2024 19:33:05 +0200 David Hildenbrand <david@redhat.com> wrote:

> > Well, which series are we talking about?  "mm/madvise: enhance
> > lazyfreeing with mTHP in madvise_free v10" or ""Reclaim lazyfree THP
> > without splitting v4" or both?
> 
> See my other mail, "mm/madvise: enhance lazyfreeing with mTHP in 
> madvise_free v10" is all acked/reviewed and good to go.
> 
> > 
> > And how significant are the needed fixup patches?
> > 
> > And what is our confidence level after those fixups are in place?
> 
> I'm afraid I won't have time to review this series this/next week, so I 
> cannot tell. I already assumed this would not be 6.10 material.

OK, I've dropped the series "Reclaim lazyfree THP without splitting",
v4.  Let's revisit in the next cycle.

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[David Hildenbrand]

On 07.05.24 19:38, Andrew Morton wrote:
> On Tue, 7 May 2024 19:33:05 +0200 David Hildenbrand <david@redhat.com> wrote:
> 
>>> Well, which series are we talking about?  "mm/madvise: enhance
>>> lazyfreeing with mTHP in madvise_free v10" or ""Reclaim lazyfree THP
>>> without splitting v4" or both?
>>
>> See my other mail, "mm/madvise: enhance lazyfreeing with mTHP in
>> madvise_free v10" is all acked/reviewed and good to go.
>>
>>>
>>> And how significant are the needed fixup patches?
>>>
>>> And what is our confidence level after those fixups are in place?
>>
>> I'm afraid I won't have time to review this series this/next week, so I
>> cannot tell. I already assumed this would not be 6.10 material.
> 
> OK, I've dropped the series "Reclaim lazyfree THP without splitting",
> v4.  Let's revisit in the next cycle.

Thanks, should be more than ready by then :)

-- 
Cheers,

David / dhildenb

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 6767 bytes --]

On 1 May 2024, at 0:26, Lance Yang wrote:

> In preparation for supporting try_to_unmap_one() to unmap PMD-mapped
> folios, start the pagewalk first, then call split_huge_pmd_address()
> to split the folio.
>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Lance Yang <ioworker0@gmail.com>
> ---
>  include/linux/huge_mm.h | 20 ++++++++++++++++++++
>  mm/huge_memory.c        | 42 +++++++++++++++++++++--------------------
>  mm/rmap.c               | 24 +++++++++++++++++------
>  3 files changed, 60 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index c8d3ec116e29..38c4b5537715 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void)
>  	return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
>  }
>
> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> +			   pmd_t *pmd, bool freeze, struct folio *folio);
> +
> +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> +					unsigned long *start,
> +					unsigned long *end)
> +{
> +	*start = ALIGN(*start, HPAGE_PMD_SIZE);
> +	*end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE);
> +
> +	VM_WARN_ON_ONCE(vma->vm_start > *start);
> +	VM_WARN_ON_ONCE(vma->vm_end < *end);
> +}
> +
>  #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>
>  static inline bool folio_test_pmd_mappable(struct folio *folio)
> @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>  		unsigned long address, bool freeze, struct folio *folio) {}
>  static inline void split_huge_pmd_address(struct vm_area_struct *vma,
>  		unsigned long address, bool freeze, struct folio *folio) {}
> +static inline void split_huge_pmd_locked(struct vm_area_struct *vma,
> +					 unsigned long address, pmd_t *pmd,
> +					 bool freeze, struct folio *folio) {}
> +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> +					unsigned long *start,
> +					unsigned long *end) {}
>
>  #define split_huge_pud(__vma, __pmd, __address)	\
>  	do { } while (0)
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 8261b5669397..145505a1dd05 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  	pmd_populate(mm, pmd, pgtable);
>  }
>
> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> +			   pmd_t *pmd, bool freeze, struct folio *folio)
> +{
> +	VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio));
> +	VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE));
> +	VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
> +	VM_BUG_ON(freeze && !folio);
> +
> +	/*
> +	 * When the caller requests to set up a migration entry, we
> +	 * require a folio to check the PMD against. Otherwise, there
> +	 * is a risk of replacing the wrong folio.
> +	 */
> +	if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
> +	    is_pmd_migration_entry(*pmd)) {
> +		if (folio && folio != pmd_folio(*pmd))
> +			return;
> +		__split_huge_pmd_locked(vma, pmd, address, freeze);
> +	}
> +}
> +
>  void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>  		unsigned long address, bool freeze, struct folio *folio)
>  {
> @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>  				(address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE);
>  	mmu_notifier_invalidate_range_start(&range);
>  	ptl = pmd_lock(vma->vm_mm, pmd);
> -
> -	/*
> -	 * If caller asks to setup a migration entry, we need a folio to check
> -	 * pmd against. Otherwise we can end up replacing wrong folio.
> -	 */
> -	VM_BUG_ON(freeze && !folio);
> -	VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
> -
> -	if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
> -	    is_pmd_migration_entry(*pmd)) {
> -		/*
> -		 * It's safe to call pmd_page when folio is set because it's
> -		 * guaranteed that pmd is present.
> -		 */
> -		if (folio && folio != pmd_folio(*pmd))
> -			goto out;
> -		__split_huge_pmd_locked(vma, pmd, range.start, freeze);
> -	}
> -
> -out:
> +	split_huge_pmd_locked(vma, range.start, pmd, freeze, folio);
>  	spin_unlock(ptl);
>  	mmu_notifier_invalidate_range_end(&range);
>  }
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 7e2575d669a9..432601154583 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>  	if (flags & TTU_SYNC)
>  		pvmw.flags = PVMW_SYNC;
>
> -	if (flags & TTU_SPLIT_HUGE_PMD)
> -		split_huge_pmd_address(vma, address, false, folio);
> -
>  	/*
>  	 * For THP, we have to assume the worse case ie pmd for invalidation.
>  	 * For hugetlb, it could be much worse if we need to do pud
> @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>  	range.end = vma_address_end(&pvmw);
>  	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
>  				address, range.end);
> +	if (flags & TTU_SPLIT_HUGE_PMD)
> +		align_huge_pmd_range(vma, &range.start, &range.end);
>  	if (folio_test_hugetlb(folio)) {
>  		/*
>  		 * If sharing is possible, start and end will be adjusted
> @@ -1664,9 +1663,6 @@ 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)) {
> -		/* Unexpected PMD-mapped THP? */
> -		VM_BUG_ON_FOLIO(!pvmw.pte, folio);
> -
>  		/*
>  		 * If the folio is in an mlock()d vma, we must not swap it out.
>  		 */
> @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>  			goto walk_done_err;
>  		}
>
> +		if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
> +			/*
> +			 * We temporarily have to drop the PTL and start once
> +			 * again from that now-PTE-mapped page table.
> +			 */
> +			split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
> +					      folio);

Just in case you might miss here, since you will no longer align
range.start as Baolin mentioned in another email and you have a VM_WARN_ONCE
in split_huge_pmd_locked(), you will need to align the input address now.

> +			pvmw.pmd = NULL;
> +			spin_unlock(pvmw.ptl);
> +			flags &= ~TTU_SPLIT_HUGE_PMD;
> +			continue;
> +		}
> +
> +		/* Unexpected PMD-mapped THP? */
> +		VM_BUG_ON_FOLIO(!pvmw.pte, folio);
> +
>  		pfn = pte_pfn(ptep_get(pvmw.pte));
>  		subpage = folio_page(folio, pfn - folio_pfn(folio));
>  		address = pvmw.address;
> -- 
> 2.33.1


--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Lance Yang]

On Tue, May 7, 2024 at 11:26 PM Zi Yan <ziy@nvidia.com> wrote:
>
> On 1 May 2024, at 0:26, Lance Yang wrote:
>
> > In preparation for supporting try_to_unmap_one() to unmap PMD-mapped
> > folios, start the pagewalk first, then call split_huge_pmd_address()
> > to split the folio.
> >
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Lance Yang <ioworker0@gmail.com>
> > ---
> >  include/linux/huge_mm.h | 20 ++++++++++++++++++++
> >  mm/huge_memory.c        | 42 +++++++++++++++++++++--------------------
> >  mm/rmap.c               | 24 +++++++++++++++++------
> >  3 files changed, 60 insertions(+), 26 deletions(-)
> >
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index c8d3ec116e29..38c4b5537715 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void)
> >       return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
> >  }
> >
> > +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> > +                        pmd_t *pmd, bool freeze, struct folio *folio);
> > +
> > +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> > +                                     unsigned long *start,
> > +                                     unsigned long *end)
> > +{
> > +     *start = ALIGN(*start, HPAGE_PMD_SIZE);
> > +     *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE);
> > +
> > +     VM_WARN_ON_ONCE(vma->vm_start > *start);
> > +     VM_WARN_ON_ONCE(vma->vm_end < *end);
> > +}
> > +
> >  #else /* CONFIG_TRANSPARENT_HUGEPAGE */
> >
> >  static inline bool folio_test_pmd_mappable(struct folio *folio)
> > @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> >               unsigned long address, bool freeze, struct folio *folio) {}
> >  static inline void split_huge_pmd_address(struct vm_area_struct *vma,
> >               unsigned long address, bool freeze, struct folio *folio) {}
> > +static inline void split_huge_pmd_locked(struct vm_area_struct *vma,
> > +                                      unsigned long address, pmd_t *pmd,
> > +                                      bool freeze, struct folio *folio) {}
> > +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> > +                                     unsigned long *start,
> > +                                     unsigned long *end) {}
> >
> >  #define split_huge_pud(__vma, __pmd, __address)      \
> >       do { } while (0)
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 8261b5669397..145505a1dd05 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >       pmd_populate(mm, pmd, pgtable);
> >  }
> >
> > +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> > +                        pmd_t *pmd, bool freeze, struct folio *folio)
> > +{
> > +     VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio));
> > +     VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE));
> > +     VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
> > +     VM_BUG_ON(freeze && !folio);
> > +
> > +     /*
> > +      * When the caller requests to set up a migration entry, we
> > +      * require a folio to check the PMD against. Otherwise, there
> > +      * is a risk of replacing the wrong folio.
> > +      */
> > +     if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
> > +         is_pmd_migration_entry(*pmd)) {
> > +             if (folio && folio != pmd_folio(*pmd))
> > +                     return;
> > +             __split_huge_pmd_locked(vma, pmd, address, freeze);
> > +     }
> > +}
> > +
> >  void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> >               unsigned long address, bool freeze, struct folio *folio)
> >  {
> > @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> >                               (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE);
> >       mmu_notifier_invalidate_range_start(&range);
> >       ptl = pmd_lock(vma->vm_mm, pmd);
> > -
> > -     /*
> > -      * If caller asks to setup a migration entry, we need a folio to check
> > -      * pmd against. Otherwise we can end up replacing wrong folio.
> > -      */
> > -     VM_BUG_ON(freeze && !folio);
> > -     VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
> > -
> > -     if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
> > -         is_pmd_migration_entry(*pmd)) {
> > -             /*
> > -              * It's safe to call pmd_page when folio is set because it's
> > -              * guaranteed that pmd is present.
> > -              */
> > -             if (folio && folio != pmd_folio(*pmd))
> > -                     goto out;
> > -             __split_huge_pmd_locked(vma, pmd, range.start, freeze);
> > -     }
> > -
> > -out:
> > +     split_huge_pmd_locked(vma, range.start, pmd, freeze, folio);
> >       spin_unlock(ptl);
> >       mmu_notifier_invalidate_range_end(&range);
> >  }
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index 7e2575d669a9..432601154583 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >       if (flags & TTU_SYNC)
> >               pvmw.flags = PVMW_SYNC;
> >
> > -     if (flags & TTU_SPLIT_HUGE_PMD)
> > -             split_huge_pmd_address(vma, address, false, folio);
> > -
> >       /*
> >        * For THP, we have to assume the worse case ie pmd for invalidation.
> >        * For hugetlb, it could be much worse if we need to do pud
> > @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >       range.end = vma_address_end(&pvmw);
> >       mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
> >                               address, range.end);
> > +     if (flags & TTU_SPLIT_HUGE_PMD)
> > +             align_huge_pmd_range(vma, &range.start, &range.end);
> >       if (folio_test_hugetlb(folio)) {
> >               /*
> >                * If sharing is possible, start and end will be adjusted
> > @@ -1664,9 +1663,6 @@ 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)) {
> > -             /* Unexpected PMD-mapped THP? */
> > -             VM_BUG_ON_FOLIO(!pvmw.pte, folio);
> > -
> >               /*
> >                * If the folio is in an mlock()d vma, we must not swap it out.
> >                */
> > @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >                       goto walk_done_err;
> >               }
> >
> > +             if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
> > +                     /*
> > +                      * We temporarily have to drop the PTL and start once
> > +                      * again from that now-PTE-mapped page table.
> > +                      */
> > +                     split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
> > +                                           folio);
>
> Just in case you might miss here, since you will no longer align
> range.start as Baolin mentioned in another email and you have a VM_WARN_ONCE
> in split_huge_pmd_locked(), you will need to align the input address now.

Thanks for bringing that up!

I do miss the alignment here when I decide to no longer align range.start
in another email - thanks!

Zi, could I move the alignment here?
IIUC, we will not encounter a partially mapped THP here, and range.start
and range.end should also not beyond the VMA limits.

                     align_huge_pmd_range(vma, &range.start, &range.end);
                     split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
                                           folio);


Thanks,
Lance

>
> > +                     pvmw.pmd = NULL;
> > +                     spin_unlock(pvmw.ptl);
> > +                     flags &= ~TTU_SPLIT_HUGE_PMD;
> > +                     continue;
> > +             }
> > +
> > +             /* Unexpected PMD-mapped THP? */
> > +             VM_BUG_ON_FOLIO(!pvmw.pte, folio);
> > +
> >               pfn = pte_pfn(ptep_get(pvmw.pte));
> >               subpage = folio_page(folio, pfn - folio_pfn(folio));
> >               address = pvmw.address;
> > --
> > 2.33.1
>
>
> --
> Best Regards,
> Yan, Zi

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 9141 bytes --]

On 8 May 2024, at 1:43, Lance Yang wrote:

> On Tue, May 7, 2024 at 11:26 PM Zi Yan <ziy@nvidia.com> wrote:
>>
>> On 1 May 2024, at 0:26, Lance Yang wrote:
>>
>>> In preparation for supporting try_to_unmap_one() to unmap PMD-mapped
>>> folios, start the pagewalk first, then call split_huge_pmd_address()
>>> to split the folio.
>>>
>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>> Signed-off-by: Lance Yang <ioworker0@gmail.com>
>>> ---
>>>  include/linux/huge_mm.h | 20 ++++++++++++++++++++
>>>  mm/huge_memory.c        | 42 +++++++++++++++++++++--------------------
>>>  mm/rmap.c               | 24 +++++++++++++++++------
>>>  3 files changed, 60 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>> index c8d3ec116e29..38c4b5537715 100644
>>> --- a/include/linux/huge_mm.h
>>> +++ b/include/linux/huge_mm.h
>>> @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void)
>>>       return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
>>>  }
>>>
>>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
>>> +                        pmd_t *pmd, bool freeze, struct folio *folio);
>>> +
>>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>>> +                                     unsigned long *start,
>>> +                                     unsigned long *end)
>>> +{
>>> +     *start = ALIGN(*start, HPAGE_PMD_SIZE);
>>> +     *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE);
>>> +
>>> +     VM_WARN_ON_ONCE(vma->vm_start > *start);
>>> +     VM_WARN_ON_ONCE(vma->vm_end < *end);
>>> +}
>>> +
>>>  #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>
>>>  static inline bool folio_test_pmd_mappable(struct folio *folio)
>>> @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>>>               unsigned long address, bool freeze, struct folio *folio) {}
>>>  static inline void split_huge_pmd_address(struct vm_area_struct *vma,
>>>               unsigned long address, bool freeze, struct folio *folio) {}
>>> +static inline void split_huge_pmd_locked(struct vm_area_struct *vma,
>>> +                                      unsigned long address, pmd_t *pmd,
>>> +                                      bool freeze, struct folio *folio) {}
>>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>>> +                                     unsigned long *start,
>>> +                                     unsigned long *end) {}
>>>
>>>  #define split_huge_pud(__vma, __pmd, __address)      \
>>>       do { } while (0)
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index 8261b5669397..145505a1dd05 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>>>       pmd_populate(mm, pmd, pgtable);
>>>  }
>>>
>>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
>>> +                        pmd_t *pmd, bool freeze, struct folio *folio)
>>> +{
>>> +     VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio));
>>> +     VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE));
>>> +     VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
>>> +     VM_BUG_ON(freeze && !folio);
>>> +
>>> +     /*
>>> +      * When the caller requests to set up a migration entry, we
>>> +      * require a folio to check the PMD against. Otherwise, there
>>> +      * is a risk of replacing the wrong folio.
>>> +      */
>>> +     if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
>>> +         is_pmd_migration_entry(*pmd)) {
>>> +             if (folio && folio != pmd_folio(*pmd))
>>> +                     return;
>>> +             __split_huge_pmd_locked(vma, pmd, address, freeze);
>>> +     }
>>> +}
>>> +
>>>  void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>>>               unsigned long address, bool freeze, struct folio *folio)
>>>  {
>>> @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>>>                               (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE);
>>>       mmu_notifier_invalidate_range_start(&range);
>>>       ptl = pmd_lock(vma->vm_mm, pmd);
>>> -
>>> -     /*
>>> -      * If caller asks to setup a migration entry, we need a folio to check
>>> -      * pmd against. Otherwise we can end up replacing wrong folio.
>>> -      */
>>> -     VM_BUG_ON(freeze && !folio);
>>> -     VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
>>> -
>>> -     if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
>>> -         is_pmd_migration_entry(*pmd)) {
>>> -             /*
>>> -              * It's safe to call pmd_page when folio is set because it's
>>> -              * guaranteed that pmd is present.
>>> -              */
>>> -             if (folio && folio != pmd_folio(*pmd))
>>> -                     goto out;
>>> -             __split_huge_pmd_locked(vma, pmd, range.start, freeze);
>>> -     }
>>> -
>>> -out:
>>> +     split_huge_pmd_locked(vma, range.start, pmd, freeze, folio);
>>>       spin_unlock(ptl);
>>>       mmu_notifier_invalidate_range_end(&range);
>>>  }
>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>> index 7e2575d669a9..432601154583 100644
>>> --- a/mm/rmap.c
>>> +++ b/mm/rmap.c
>>> @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>>       if (flags & TTU_SYNC)
>>>               pvmw.flags = PVMW_SYNC;
>>>
>>> -     if (flags & TTU_SPLIT_HUGE_PMD)
>>> -             split_huge_pmd_address(vma, address, false, folio);
>>> -
>>>       /*
>>>        * For THP, we have to assume the worse case ie pmd for invalidation.
>>>        * For hugetlb, it could be much worse if we need to do pud
>>> @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>>       range.end = vma_address_end(&pvmw);
>>>       mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
>>>                               address, range.end);
>>> +     if (flags & TTU_SPLIT_HUGE_PMD)
>>> +             align_huge_pmd_range(vma, &range.start, &range.end);
>>>       if (folio_test_hugetlb(folio)) {
>>>               /*
>>>                * If sharing is possible, start and end will be adjusted
>>> @@ -1664,9 +1663,6 @@ 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)) {
>>> -             /* Unexpected PMD-mapped THP? */
>>> -             VM_BUG_ON_FOLIO(!pvmw.pte, folio);
>>> -
>>>               /*
>>>                * If the folio is in an mlock()d vma, we must not swap it out.
>>>                */
>>> @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>>                       goto walk_done_err;
>>>               }
>>>
>>> +             if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
>>> +                     /*
>>> +                      * We temporarily have to drop the PTL and start once
>>> +                      * again from that now-PTE-mapped page table.
>>> +                      */
>>> +                     split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
>>> +                                           folio);
>>
>> Just in case you might miss here, since you will no longer align
>> range.start as Baolin mentioned in another email and you have a VM_WARN_ONCE
>> in split_huge_pmd_locked(), you will need to align the input address now.
>
> Thanks for bringing that up!
>
> I do miss the alignment here when I decide to no longer align range.start
> in another email - thanks!
>
No problem.

> Zi, could I move the alignment here?
> IIUC, we will not encounter a partially mapped THP here, and range.start
> and range.end should also not beyond the VMA limits.
>
>                      align_huge_pmd_range(vma, &range.start, &range.end);
>                      split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
>                                            folio);

I think you can just do

split_huge_pmd_locked(vma, ALIGN(range.start, HPAGE_PMD_SIZE), pvmw.pmd, false, folio);

since range will later be used by mmu_notifier_invalidate_range_end() and changing
it might cause secondary TLB invalidation issues.

>
> Thanks,
> Lance
>
>>
>>> +                     pvmw.pmd = NULL;
>>> +                     spin_unlock(pvmw.ptl);
>>> +                     flags &= ~TTU_SPLIT_HUGE_PMD;
>>> +                     continue;
>>> +             }
>>> +
>>> +             /* Unexpected PMD-mapped THP? */
>>> +             VM_BUG_ON_FOLIO(!pvmw.pte, folio);
>>> +
>>>               pfn = pte_pfn(ptep_get(pvmw.pte));
>>>               subpage = folio_page(folio, pfn - folio_pfn(folio));
>>>               address = pvmw.address;
>>> --
>>> 2.33.1
>>
>>
>> --
>> Best Regards,
>> Yan, Zi


--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Lance Yang]

On Wed, May 8, 2024 at 10:07 PM Zi Yan <ziy@nvidia.com> wrote:
>
> On 8 May 2024, at 1:43, Lance Yang wrote:
>
> > On Tue, May 7, 2024 at 11:26 PM Zi Yan <ziy@nvidia.com> wrote:
> >>
> >> On 1 May 2024, at 0:26, Lance Yang wrote:
> >>
> >>> In preparation for supporting try_to_unmap_one() to unmap PMD-mapped
> >>> folios, start the pagewalk first, then call split_huge_pmd_address()
> >>> to split the folio.
> >>>
> >>> Suggested-by: David Hildenbrand <david@redhat.com>
> >>> Signed-off-by: Lance Yang <ioworker0@gmail.com>
> >>> ---
> >>>  include/linux/huge_mm.h | 20 ++++++++++++++++++++
> >>>  mm/huge_memory.c        | 42 +++++++++++++++++++++--------------------
> >>>  mm/rmap.c               | 24 +++++++++++++++++------
> >>>  3 files changed, 60 insertions(+), 26 deletions(-)
> >>>
> >>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >>> index c8d3ec116e29..38c4b5537715 100644
> >>> --- a/include/linux/huge_mm.h
> >>> +++ b/include/linux/huge_mm.h
> >>> @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void)
> >>>       return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
> >>>  }
> >>>
> >>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> >>> +                        pmd_t *pmd, bool freeze, struct folio *folio);
> >>> +
> >>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> >>> +                                     unsigned long *start,
> >>> +                                     unsigned long *end)
> >>> +{
> >>> +     *start = ALIGN(*start, HPAGE_PMD_SIZE);
> >>> +     *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE);
> >>> +
> >>> +     VM_WARN_ON_ONCE(vma->vm_start > *start);
> >>> +     VM_WARN_ON_ONCE(vma->vm_end < *end);
> >>> +}
> >>> +
> >>>  #else /* CONFIG_TRANSPARENT_HUGEPAGE */
> >>>
> >>>  static inline bool folio_test_pmd_mappable(struct folio *folio)
> >>> @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> >>>               unsigned long address, bool freeze, struct folio *folio) {}
> >>>  static inline void split_huge_pmd_address(struct vm_area_struct *vma,
> >>>               unsigned long address, bool freeze, struct folio *folio) {}
> >>> +static inline void split_huge_pmd_locked(struct vm_area_struct *vma,
> >>> +                                      unsigned long address, pmd_t *pmd,
> >>> +                                      bool freeze, struct folio *folio) {}
> >>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> >>> +                                     unsigned long *start,
> >>> +                                     unsigned long *end) {}
> >>>
> >>>  #define split_huge_pud(__vma, __pmd, __address)      \
> >>>       do { } while (0)
> >>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >>> index 8261b5669397..145505a1dd05 100644
> >>> --- a/mm/huge_memory.c
> >>> +++ b/mm/huge_memory.c
> >>> @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> >>>       pmd_populate(mm, pmd, pgtable);
> >>>  }
> >>>
> >>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> >>> +                        pmd_t *pmd, bool freeze, struct folio *folio)
> >>> +{
> >>> +     VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio));
> >>> +     VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE));
> >>> +     VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
> >>> +     VM_BUG_ON(freeze && !folio);
> >>> +
> >>> +     /*
> >>> +      * When the caller requests to set up a migration entry, we
> >>> +      * require a folio to check the PMD against. Otherwise, there
> >>> +      * is a risk of replacing the wrong folio.
> >>> +      */
> >>> +     if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
> >>> +         is_pmd_migration_entry(*pmd)) {
> >>> +             if (folio && folio != pmd_folio(*pmd))
> >>> +                     return;
> >>> +             __split_huge_pmd_locked(vma, pmd, address, freeze);
> >>> +     }
> >>> +}
> >>> +
> >>>  void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> >>>               unsigned long address, bool freeze, struct folio *folio)
> >>>  {
> >>> @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> >>>                               (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE);
> >>>       mmu_notifier_invalidate_range_start(&range);
> >>>       ptl = pmd_lock(vma->vm_mm, pmd);
> >>> -
> >>> -     /*
> >>> -      * If caller asks to setup a migration entry, we need a folio to check
> >>> -      * pmd against. Otherwise we can end up replacing wrong folio.
> >>> -      */
> >>> -     VM_BUG_ON(freeze && !folio);
> >>> -     VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
> >>> -
> >>> -     if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
> >>> -         is_pmd_migration_entry(*pmd)) {
> >>> -             /*
> >>> -              * It's safe to call pmd_page when folio is set because it's
> >>> -              * guaranteed that pmd is present.
> >>> -              */
> >>> -             if (folio && folio != pmd_folio(*pmd))
> >>> -                     goto out;
> >>> -             __split_huge_pmd_locked(vma, pmd, range.start, freeze);
> >>> -     }
> >>> -
> >>> -out:
> >>> +     split_huge_pmd_locked(vma, range.start, pmd, freeze, folio);
> >>>       spin_unlock(ptl);
> >>>       mmu_notifier_invalidate_range_end(&range);
> >>>  }
> >>> diff --git a/mm/rmap.c b/mm/rmap.c
> >>> index 7e2575d669a9..432601154583 100644
> >>> --- a/mm/rmap.c
> >>> +++ b/mm/rmap.c
> >>> @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >>>       if (flags & TTU_SYNC)
> >>>               pvmw.flags = PVMW_SYNC;
> >>>
> >>> -     if (flags & TTU_SPLIT_HUGE_PMD)
> >>> -             split_huge_pmd_address(vma, address, false, folio);
> >>> -
> >>>       /*
> >>>        * For THP, we have to assume the worse case ie pmd for invalidation.
> >>>        * For hugetlb, it could be much worse if we need to do pud
> >>> @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >>>       range.end = vma_address_end(&pvmw);
> >>>       mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
> >>>                               address, range.end);
> >>> +     if (flags & TTU_SPLIT_HUGE_PMD)
> >>> +             align_huge_pmd_range(vma, &range.start, &range.end);
> >>>       if (folio_test_hugetlb(folio)) {
> >>>               /*
> >>>                * If sharing is possible, start and end will be adjusted
> >>> @@ -1664,9 +1663,6 @@ 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)) {
> >>> -             /* Unexpected PMD-mapped THP? */
> >>> -             VM_BUG_ON_FOLIO(!pvmw.pte, folio);
> >>> -
> >>>               /*
> >>>                * If the folio is in an mlock()d vma, we must not swap it out.
> >>>                */
> >>> @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >>>                       goto walk_done_err;
> >>>               }
> >>>
> >>> +             if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
> >>> +                     /*
> >>> +                      * We temporarily have to drop the PTL and start once
> >>> +                      * again from that now-PTE-mapped page table.
> >>> +                      */
> >>> +                     split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
> >>> +                                           folio);
> >>
> >> Just in case you might miss here, since you will no longer align
> >> range.start as Baolin mentioned in another email and you have a VM_WARN_ONCE
> >> in split_huge_pmd_locked(), you will need to align the input address now.
> >
> > Thanks for bringing that up!
> >
> > I do miss the alignment here when I decide to no longer align range.start
> > in another email - thanks!
> >
> No problem.
>
> > Zi, could I move the alignment here?
> > IIUC, we will not encounter a partially mapped THP here, and range.start
> > and range.end should also not beyond the VMA limits.
> >
> >                      align_huge_pmd_range(vma, &range.start, &range.end);
> >                      split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
> >                                            folio);
>
> I think you can just do
>
> split_huge_pmd_locked(vma, ALIGN(range.start, HPAGE_PMD_SIZE), pvmw.pmd, false, folio);
>
> since range will later be used by mmu_notifier_invalidate_range_end() and changing
> it might cause secondary TLB invalidation issues.

Ok, makes sense to me - thanks!

But we probably cannot use the HPAGE_PMD_SIZE here; it will cause
broken compilation as seen in v3[1].

Perhaps we still need to add a new alignment function for the huge PMD?

[1] https://lore.kernel.org/linux-mm/20240429202040.187453-1-sj@kernel.org/

Thanks again for the review!

Best,
Lance



>
> >
> > Thanks,
> > Lance
> >
> >>
> >>> +                     pvmw.pmd = NULL;
> >>> +                     spin_unlock(pvmw.ptl);
> >>> +                     flags &= ~TTU_SPLIT_HUGE_PMD;
> >>> +                     continue;
> >>> +             }
> >>> +
> >>> +             /* Unexpected PMD-mapped THP? */
> >>> +             VM_BUG_ON_FOLIO(!pvmw.pte, folio);
> >>> +
> >>>               pfn = pte_pfn(ptep_get(pvmw.pte));
> >>>               subpage = folio_page(folio, pfn - folio_pfn(folio));
> >>>               address = pvmw.address;
> >>> --
> >>> 2.33.1
> >>
> >>
> >> --
> >> Best Regards,
> >> Yan, Zi
>
>
> --
> Best Regards,
> Yan, Zi

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 9737 bytes --]

On 8 May 2024, at 10:35, Lance Yang wrote:

> On Wed, May 8, 2024 at 10:07 PM Zi Yan <ziy@nvidia.com> wrote:
>>
>> On 8 May 2024, at 1:43, Lance Yang wrote:
>>
>>> On Tue, May 7, 2024 at 11:26 PM Zi Yan <ziy@nvidia.com> wrote:
>>>>
>>>> On 1 May 2024, at 0:26, Lance Yang wrote:
>>>>
>>>>> In preparation for supporting try_to_unmap_one() to unmap PMD-mapped
>>>>> folios, start the pagewalk first, then call split_huge_pmd_address()
>>>>> to split the folio.
>>>>>
>>>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>>>> Signed-off-by: Lance Yang <ioworker0@gmail.com>
>>>>> ---
>>>>>  include/linux/huge_mm.h | 20 ++++++++++++++++++++
>>>>>  mm/huge_memory.c        | 42 +++++++++++++++++++++--------------------
>>>>>  mm/rmap.c               | 24 +++++++++++++++++------
>>>>>  3 files changed, 60 insertions(+), 26 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>>> index c8d3ec116e29..38c4b5537715 100644
>>>>> --- a/include/linux/huge_mm.h
>>>>> +++ b/include/linux/huge_mm.h
>>>>> @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void)
>>>>>       return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
>>>>>  }
>>>>>
>>>>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
>>>>> +                        pmd_t *pmd, bool freeze, struct folio *folio);
>>>>> +
>>>>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>>>>> +                                     unsigned long *start,
>>>>> +                                     unsigned long *end)
>>>>> +{
>>>>> +     *start = ALIGN(*start, HPAGE_PMD_SIZE);
>>>>> +     *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE);
>>>>> +
>>>>> +     VM_WARN_ON_ONCE(vma->vm_start > *start);
>>>>> +     VM_WARN_ON_ONCE(vma->vm_end < *end);
>>>>> +}
>>>>> +
>>>>>  #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>>>
>>>>>  static inline bool folio_test_pmd_mappable(struct folio *folio)
>>>>> @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>>>>>               unsigned long address, bool freeze, struct folio *folio) {}
>>>>>  static inline void split_huge_pmd_address(struct vm_area_struct *vma,
>>>>>               unsigned long address, bool freeze, struct folio *folio) {}
>>>>> +static inline void split_huge_pmd_locked(struct vm_area_struct *vma,
>>>>> +                                      unsigned long address, pmd_t *pmd,
>>>>> +                                      bool freeze, struct folio *folio) {}
>>>>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>>>>> +                                     unsigned long *start,
>>>>> +                                     unsigned long *end) {}
>>>>>
>>>>>  #define split_huge_pud(__vma, __pmd, __address)      \
>>>>>       do { } while (0)
>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>> index 8261b5669397..145505a1dd05 100644
>>>>> --- a/mm/huge_memory.c
>>>>> +++ b/mm/huge_memory.c
>>>>> @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>>>>>       pmd_populate(mm, pmd, pgtable);
>>>>>  }
>>>>>
>>>>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
>>>>> +                        pmd_t *pmd, bool freeze, struct folio *folio)
>>>>> +{
>>>>> +     VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio));
>>>>> +     VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE));
>>>>> +     VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
>>>>> +     VM_BUG_ON(freeze && !folio);
>>>>> +
>>>>> +     /*
>>>>> +      * When the caller requests to set up a migration entry, we
>>>>> +      * require a folio to check the PMD against. Otherwise, there
>>>>> +      * is a risk of replacing the wrong folio.
>>>>> +      */
>>>>> +     if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
>>>>> +         is_pmd_migration_entry(*pmd)) {
>>>>> +             if (folio && folio != pmd_folio(*pmd))
>>>>> +                     return;
>>>>> +             __split_huge_pmd_locked(vma, pmd, address, freeze);
>>>>> +     }
>>>>> +}
>>>>> +
>>>>>  void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>>>>>               unsigned long address, bool freeze, struct folio *folio)
>>>>>  {
>>>>> @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>>>>>                               (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE);
>>>>>       mmu_notifier_invalidate_range_start(&range);
>>>>>       ptl = pmd_lock(vma->vm_mm, pmd);
>>>>> -
>>>>> -     /*
>>>>> -      * If caller asks to setup a migration entry, we need a folio to check
>>>>> -      * pmd against. Otherwise we can end up replacing wrong folio.
>>>>> -      */
>>>>> -     VM_BUG_ON(freeze && !folio);
>>>>> -     VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
>>>>> -
>>>>> -     if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
>>>>> -         is_pmd_migration_entry(*pmd)) {
>>>>> -             /*
>>>>> -              * It's safe to call pmd_page when folio is set because it's
>>>>> -              * guaranteed that pmd is present.
>>>>> -              */
>>>>> -             if (folio && folio != pmd_folio(*pmd))
>>>>> -                     goto out;
>>>>> -             __split_huge_pmd_locked(vma, pmd, range.start, freeze);
>>>>> -     }
>>>>> -
>>>>> -out:
>>>>> +     split_huge_pmd_locked(vma, range.start, pmd, freeze, folio);
>>>>>       spin_unlock(ptl);
>>>>>       mmu_notifier_invalidate_range_end(&range);
>>>>>  }
>>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>>> index 7e2575d669a9..432601154583 100644
>>>>> --- a/mm/rmap.c
>>>>> +++ b/mm/rmap.c
>>>>> @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>>>>       if (flags & TTU_SYNC)
>>>>>               pvmw.flags = PVMW_SYNC;
>>>>>
>>>>> -     if (flags & TTU_SPLIT_HUGE_PMD)
>>>>> -             split_huge_pmd_address(vma, address, false, folio);
>>>>> -
>>>>>       /*
>>>>>        * For THP, we have to assume the worse case ie pmd for invalidation.
>>>>>        * For hugetlb, it could be much worse if we need to do pud
>>>>> @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>>>>       range.end = vma_address_end(&pvmw);
>>>>>       mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
>>>>>                               address, range.end);
>>>>> +     if (flags & TTU_SPLIT_HUGE_PMD)
>>>>> +             align_huge_pmd_range(vma, &range.start, &range.end);
>>>>>       if (folio_test_hugetlb(folio)) {
>>>>>               /*
>>>>>                * If sharing is possible, start and end will be adjusted
>>>>> @@ -1664,9 +1663,6 @@ 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)) {
>>>>> -             /* Unexpected PMD-mapped THP? */
>>>>> -             VM_BUG_ON_FOLIO(!pvmw.pte, folio);
>>>>> -
>>>>>               /*
>>>>>                * If the folio is in an mlock()d vma, we must not swap it out.
>>>>>                */
>>>>> @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>>>>                       goto walk_done_err;
>>>>>               }
>>>>>
>>>>> +             if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
>>>>> +                     /*
>>>>> +                      * We temporarily have to drop the PTL and start once
>>>>> +                      * again from that now-PTE-mapped page table.
>>>>> +                      */
>>>>> +                     split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
>>>>> +                                           folio);
>>>>
>>>> Just in case you might miss here, since you will no longer align
>>>> range.start as Baolin mentioned in another email and you have a VM_WARN_ONCE
>>>> in split_huge_pmd_locked(), you will need to align the input address now.
>>>
>>> Thanks for bringing that up!
>>>
>>> I do miss the alignment here when I decide to no longer align range.start
>>> in another email - thanks!
>>>
>> No problem.
>>
>>> Zi, could I move the alignment here?
>>> IIUC, we will not encounter a partially mapped THP here, and range.start
>>> and range.end should also not beyond the VMA limits.
>>>
>>>                      align_huge_pmd_range(vma, &range.start, &range.end);
>>>                      split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
>>>                                            folio);
>>
>> I think you can just do
>>
>> split_huge_pmd_locked(vma, ALIGN(range.start, HPAGE_PMD_SIZE), pvmw.pmd, false, folio);
>>
>> since range will later be used by mmu_notifier_invalidate_range_end() and changing
>> it might cause secondary TLB invalidation issues.
>
> Ok, makes sense to me - thanks!
>
> But we probably cannot use the HPAGE_PMD_SIZE here; it will cause
> broken compilation as seen in v3[1].
>
> Perhaps we still need to add a new alignment function for the huge PMD?
>
> [1] https://lore.kernel.org/linux-mm/20240429202040.187453-1-sj@kernel.org/
>
> Thanks again for the review!
>
Or you can adjust the alignment inside split_huge_pmd_locked(), since it can
be called other than __split_huge_pmd().

Hmm, I notice that split_huge_pmd_address() has mmu_notifier ops but your
split_huge_pmd_locked() does not include them, I wonder if that could cause
issues with mmu_notifier issues. Adding mmu_notifier people to confirm.


--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 10585 bytes --]

On 8 May 2024, at 10:48, Zi Yan wrote:

> On 8 May 2024, at 10:35, Lance Yang wrote:
>
>> On Wed, May 8, 2024 at 10:07 PM Zi Yan <ziy@nvidia.com> wrote:
>>>
>>> On 8 May 2024, at 1:43, Lance Yang wrote:
>>>
>>>> On Tue, May 7, 2024 at 11:26 PM Zi Yan <ziy@nvidia.com> wrote:
>>>>>
>>>>> On 1 May 2024, at 0:26, Lance Yang wrote:
>>>>>
>>>>>> In preparation for supporting try_to_unmap_one() to unmap PMD-mapped
>>>>>> folios, start the pagewalk first, then call split_huge_pmd_address()
>>>>>> to split the folio.
>>>>>>
>>>>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>>>>> Signed-off-by: Lance Yang <ioworker0@gmail.com>
>>>>>> ---
>>>>>>  include/linux/huge_mm.h | 20 ++++++++++++++++++++
>>>>>>  mm/huge_memory.c        | 42 +++++++++++++++++++++--------------------
>>>>>>  mm/rmap.c               | 24 +++++++++++++++++------
>>>>>>  3 files changed, 60 insertions(+), 26 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>>>> index c8d3ec116e29..38c4b5537715 100644
>>>>>> --- a/include/linux/huge_mm.h
>>>>>> +++ b/include/linux/huge_mm.h
>>>>>> @@ -409,6 +409,20 @@ static inline bool thp_migration_supported(void)
>>>>>>       return IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION);
>>>>>>  }
>>>>>>
>>>>>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
>>>>>> +                        pmd_t *pmd, bool freeze, struct folio *folio);
>>>>>> +
>>>>>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>>>>>> +                                     unsigned long *start,
>>>>>> +                                     unsigned long *end)
>>>>>> +{
>>>>>> +     *start = ALIGN(*start, HPAGE_PMD_SIZE);
>>>>>> +     *end = ALIGN_DOWN(*end, HPAGE_PMD_SIZE);
>>>>>> +
>>>>>> +     VM_WARN_ON_ONCE(vma->vm_start > *start);
>>>>>> +     VM_WARN_ON_ONCE(vma->vm_end < *end);
>>>>>> +}
>>>>>> +
>>>>>>  #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>>>>
>>>>>>  static inline bool folio_test_pmd_mappable(struct folio *folio)
>>>>>> @@ -471,6 +485,12 @@ static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>>>>>>               unsigned long address, bool freeze, struct folio *folio) {}
>>>>>>  static inline void split_huge_pmd_address(struct vm_area_struct *vma,
>>>>>>               unsigned long address, bool freeze, struct folio *folio) {}
>>>>>> +static inline void split_huge_pmd_locked(struct vm_area_struct *vma,
>>>>>> +                                      unsigned long address, pmd_t *pmd,
>>>>>> +                                      bool freeze, struct folio *folio) {}
>>>>>> +static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>>>>>> +                                     unsigned long *start,
>>>>>> +                                     unsigned long *end) {}
>>>>>>
>>>>>>  #define split_huge_pud(__vma, __pmd, __address)      \
>>>>>>       do { } while (0)
>>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>>> index 8261b5669397..145505a1dd05 100644
>>>>>> --- a/mm/huge_memory.c
>>>>>> +++ b/mm/huge_memory.c
>>>>>> @@ -2584,6 +2584,27 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>>>>>>       pmd_populate(mm, pmd, pgtable);
>>>>>>  }
>>>>>>
>>>>>> +void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
>>>>>> +                        pmd_t *pmd, bool freeze, struct folio *folio)
>>>>>> +{
>>>>>> +     VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio));
>>>>>> +     VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE));
>>>>>> +     VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
>>>>>> +     VM_BUG_ON(freeze && !folio);
>>>>>> +
>>>>>> +     /*
>>>>>> +      * When the caller requests to set up a migration entry, we
>>>>>> +      * require a folio to check the PMD against. Otherwise, there
>>>>>> +      * is a risk of replacing the wrong folio.
>>>>>> +      */
>>>>>> +     if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
>>>>>> +         is_pmd_migration_entry(*pmd)) {
>>>>>> +             if (folio && folio != pmd_folio(*pmd))
>>>>>> +                     return;
>>>>>> +             __split_huge_pmd_locked(vma, pmd, address, freeze);
>>>>>> +     }
>>>>>> +}
>>>>>> +
>>>>>>  void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>>>>>>               unsigned long address, bool freeze, struct folio *folio)
>>>>>>  {
>>>>>> @@ -2595,26 +2616,7 @@ void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
>>>>>>                               (address & HPAGE_PMD_MASK) + HPAGE_PMD_SIZE);
>>>>>>       mmu_notifier_invalidate_range_start(&range);
>>>>>>       ptl = pmd_lock(vma->vm_mm, pmd);
>>>>>> -
>>>>>> -     /*
>>>>>> -      * If caller asks to setup a migration entry, we need a folio to check
>>>>>> -      * pmd against. Otherwise we can end up replacing wrong folio.
>>>>>> -      */
>>>>>> -     VM_BUG_ON(freeze && !folio);
>>>>>> -     VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
>>>>>> -
>>>>>> -     if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
>>>>>> -         is_pmd_migration_entry(*pmd)) {
>>>>>> -             /*
>>>>>> -              * It's safe to call pmd_page when folio is set because it's
>>>>>> -              * guaranteed that pmd is present.
>>>>>> -              */
>>>>>> -             if (folio && folio != pmd_folio(*pmd))
>>>>>> -                     goto out;
>>>>>> -             __split_huge_pmd_locked(vma, pmd, range.start, freeze);
>>>>>> -     }
>>>>>> -
>>>>>> -out:
>>>>>> +     split_huge_pmd_locked(vma, range.start, pmd, freeze, folio);
>>>>>>       spin_unlock(ptl);
>>>>>>       mmu_notifier_invalidate_range_end(&range);
>>>>>>  }
>>>>>> diff --git a/mm/rmap.c b/mm/rmap.c
>>>>>> index 7e2575d669a9..432601154583 100644
>>>>>> --- a/mm/rmap.c
>>>>>> +++ b/mm/rmap.c
>>>>>> @@ -1636,9 +1636,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>>>>>       if (flags & TTU_SYNC)
>>>>>>               pvmw.flags = PVMW_SYNC;
>>>>>>
>>>>>> -     if (flags & TTU_SPLIT_HUGE_PMD)
>>>>>> -             split_huge_pmd_address(vma, address, false, folio);
>>>>>> -
>>>>>>       /*
>>>>>>        * For THP, we have to assume the worse case ie pmd for invalidation.
>>>>>>        * For hugetlb, it could be much worse if we need to do pud
>>>>>> @@ -1650,6 +1647,8 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>>>>>       range.end = vma_address_end(&pvmw);
>>>>>>       mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
>>>>>>                               address, range.end);
>>>>>> +     if (flags & TTU_SPLIT_HUGE_PMD)
>>>>>> +             align_huge_pmd_range(vma, &range.start, &range.end);
>>>>>>       if (folio_test_hugetlb(folio)) {
>>>>>>               /*
>>>>>>                * If sharing is possible, start and end will be adjusted
>>>>>> @@ -1664,9 +1663,6 @@ 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)) {
>>>>>> -             /* Unexpected PMD-mapped THP? */
>>>>>> -             VM_BUG_ON_FOLIO(!pvmw.pte, folio);
>>>>>> -
>>>>>>               /*
>>>>>>                * If the folio is in an mlock()d vma, we must not swap it out.
>>>>>>                */
>>>>>> @@ -1678,6 +1674,22 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>>>>>>                       goto walk_done_err;
>>>>>>               }
>>>>>>
>>>>>> +             if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
>>>>>> +                     /*
>>>>>> +                      * We temporarily have to drop the PTL and start once
>>>>>> +                      * again from that now-PTE-mapped page table.
>>>>>> +                      */
>>>>>> +                     split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
>>>>>> +                                           folio);
>>>>>
>>>>> Just in case you might miss here, since you will no longer align
>>>>> range.start as Baolin mentioned in another email and you have a VM_WARN_ONCE
>>>>> in split_huge_pmd_locked(), you will need to align the input address now.
>>>>
>>>> Thanks for bringing that up!
>>>>
>>>> I do miss the alignment here when I decide to no longer align range.start
>>>> in another email - thanks!
>>>>
>>> No problem.
>>>
>>>> Zi, could I move the alignment here?
>>>> IIUC, we will not encounter a partially mapped THP here, and range.start
>>>> and range.end should also not beyond the VMA limits.
>>>>
>>>>                      align_huge_pmd_range(vma, &range.start, &range.end);
>>>>                      split_huge_pmd_locked(vma, range.start, pvmw.pmd, false,
>>>>                                            folio);
>>>
>>> I think you can just do
>>>
>>> split_huge_pmd_locked(vma, ALIGN(range.start, HPAGE_PMD_SIZE), pvmw.pmd, false, folio);
>>>
>>> since range will later be used by mmu_notifier_invalidate_range_end() and changing
>>> it might cause secondary TLB invalidation issues.
>>
>> Ok, makes sense to me - thanks!
>>
>> But we probably cannot use the HPAGE_PMD_SIZE here; it will cause
>> broken compilation as seen in v3[1].
>>
>> Perhaps we still need to add a new alignment function for the huge PMD?
>>
>> [1] https://lore.kernel.org/linux-mm/20240429202040.187453-1-sj@kernel.org/
>>
>> Thanks again for the review!
>>
> Or you can adjust the alignment inside split_huge_pmd_locked(), since it can
> be called other than __split_huge_pmd().
>
> Hmm, I notice that split_huge_pmd_address() has mmu_notifier ops but your
> split_huge_pmd_locked() does not include them, I wonder if that could cause
> issues with mmu_notifier issues. Adding mmu_notifier people to confirm.

Hi Alistair and Jason,

Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole,
so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw))
and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked()
and does not include the mmu notifier ops inside split_huge_pmd_address().
I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start()
before the while loop only has range of the original address and
split huge pmd can affect the entire PMD address range and these two ranges
might not be the same.

--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Jason Gunthorpe]

On Wed, May 08, 2024 at 10:56:34AM -0400, Zi Yan wrote:

> Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole,
> so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw))
> and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked()
> and does not include the mmu notifier ops inside split_huge_pmd_address().
> I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start()
> before the while loop only has range of the original address and
> split huge pmd can affect the entire PMD address range and these two ranges
> might not be the same.

That does not sound entirely good..

I suppose it depends on what split does, if the MM page table has the
same translation before and after split then perhaps no invalidation
is even necessary.

Jason 

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

On 8 May 2024, at 11:52, Jason Gunthorpe wrote:

> On Wed, May 08, 2024 at 10:56:34AM -0400, Zi Yan wrote:
>
>> Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole,
>> so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw))
>> and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked()
>> and does not include the mmu notifier ops inside split_huge_pmd_address().
>> I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start()
>> before the while loop only has range of the original address and
>> split huge pmd can affect the entire PMD address range and these two ranges
>> might not be the same.
>
> That does not sound entirely good..
>
> I suppose it depends on what split does, if the MM page table has the
> same translation before and after split then perhaps no invalidation
> is even necessary.

Before split, it is a PMD mapping to a PMD THP (order-9). After split,
they are 512 PTEs mapping to the same THP. Unless the secondary TLB
does not support PMD mapping and use 512 PTEs instead, it seems to
be an issue from my understanding.

In terms of two mmu_notifier ranges, first is in the split_huge_pmd_address()[1]
and second is in try_to_unmap_one()[2]. When try_to_unmap_one() is unmapping
a subpage in the middle of a PMD THP, the former notifies about the PMD range
change due to one PMD split into 512 PTEs and the latter only needs to notify
about the invalidation of the unmapped PTE. I do not think the latter can
replace the former, although a potential optimization can be that the latter
can be removed as it is included in the range of the former.

Regarding Lance's current code change, is it OK to change mmu_notifier range
after mmu_notifier_invalidate_range_start()? Since in Lance's code, first
mmu_notifier is gone and second, whose range is only about the single PTE,
starts mmu_notifier_invalidate_range_start(), then the code finds that
a PMD needs to be split into 512 PTEs. Would changing the range from PTE
to PMD suffice? Or the code should call mmu_notifier_invalidate_range_start()
with the new PMD range? I am not even sure two start with one end is legit.

[1] https://elixir.bootlin.com/linux/v6.9-rc7/source/mm/huge_memory.c#L2658
[2] https://elixir.bootlin.com/linux/v6.9-rc7/source/mm/rmap.c#L1650

--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Jason Gunthorpe]

On Wed, May 08, 2024 at 12:22:08PM -0400, Zi Yan wrote:
> On 8 May 2024, at 11:52, Jason Gunthorpe wrote:
> 
> > On Wed, May 08, 2024 at 10:56:34AM -0400, Zi Yan wrote:
> >
> >> Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole,
> >> so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw))
> >> and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked()
> >> and does not include the mmu notifier ops inside split_huge_pmd_address().
> >> I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start()
> >> before the while loop only has range of the original address and
> >> split huge pmd can affect the entire PMD address range and these two ranges
> >> might not be the same.
> >
> > That does not sound entirely good..
> >
> > I suppose it depends on what split does, if the MM page table has the
> > same translation before and after split then perhaps no invalidation
> > is even necessary.
> 
> Before split, it is a PMD mapping to a PMD THP (order-9). After split,
> they are 512 PTEs mapping to the same THP. Unless the secondary TLB
> does not support PMD mapping and use 512 PTEs instead, it seems to
> be an issue from my understanding.

I may not recall fully, but I don't think any secondaries are
so sensitive to the PMD/PTE distinction.. At least the ones using
hmm_range_fault() are not.

When the PTE eventually comes up for invalidation then the secondary
should wipe out any granual they may have captured.

Though, perhaps KVM should be checked carefully.

> In terms of two mmu_notifier ranges, first is in the split_huge_pmd_address()[1]
> and second is in try_to_unmap_one()[2]. When try_to_unmap_one() is unmapping
> a subpage in the middle of a PMD THP, the former notifies about the PMD range
> change due to one PMD split into 512 PTEs and the latter only needs to notify
> about the invalidation of the unmapped PTE. I do not think the latter can
> replace the former, although a potential optimization can be that the latter
> can be removed as it is included in the range of the former.

I think we probably don't need both, either size might be fine, but
the larger size is definately fine..

> Regarding Lance's current code change, is it OK to change mmu_notifier range
> after mmu_notifier_invalidate_range_start()? 

No, it cannot be changed during a start/stop transaction.

Jason

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Lance Yang]

Hey Zi and Jason,

Thanks a lot for reaching out!

On Thu, May 9, 2024 at 12:35 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Wed, May 08, 2024 at 12:22:08PM -0400, Zi Yan wrote:
> > On 8 May 2024, at 11:52, Jason Gunthorpe wrote:
> >
> > > On Wed, May 08, 2024 at 10:56:34AM -0400, Zi Yan wrote:
> > >
> > >> Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole,
> > >> so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw))
> > >> and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked()
> > >> and does not include the mmu notifier ops inside split_huge_pmd_address().

IMO, It might be reasonable to exclude the mmu notifier ops in
split_huge_pmd_locked(). IIUC, before acquiring the PTL, callers need to tear
down the secondary mappings via mmu_notifier_invalidate_range_start() with
the range aligned to HPAGE_PMD_SIZE.

> > >> I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start()
> > >> before the while loop only has range of the original address and
> > >> split huge pmd can affect the entire PMD address range and these two ranges
> > >> might not be the same.

As Baolin mentioned [1] before:
"For a PMD mapped THP, I think the address is already THP size alignment
returned from vma_address(&folio->page, vma)."

Given this, perhaps we don't need to re-align the input address after
starting the pagewalk? IMO, if any corner cases arise, we should catch them
by using VM_WARN_ON_ONCE() in split_huge_pmd_locked().

Zi, what do you think?

[1] https://lore.kernel.org/linux-mm/cc9fd23f-7d87-48a7-a737-acbea8e95fb7@linux.alibaba.com/

> > >
> > > That does not sound entirely good..
> > >
> > > I suppose it depends on what split does, if the MM page table has the
> > > same translation before and after split then perhaps no invalidation
> > > is even necessary.
> >
> > Before split, it is a PMD mapping to a PMD THP (order-9). After split,
> > they are 512 PTEs mapping to the same THP. Unless the secondary TLB
> > does not support PMD mapping and use 512 PTEs instead, it seems to
> > be an issue from my understanding.
>
> I may not recall fully, but I don't think any secondaries are
> so sensitive to the PMD/PTE distinction.. At least the ones using
> hmm_range_fault() are not.
>
> When the PTE eventually comes up for invalidation then the secondary
> should wipe out any granual they may have captured.
>
> Though, perhaps KVM should be checked carefully.
>
> > In terms of two mmu_notifier ranges, first is in the split_huge_pmd_address()[1]
> > and second is in try_to_unmap_one()[2]. When try_to_unmap_one() is unmapping
> > a subpage in the middle of a PMD THP, the former notifies about the PMD range
> > change due to one PMD split into 512 PTEs and the latter only needs to notify
> > about the invalidation of the unmapped PTE. I do not think the latter can
> > replace the former, although a potential optimization can be that the latter
> > can be removed as it is included in the range of the former.
>
> I think we probably don't need both, either size might be fine, but
> the larger size is definately fine..
>
> > Regarding Lance's current code change, is it OK to change mmu_notifier range
> > after mmu_notifier_invalidate_range_start()?
>
> No, it cannot be changed during a start/stop transaction.

I understood and will keep that in mind - thanks!

Thanks again for clarifying!
Lance

>
> Jason
>
>

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 3712 bytes --]

On 9 May 2024, at 4:21, Lance Yang wrote:

> Hey Zi and Jason,
>
> Thanks a lot for reaching out!
>
> On Thu, May 9, 2024 at 12:35 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>>
>> On Wed, May 08, 2024 at 12:22:08PM -0400, Zi Yan wrote:
>>> On 8 May 2024, at 11:52, Jason Gunthorpe wrote:
>>>
>>>> On Wed, May 08, 2024 at 10:56:34AM -0400, Zi Yan wrote:
>>>>
>>>>> Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole,
>>>>> so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw))
>>>>> and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked()
>>>>> and does not include the mmu notifier ops inside split_huge_pmd_address().
>
> IMO, It might be reasonable to exclude the mmu notifier ops in
> split_huge_pmd_locked(). IIUC, before acquiring the PTL, callers need to tear
> down the secondary mappings via mmu_notifier_invalidate_range_start() with
> the range aligned to HPAGE_PMD_SIZE.
>
>>>>> I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start()
>>>>> before the while loop only has range of the original address and
>>>>> split huge pmd can affect the entire PMD address range and these two ranges
>>>>> might not be the same.
>
> As Baolin mentioned [1] before:
> "For a PMD mapped THP, I think the address is already THP size alignment
> returned from vma_address(&folio->page, vma)."
>
> Given this, perhaps we don't need to re-align the input address after
> starting the pagewalk? IMO, if any corner cases arise, we should catch them
> by using VM_WARN_ON_ONCE() in split_huge_pmd_locked().
>
> Zi, what do you think?

Yes, I agree. Thanks for sorting this out.

>
> [1] https://lore.kernel.org/linux-mm/cc9fd23f-7d87-48a7-a737-acbea8e95fb7@linux.alibaba.com/
>
>>>>
>>>> That does not sound entirely good..
>>>>
>>>> I suppose it depends on what split does, if the MM page table has the
>>>> same translation before and after split then perhaps no invalidation
>>>> is even necessary.
>>>
>>> Before split, it is a PMD mapping to a PMD THP (order-9). After split,
>>> they are 512 PTEs mapping to the same THP. Unless the secondary TLB
>>> does not support PMD mapping and use 512 PTEs instead, it seems to
>>> be an issue from my understanding.
>>
>> I may not recall fully, but I don't think any secondaries are
>> so sensitive to the PMD/PTE distinction.. At least the ones using
>> hmm_range_fault() are not.
>>
>> When the PTE eventually comes up for invalidation then the secondary
>> should wipe out any granual they may have captured.
>>
>> Though, perhaps KVM should be checked carefully.
>>
>>> In terms of two mmu_notifier ranges, first is in the split_huge_pmd_address()[1]
>>> and second is in try_to_unmap_one()[2]. When try_to_unmap_one() is unmapping
>>> a subpage in the middle of a PMD THP, the former notifies about the PMD range
>>> change due to one PMD split into 512 PTEs and the latter only needs to notify
>>> about the invalidation of the unmapped PTE. I do not think the latter can
>>> replace the former, although a potential optimization can be that the latter
>>> can be removed as it is included in the range of the former.
>>
>> I think we probably don't need both, either size might be fine, but
>> the larger size is definately fine..
>>
>>> Regarding Lance's current code change, is it OK to change mmu_notifier range
>>> after mmu_notifier_invalidate_range_start()?
>>
>> No, it cannot be changed during a start/stop transaction.
>
> I understood and will keep that in mind - thanks!
>
> Thanks again for clarifying!
> Lance
>
>>
>> Jason
>>
>>


--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH v4 2/3] mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop

[Lance Yang]

Hey Jason,

Thanks a lot for clarifying!

On Thu, May 9, 2024 at 12:35 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Wed, May 08, 2024 at 12:22:08PM -0400, Zi Yan wrote:
> > On 8 May 2024, at 11:52, Jason Gunthorpe wrote:
> >
> > > On Wed, May 08, 2024 at 10:56:34AM -0400, Zi Yan wrote:
> > >
> > >> Lance is improving try_to_unmap_one() to support unmapping PMD THP as a whole,
> > >> so he moves split_huge_pmd_address() inside while (page_vma_mapped_walk(&pvmw))
> > >> and after mmu_notifier_invalidate_range_start() as split_huge_pmd_locked()
> > >> and does not include the mmu notifier ops inside split_huge_pmd_address().
> > >> I wonder if that could cause issues, since the mmu_notifier_invalidate_range_start()
> > >> before the while loop only has range of the original address and
> > >> split huge pmd can affect the entire PMD address range and these two ranges
> > >> might not be the same.
> > >
> > > That does not sound entirely good..
> > >
> > > I suppose it depends on what split does, if the MM page table has the
> > > same translation before and after split then perhaps no invalidation
> > > is even necessary.
> >
> > Before split, it is a PMD mapping to a PMD THP (order-9). After split,
> > they are 512 PTEs mapping to the same THP. Unless the secondary TLB
> > does not support PMD mapping and use 512 PTEs instead, it seems to
> > be an issue from my understanding.
>
> I may not recall fully, but I don't think any secondaries are
> so sensitive to the PMD/PTE distinction.. At least the ones using
> hmm_range_fault() are not.
>
> When the PTE eventually comes up for invalidation then the secondary
> should wipe out any granual they may have captured.
>
> Though, perhaps KVM should be checked carefully.

Agreed. IIUC,  the secondary mappings are teardown in
mmu_notifier_invalidate_range_start() and allowed to be established again
only mmu_notifier_invalidate_range_end(), then all modifications will be
picked up by the secondary MMU. I just image that the secondary MMU
like a TLB, where you only invalidate, and then it will be refilled later (after
mmu_notifier_invalidate_range_end()).

Thanks again for the lesson!
Lance

>
> > In terms of two mmu_notifier ranges, first is in the split_huge_pmd_address()[1]
> > and second is in try_to_unmap_one()[2]. When try_to_unmap_one() is unmapping
> > a subpage in the middle of a PMD THP, the former notifies about the PMD range
> > change due to one PMD split into 512 PTEs and the latter only needs to notify
> > about the invalidation of the unmapped PTE. I do not think the latter can
> > replace the former, although a potential optimization can be that the latter
> > can be removed as it is included in the range of the former.
>
> I think we probably don't need both, either size might be fine, but
> the larger size is definately fine..
>
> > Regarding Lance's current code change, is it OK to change mmu_notifier range
> > after mmu_notifier_invalidate_range_start()?
>
> No, it cannot be changed during a start/stop transaction.
>
> Jason
>
>

[PATCH v4 3/3] mm/vmscan: avoid split lazyfree THP during shrink_folio_list()

[Lance Yang]

When the user no longer requires the pages, they would use
madvise(MADV_FREE) to mark the pages as lazy free. Subsequently, they
typically would not re-write to that memory again.

During memory reclaim, if we detect that the large folio and its PMD are
both still marked as clean and there are no unexpected references
(such as GUP), so we can just discard the memory lazily, improving the
efficiency of memory reclamation in this case.  On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
mem_cgroup_force_empty() results in the following runtimes in seconds
(shorter is better):

--------------------------------------------
|     Old       |      New       |  Change  |
--------------------------------------------
|   0.683426    |    0.049197    |  -92.80% |
--------------------------------------------

Suggested-by: Zi Yan <ziy@nvidia.com>
Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Lance Yang <ioworker0@gmail.com>
---
 include/linux/huge_mm.h |  9 +++++
 mm/huge_memory.c        | 73 +++++++++++++++++++++++++++++++++++++++++
 mm/rmap.c               |  3 ++
 3 files changed, 85 insertions(+)

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 38c4b5537715..017cee864080 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -411,6 +411,8 @@ static inline bool thp_migration_supported(void)
 
 void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
 			   pmd_t *pmd, bool freeze, struct folio *folio);
+bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
+			   pmd_t *pmdp, struct folio *folio);
 
 static inline void align_huge_pmd_range(struct vm_area_struct *vma,
 					unsigned long *start,
@@ -492,6 +494,13 @@ static inline void align_huge_pmd_range(struct vm_area_struct *vma,
 					unsigned long *start,
 					unsigned long *end) {}
 
+static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
+					 unsigned long addr, pmd_t *pmdp,
+					 struct folio *folio)
+{
+	return false;
+}
+
 #define split_huge_pud(__vma, __pmd, __address)	\
 	do { } while (0)
 
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 145505a1dd05..90fdef847a88 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2690,6 +2690,79 @@ static void unmap_folio(struct folio *folio)
 	try_to_unmap_flush();
 }
 
+static bool __discard_trans_pmd_locked(struct vm_area_struct *vma,
+				       unsigned long addr, pmd_t *pmdp,
+				       struct folio *folio)
+{
+	struct mm_struct *mm = vma->vm_mm;
+	int ref_count, map_count;
+	pmd_t orig_pmd = *pmdp;
+	struct mmu_gather tlb;
+	struct page *page;
+
+	if (pmd_dirty(orig_pmd) || folio_test_dirty(folio))
+		return false;
+	if (unlikely(!pmd_present(orig_pmd) || !pmd_trans_huge(orig_pmd)))
+		return false;
+
+	page = pmd_page(orig_pmd);
+	if (unlikely(page_folio(page) != folio))
+		return false;
+
+	tlb_gather_mmu(&tlb, mm);
+	orig_pmd = pmdp_huge_get_and_clear(mm, addr, pmdp);
+	tlb_remove_pmd_tlb_entry(&tlb, pmdp, addr);
+
+	/*
+	 * Syncing against concurrent GUP-fast:
+	 * - clear PMD; barrier; read refcount
+	 * - inc refcount; barrier; read PMD
+	 */
+	smp_mb();
+
+	ref_count = folio_ref_count(folio);
+	map_count = folio_mapcount(folio);
+
+	/*
+	 * Order reads for folio refcount and dirty flag
+	 * (see comments in __remove_mapping()).
+	 */
+	smp_rmb();
+
+	/*
+	 * If the PMD or folio is redirtied at this point, or if there are
+	 * unexpected references, we will give up to discard this folio
+	 * and remap it.
+	 *
+	 * The only folio refs must be one from isolation plus the rmap(s).
+	 */
+	if (ref_count != map_count + 1 || folio_test_dirty(folio) ||
+	    pmd_dirty(orig_pmd)) {
+		set_pmd_at(mm, addr, pmdp, orig_pmd);
+		return false;
+	}
+
+	folio_remove_rmap_pmd(folio, page, vma);
+	zap_deposited_table(mm, pmdp);
+	add_mm_counter(mm, MM_ANONPAGES, -HPAGE_PMD_NR);
+	folio_put(folio);
+
+	return true;
+}
+
+bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
+			   pmd_t *pmdp, struct folio *folio)
+{
+	VM_WARN_ON_FOLIO(!folio_test_pmd_mappable(folio), folio);
+	VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
+	VM_WARN_ON_ONCE(!IS_ALIGNED(addr, HPAGE_PMD_SIZE));
+
+	if (folio_test_anon(folio) && !folio_test_swapbacked(folio))
+		return __discard_trans_pmd_locked(vma, addr, pmdp, folio);
+
+	return false;
+}
+
 static void remap_page(struct folio *folio, unsigned long nr)
 {
 	int i = 0;
diff --git a/mm/rmap.c b/mm/rmap.c
index 432601154583..1d3d30cb752c 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1675,6 +1675,9 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 		}
 
 		if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
+			if (unmap_huge_pmd_locked(vma, range.start, pvmw.pmd,
+						  folio))
+				goto walk_done;
 			/*
 			 * We temporarily have to drop the PTL and start once
 			 * again from that now-PTE-mapped page table.
-- 
2.33.1

Re: [PATCH v4 3/3] mm/vmscan: avoid split lazyfree THP during shrink_folio_list()

[Baolin Wang]

On 2024/5/1 12:27, Lance Yang wrote:
> When the user no longer requires the pages, they would use
> madvise(MADV_FREE) to mark the pages as lazy free. Subsequently, they
> typically would not re-write to that memory again.
> 
> During memory reclaim, if we detect that the large folio and its PMD are
> both still marked as clean and there are no unexpected references
> (such as GUP), so we can just discard the memory lazily, improving the
> efficiency of memory reclamation in this case.  On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
> mem_cgroup_force_empty() results in the following runtimes in seconds
> (shorter is better):
> 
> --------------------------------------------
> |     Old       |      New       |  Change  |
> --------------------------------------------
> |   0.683426    |    0.049197    |  -92.80% |
> --------------------------------------------
> 
> Suggested-by: Zi Yan <ziy@nvidia.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Lance Yang <ioworker0@gmail.com>
> ---
>   include/linux/huge_mm.h |  9 +++++
>   mm/huge_memory.c        | 73 +++++++++++++++++++++++++++++++++++++++++
>   mm/rmap.c               |  3 ++
>   3 files changed, 85 insertions(+)
> 
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index 38c4b5537715..017cee864080 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -411,6 +411,8 @@ static inline bool thp_migration_supported(void)
>   
>   void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
>   			   pmd_t *pmd, bool freeze, struct folio *folio);
> +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> +			   pmd_t *pmdp, struct folio *folio);
>   
>   static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>   					unsigned long *start,
> @@ -492,6 +494,13 @@ static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>   					unsigned long *start,
>   					unsigned long *end) {}
>   
> +static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
> +					 unsigned long addr, pmd_t *pmdp,
> +					 struct folio *folio)
> +{
> +	return false;
> +}
> +
>   #define split_huge_pud(__vma, __pmd, __address)	\
>   	do { } while (0)
>   
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 145505a1dd05..90fdef847a88 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2690,6 +2690,79 @@ static void unmap_folio(struct folio *folio)
>   	try_to_unmap_flush();
>   }
>   
> +static bool __discard_trans_pmd_locked(struct vm_area_struct *vma,
> +				       unsigned long addr, pmd_t *pmdp,
> +				       struct folio *folio)
> +{
> +	struct mm_struct *mm = vma->vm_mm;
> +	int ref_count, map_count;
> +	pmd_t orig_pmd = *pmdp;
> +	struct mmu_gather tlb;
> +	struct page *page;
> +
> +	if (pmd_dirty(orig_pmd) || folio_test_dirty(folio))
> +		return false;
> +	if (unlikely(!pmd_present(orig_pmd) || !pmd_trans_huge(orig_pmd)))
> +		return false;
> +
> +	page = pmd_page(orig_pmd);
> +	if (unlikely(page_folio(page) != folio))
> +		return false;
> +
> +	tlb_gather_mmu(&tlb, mm);
> +	orig_pmd = pmdp_huge_get_and_clear(mm, addr, pmdp);
> +	tlb_remove_pmd_tlb_entry(&tlb, pmdp, addr);
> +
> +	/*
> +	 * Syncing against concurrent GUP-fast:
> +	 * - clear PMD; barrier; read refcount
> +	 * - inc refcount; barrier; read PMD
> +	 */
> +	smp_mb();
> +
> +	ref_count = folio_ref_count(folio);
> +	map_count = folio_mapcount(folio);
> +
> +	/*
> +	 * Order reads for folio refcount and dirty flag
> +	 * (see comments in __remove_mapping()).
> +	 */
> +	smp_rmb();
> +
> +	/*
> +	 * If the PMD or folio is redirtied at this point, or if there are
> +	 * unexpected references, we will give up to discard this folio
> +	 * and remap it.
> +	 *
> +	 * The only folio refs must be one from isolation plus the rmap(s).
> +	 */
> +	if (ref_count != map_count + 1 || folio_test_dirty(folio) ||
> +	    pmd_dirty(orig_pmd)) {
> +		set_pmd_at(mm, addr, pmdp, orig_pmd);
> +		return false;
> +	}
> +
> +	folio_remove_rmap_pmd(folio, page, vma);
> +	zap_deposited_table(mm, pmdp);
> +	add_mm_counter(mm, MM_ANONPAGES, -HPAGE_PMD_NR);
> +	folio_put(folio);

IIUC, you missed handling mlock vma, see mlock_drain_local() in 
try_to_unmap_one().

> +
> +	return true;
> +}
> +
> +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> +			   pmd_t *pmdp, struct folio *folio)
> +{
> +	VM_WARN_ON_FOLIO(!folio_test_pmd_mappable(folio), folio);
> +	VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
> +	VM_WARN_ON_ONCE(!IS_ALIGNED(addr, HPAGE_PMD_SIZE));
> +
> +	if (folio_test_anon(folio) && !folio_test_swapbacked(folio))
> +		return __discard_trans_pmd_locked(vma, addr, pmdp, folio);

Why add a new function, which is only used by unmap_huge_pmd_locked()? 
Seems can be folded in unmap_huge_pmd_locked(), but not a strong 
preference:)

> +
> +	return false;
> +}
> +
>   static void remap_page(struct folio *folio, unsigned long nr)
>   {
>   	int i = 0;
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 432601154583..1d3d30cb752c 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1675,6 +1675,9 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>   		}
>   
>   		if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
> +			if (unmap_huge_pmd_locked(vma, range.start, pvmw.pmd,
> +						  folio))
> +				goto walk_done;
>   			/*
>   			 * We temporarily have to drop the PTL and start once
>   			 * again from that now-PTE-mapped page table.

Re: [PATCH v4 3/3] mm/vmscan: avoid split lazyfree THP during shrink_folio_list()

[Lance Yang]

Hey Baolin,

Thanks a lot for taking time to review!

On Tue, May 7, 2024 at 12:01 PM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 2024/5/1 12:27, Lance Yang wrote:
> > When the user no longer requires the pages, they would use
> > madvise(MADV_FREE) to mark the pages as lazy free. Subsequently, they
> > typically would not re-write to that memory again.
> >
> > During memory reclaim, if we detect that the large folio and its PMD are
> > both still marked as clean and there are no unexpected references
> > (such as GUP), so we can just discard the memory lazily, improving the
> > efficiency of memory reclamation in this case.  On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
> > mem_cgroup_force_empty() results in the following runtimes in seconds
> > (shorter is better):
> >
> > --------------------------------------------
> > |     Old       |      New       |  Change  |
> > --------------------------------------------
> > |   0.683426    |    0.049197    |  -92.80% |
> > --------------------------------------------
> >
> > Suggested-by: Zi Yan <ziy@nvidia.com>
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Lance Yang <ioworker0@gmail.com>
> > ---
> >   include/linux/huge_mm.h |  9 +++++
> >   mm/huge_memory.c        | 73 +++++++++++++++++++++++++++++++++++++++++
> >   mm/rmap.c               |  3 ++
> >   3 files changed, 85 insertions(+)
> >
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index 38c4b5537715..017cee864080 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -411,6 +411,8 @@ static inline bool thp_migration_supported(void)
> >
> >   void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> >                          pmd_t *pmd, bool freeze, struct folio *folio);
> > +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> > +                        pmd_t *pmdp, struct folio *folio);
> >
> >   static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> >                                       unsigned long *start,
> > @@ -492,6 +494,13 @@ static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> >                                       unsigned long *start,
> >                                       unsigned long *end) {}
> >
> > +static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
> > +                                      unsigned long addr, pmd_t *pmdp,
> > +                                      struct folio *folio)
> > +{
> > +     return false;
> > +}
> > +
> >   #define split_huge_pud(__vma, __pmd, __address)     \
> >       do { } while (0)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 145505a1dd05..90fdef847a88 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2690,6 +2690,79 @@ static void unmap_folio(struct folio *folio)
> >       try_to_unmap_flush();
> >   }
> >
> > +static bool __discard_trans_pmd_locked(struct vm_area_struct *vma,
> > +                                    unsigned long addr, pmd_t *pmdp,
> > +                                    struct folio *folio)
> > +{
> > +     struct mm_struct *mm = vma->vm_mm;
> > +     int ref_count, map_count;
> > +     pmd_t orig_pmd = *pmdp;
> > +     struct mmu_gather tlb;
> > +     struct page *page;
> > +
> > +     if (pmd_dirty(orig_pmd) || folio_test_dirty(folio))
> > +             return false;
> > +     if (unlikely(!pmd_present(orig_pmd) || !pmd_trans_huge(orig_pmd)))
> > +             return false;
> > +
> > +     page = pmd_page(orig_pmd);
> > +     if (unlikely(page_folio(page) != folio))
> > +             return false;
> > +
> > +     tlb_gather_mmu(&tlb, mm);
> > +     orig_pmd = pmdp_huge_get_and_clear(mm, addr, pmdp);
> > +     tlb_remove_pmd_tlb_entry(&tlb, pmdp, addr);
> > +
> > +     /*
> > +      * Syncing against concurrent GUP-fast:
> > +      * - clear PMD; barrier; read refcount
> > +      * - inc refcount; barrier; read PMD
> > +      */
> > +     smp_mb();
> > +
> > +     ref_count = folio_ref_count(folio);
> > +     map_count = folio_mapcount(folio);
> > +
> > +     /*
> > +      * Order reads for folio refcount and dirty flag
> > +      * (see comments in __remove_mapping()).
> > +      */
> > +     smp_rmb();
> > +
> > +     /*
> > +      * If the PMD or folio is redirtied at this point, or if there are
> > +      * unexpected references, we will give up to discard this folio
> > +      * and remap it.
> > +      *
> > +      * The only folio refs must be one from isolation plus the rmap(s).
> > +      */
> > +     if (ref_count != map_count + 1 || folio_test_dirty(folio) ||
> > +         pmd_dirty(orig_pmd)) {
> > +             set_pmd_at(mm, addr, pmdp, orig_pmd);
> > +             return false;
> > +     }
> > +
> > +     folio_remove_rmap_pmd(folio, page, vma);
> > +     zap_deposited_table(mm, pmdp);
> > +     add_mm_counter(mm, MM_ANONPAGES, -HPAGE_PMD_NR);
> > +     folio_put(folio);
>
> IIUC, you missed handling mlock vma, see mlock_drain_local() in
> try_to_unmap_one().

Good spot!

I suddenly realized that I overlooked another thing: If we detect that a
PMD-mapped THP is within the range of the VM_LOCKED VMA, we
should check whether the TTU_IGNORE_MLOCK flag is set in
try_to_unmap_one(). If the flag is set, we will remove the PMD mapping
from the folio. Otherwise, the folio should be mlocked, which avoids
splitting the folio and then mlocking each page again.

What do you think?

>
> > +
> > +     return true;
> > +}
> > +
> > +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> > +                        pmd_t *pmdp, struct folio *folio)
> > +{
> > +     VM_WARN_ON_FOLIO(!folio_test_pmd_mappable(folio), folio);
> > +     VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
> > +     VM_WARN_ON_ONCE(!IS_ALIGNED(addr, HPAGE_PMD_SIZE));
> > +
> > +     if (folio_test_anon(folio) && !folio_test_swapbacked(folio))
> > +             return __discard_trans_pmd_locked(vma, addr, pmdp, folio);
>
> Why add a new function, which is only used by unmap_huge_pmd_locked()?
> Seems can be folded in unmap_huge_pmd_locked(), but not a strong
> preference:)

Thanks for the suggestion!

Personally, I prefer adding a new function, rather than folding
__discard_trans_pmd_locked() into unmap_huge_pmd_locked().

While unmap_huge_pmd_locked() currently only deals with lazyfree THP,
It could be expanded to support other types of large folios that are
PMD-mapped in the future if needed.

Thanks a lot again for the review!
Lance

>
> > +
> > +     return false;
> > +}
> > +
> >   static void remap_page(struct folio *folio, unsigned long nr)
> >   {
> >       int i = 0;
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index 432601154583..1d3d30cb752c 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1675,6 +1675,9 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >               }
> >
> >               if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
> > +                     if (unmap_huge_pmd_locked(vma, range.start, pvmw.pmd,
> > +                                               folio))
> > +                             goto walk_done;
> >                       /*
> >                        * We temporarily have to drop the PTL and start once
> >                        * again from that now-PTE-mapped page table.

Re: [PATCH v4 3/3] mm/vmscan: avoid split lazyfree THP during shrink_folio_list()

[Lance Yang]

On Tue, May 7, 2024 at 2:32 PM Lance Yang <ioworker0@gmail.com> wrote:
>
> Hey Baolin,
>
> Thanks a lot for taking time to review!
>
> On Tue, May 7, 2024 at 12:01 PM Baolin Wang
> <baolin.wang@linux.alibaba.com> wrote:
> >
> >
> >
> > On 2024/5/1 12:27, Lance Yang wrote:
> > > When the user no longer requires the pages, they would use
> > > madvise(MADV_FREE) to mark the pages as lazy free. Subsequently, they
> > > typically would not re-write to that memory again.
> > >
> > > During memory reclaim, if we detect that the large folio and its PMD are
> > > both still marked as clean and there are no unexpected references
> > > (such as GUP), so we can just discard the memory lazily, improving the
> > > efficiency of memory reclamation in this case.  On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
> > > mem_cgroup_force_empty() results in the following runtimes in seconds
> > > (shorter is better):
> > >
> > > --------------------------------------------
> > > |     Old       |      New       |  Change  |
> > > --------------------------------------------
> > > |   0.683426    |    0.049197    |  -92.80% |
> > > --------------------------------------------
> > >
> > > Suggested-by: Zi Yan <ziy@nvidia.com>
> > > Suggested-by: David Hildenbrand <david@redhat.com>
> > > Signed-off-by: Lance Yang <ioworker0@gmail.com>
> > > ---
> > >   include/linux/huge_mm.h |  9 +++++
> > >   mm/huge_memory.c        | 73 +++++++++++++++++++++++++++++++++++++++++
> > >   mm/rmap.c               |  3 ++
> > >   3 files changed, 85 insertions(+)
> > >
> > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > > index 38c4b5537715..017cee864080 100644
> > > --- a/include/linux/huge_mm.h
> > > +++ b/include/linux/huge_mm.h
> > > @@ -411,6 +411,8 @@ static inline bool thp_migration_supported(void)
> > >
> > >   void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> > >                          pmd_t *pmd, bool freeze, struct folio *folio);
> > > +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> > > +                        pmd_t *pmdp, struct folio *folio);
> > >
> > >   static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> > >                                       unsigned long *start,
> > > @@ -492,6 +494,13 @@ static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> > >                                       unsigned long *start,
> > >                                       unsigned long *end) {}
> > >
> > > +static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
> > > +                                      unsigned long addr, pmd_t *pmdp,
> > > +                                      struct folio *folio)
> > > +{
> > > +     return false;
> > > +}
> > > +
> > >   #define split_huge_pud(__vma, __pmd, __address)     \
> > >       do { } while (0)
> > >
> > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > index 145505a1dd05..90fdef847a88 100644
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -2690,6 +2690,79 @@ static void unmap_folio(struct folio *folio)
> > >       try_to_unmap_flush();
> > >   }
> > >
> > > +static bool __discard_trans_pmd_locked(struct vm_area_struct *vma,
> > > +                                    unsigned long addr, pmd_t *pmdp,
> > > +                                    struct folio *folio)
> > > +{
> > > +     struct mm_struct *mm = vma->vm_mm;
> > > +     int ref_count, map_count;
> > > +     pmd_t orig_pmd = *pmdp;
> > > +     struct mmu_gather tlb;
> > > +     struct page *page;
> > > +
> > > +     if (pmd_dirty(orig_pmd) || folio_test_dirty(folio))
> > > +             return false;
> > > +     if (unlikely(!pmd_present(orig_pmd) || !pmd_trans_huge(orig_pmd)))
> > > +             return false;
> > > +
> > > +     page = pmd_page(orig_pmd);
> > > +     if (unlikely(page_folio(page) != folio))
> > > +             return false;
> > > +
> > > +     tlb_gather_mmu(&tlb, mm);
> > > +     orig_pmd = pmdp_huge_get_and_clear(mm, addr, pmdp);
> > > +     tlb_remove_pmd_tlb_entry(&tlb, pmdp, addr);
> > > +
> > > +     /*
> > > +      * Syncing against concurrent GUP-fast:
> > > +      * - clear PMD; barrier; read refcount
> > > +      * - inc refcount; barrier; read PMD
> > > +      */
> > > +     smp_mb();
> > > +
> > > +     ref_count = folio_ref_count(folio);
> > > +     map_count = folio_mapcount(folio);
> > > +
> > > +     /*
> > > +      * Order reads for folio refcount and dirty flag
> > > +      * (see comments in __remove_mapping()).
> > > +      */
> > > +     smp_rmb();
> > > +
> > > +     /*
> > > +      * If the PMD or folio is redirtied at this point, or if there are
> > > +      * unexpected references, we will give up to discard this folio
> > > +      * and remap it.
> > > +      *
> > > +      * The only folio refs must be one from isolation plus the rmap(s).
> > > +      */
> > > +     if (ref_count != map_count + 1 || folio_test_dirty(folio) ||
> > > +         pmd_dirty(orig_pmd)) {
> > > +             set_pmd_at(mm, addr, pmdp, orig_pmd);
> > > +             return false;
> > > +     }
> > > +
> > > +     folio_remove_rmap_pmd(folio, page, vma);
> > > +     zap_deposited_table(mm, pmdp);
> > > +     add_mm_counter(mm, MM_ANONPAGES, -HPAGE_PMD_NR);
> > > +     folio_put(folio);
> >
> > IIUC, you missed handling mlock vma, see mlock_drain_local() in
> > try_to_unmap_one().
>
> Good spot!
>
> I suddenly realized that I overlooked another thing: If we detect that a
> PMD-mapped THP is within the range of the VM_LOCKED VMA, we
> should check whether the TTU_IGNORE_MLOCK flag is set in
> try_to_unmap_one(). If the flag is set, we will remove the PMD mapping
> from the folio. Otherwise, the folio should be mlocked, which avoids
> splitting the folio and then mlocking each page again.

My previous response above is flawed - sorry :(

If we detect that a PMD-mapped THP is within the range of the
VM_LOCKED VMA.

1) If the TTU_IGNORE_MLOCK flag is set, we will try to remove the
PMD mapping from the folio, as this series has done.

2) If the flag is not set, the large folio should be mlocked to prevent it
from being picked during memory reclaim? Currently, we just leave it
as is and do not to mlock it, IIUC.

What do you think?

Thanks,
Lance

>
> What do you think?
>
> >
> > > +
> > > +     return true;
> > > +}
> > > +
> > > +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> > > +                        pmd_t *pmdp, struct folio *folio)
> > > +{
> > > +     VM_WARN_ON_FOLIO(!folio_test_pmd_mappable(folio), folio);
> > > +     VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
> > > +     VM_WARN_ON_ONCE(!IS_ALIGNED(addr, HPAGE_PMD_SIZE));
> > > +
> > > +     if (folio_test_anon(folio) && !folio_test_swapbacked(folio))
> > > +             return __discard_trans_pmd_locked(vma, addr, pmdp, folio);
> >
> > Why add a new function, which is only used by unmap_huge_pmd_locked()?
> > Seems can be folded in unmap_huge_pmd_locked(), but not a strong
> > preference:)
>
> Thanks for the suggestion!
>
> Personally, I prefer adding a new function, rather than folding
> __discard_trans_pmd_locked() into unmap_huge_pmd_locked().
>
> While unmap_huge_pmd_locked() currently only deals with lazyfree THP,
> It could be expanded to support other types of large folios that are
> PMD-mapped in the future if needed.
>
> Thanks a lot again for the review!
> Lance
>
> >
> > > +
> > > +     return false;
> > > +}
> > > +
> > >   static void remap_page(struct folio *folio, unsigned long nr)
> > >   {
> > >       int i = 0;
> > > diff --git a/mm/rmap.c b/mm/rmap.c
> > > index 432601154583..1d3d30cb752c 100644
> > > --- a/mm/rmap.c
> > > +++ b/mm/rmap.c
> > > @@ -1675,6 +1675,9 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> > >               }
> > >
> > >               if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
> > > +                     if (unmap_huge_pmd_locked(vma, range.start, pvmw.pmd,
> > > +                                               folio))
> > > +                             goto walk_done;
> > >                       /*
> > >                        * We temporarily have to drop the PTL and start once
> > >                        * again from that now-PTE-mapped page table.

Re: [PATCH v4 3/3] mm/vmscan: avoid split lazyfree THP during shrink_folio_list()

[Baolin Wang]

On 2024/5/7 16:26, Lance Yang wrote:
> On Tue, May 7, 2024 at 2:32 PM Lance Yang <ioworker0@gmail.com> wrote:
>>
>> Hey Baolin,
>>
>> Thanks a lot for taking time to review!
>>
>> On Tue, May 7, 2024 at 12:01 PM Baolin Wang
>> <baolin.wang@linux.alibaba.com> wrote:
>>>
>>>
>>>
>>> On 2024/5/1 12:27, Lance Yang wrote:
>>>> When the user no longer requires the pages, they would use
>>>> madvise(MADV_FREE) to mark the pages as lazy free. Subsequently, they
>>>> typically would not re-write to that memory again.
>>>>
>>>> During memory reclaim, if we detect that the large folio and its PMD are
>>>> both still marked as clean and there are no unexpected references
>>>> (such as GUP), so we can just discard the memory lazily, improving the
>>>> efficiency of memory reclamation in this case.  On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
>>>> mem_cgroup_force_empty() results in the following runtimes in seconds
>>>> (shorter is better):
>>>>
>>>> --------------------------------------------
>>>> |     Old       |      New       |  Change  |
>>>> --------------------------------------------
>>>> |   0.683426    |    0.049197    |  -92.80% |
>>>> --------------------------------------------
>>>>
>>>> Suggested-by: Zi Yan <ziy@nvidia.com>
>>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>>> Signed-off-by: Lance Yang <ioworker0@gmail.com>
>>>> ---
>>>>    include/linux/huge_mm.h |  9 +++++
>>>>    mm/huge_memory.c        | 73 +++++++++++++++++++++++++++++++++++++++++
>>>>    mm/rmap.c               |  3 ++
>>>>    3 files changed, 85 insertions(+)
>>>>
>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>> index 38c4b5537715..017cee864080 100644
>>>> --- a/include/linux/huge_mm.h
>>>> +++ b/include/linux/huge_mm.h
>>>> @@ -411,6 +411,8 @@ static inline bool thp_migration_supported(void)
>>>>
>>>>    void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
>>>>                           pmd_t *pmd, bool freeze, struct folio *folio);
>>>> +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
>>>> +                        pmd_t *pmdp, struct folio *folio);
>>>>
>>>>    static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>>>>                                        unsigned long *start,
>>>> @@ -492,6 +494,13 @@ static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>>>>                                        unsigned long *start,
>>>>                                        unsigned long *end) {}
>>>>
>>>> +static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
>>>> +                                      unsigned long addr, pmd_t *pmdp,
>>>> +                                      struct folio *folio)
>>>> +{
>>>> +     return false;
>>>> +}
>>>> +
>>>>    #define split_huge_pud(__vma, __pmd, __address)     \
>>>>        do { } while (0)
>>>>
>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>> index 145505a1dd05..90fdef847a88 100644
>>>> --- a/mm/huge_memory.c
>>>> +++ b/mm/huge_memory.c
>>>> @@ -2690,6 +2690,79 @@ static void unmap_folio(struct folio *folio)
>>>>        try_to_unmap_flush();
>>>>    }
>>>>
>>>> +static bool __discard_trans_pmd_locked(struct vm_area_struct *vma,
>>>> +                                    unsigned long addr, pmd_t *pmdp,
>>>> +                                    struct folio *folio)
>>>> +{
>>>> +     struct mm_struct *mm = vma->vm_mm;
>>>> +     int ref_count, map_count;
>>>> +     pmd_t orig_pmd = *pmdp;
>>>> +     struct mmu_gather tlb;
>>>> +     struct page *page;
>>>> +
>>>> +     if (pmd_dirty(orig_pmd) || folio_test_dirty(folio))
>>>> +             return false;
>>>> +     if (unlikely(!pmd_present(orig_pmd) || !pmd_trans_huge(orig_pmd)))
>>>> +             return false;
>>>> +
>>>> +     page = pmd_page(orig_pmd);
>>>> +     if (unlikely(page_folio(page) != folio))
>>>> +             return false;
>>>> +
>>>> +     tlb_gather_mmu(&tlb, mm);
>>>> +     orig_pmd = pmdp_huge_get_and_clear(mm, addr, pmdp);
>>>> +     tlb_remove_pmd_tlb_entry(&tlb, pmdp, addr);
>>>> +
>>>> +     /*
>>>> +      * Syncing against concurrent GUP-fast:
>>>> +      * - clear PMD; barrier; read refcount
>>>> +      * - inc refcount; barrier; read PMD
>>>> +      */
>>>> +     smp_mb();
>>>> +
>>>> +     ref_count = folio_ref_count(folio);
>>>> +     map_count = folio_mapcount(folio);
>>>> +
>>>> +     /*
>>>> +      * Order reads for folio refcount and dirty flag
>>>> +      * (see comments in __remove_mapping()).
>>>> +      */
>>>> +     smp_rmb();
>>>> +
>>>> +     /*
>>>> +      * If the PMD or folio is redirtied at this point, or if there are
>>>> +      * unexpected references, we will give up to discard this folio
>>>> +      * and remap it.
>>>> +      *
>>>> +      * The only folio refs must be one from isolation plus the rmap(s).
>>>> +      */
>>>> +     if (ref_count != map_count + 1 || folio_test_dirty(folio) ||
>>>> +         pmd_dirty(orig_pmd)) {
>>>> +             set_pmd_at(mm, addr, pmdp, orig_pmd);
>>>> +             return false;
>>>> +     }
>>>> +
>>>> +     folio_remove_rmap_pmd(folio, page, vma);
>>>> +     zap_deposited_table(mm, pmdp);
>>>> +     add_mm_counter(mm, MM_ANONPAGES, -HPAGE_PMD_NR);
>>>> +     folio_put(folio);
>>>
>>> IIUC, you missed handling mlock vma, see mlock_drain_local() in
>>> try_to_unmap_one().
>>
>> Good spot!
>>
>> I suddenly realized that I overlooked another thing: If we detect that a
>> PMD-mapped THP is within the range of the VM_LOCKED VMA, we
>> should check whether the TTU_IGNORE_MLOCK flag is set in
>> try_to_unmap_one(). If the flag is set, we will remove the PMD mapping
>> from the folio. Otherwise, the folio should be mlocked, which avoids
>> splitting the folio and then mlocking each page again.
> 
> My previous response above is flawed - sorry :(
> 
> If we detect that a PMD-mapped THP is within the range of the
> VM_LOCKED VMA.
> 
> 1) If the TTU_IGNORE_MLOCK flag is set, we will try to remove the
> PMD mapping from the folio, as this series has done.

Right.

> 2) If the flag is not set, the large folio should be mlocked to prevent it
> from being picked during memory reclaim? Currently, we just leave it

Yes. From commit 1acbc3f93614 ("mm: handle large folio when large folio 
in VM_LOCKED VMA range"), large folios of the mlocked VMA will be 
handled during page reclaim phase.

> as is and do not to mlock it, IIUC.

Original code already handle the mlock case after the PMD-mapped THP is 
split in try_to_unmap_one():
                 /*
                  * If the folio is in an mlock()d vma, we must not swap 
it out.
                  */
                 if (!(flags & TTU_IGNORE_MLOCK) &&
                     (vma->vm_flags & VM_LOCKED)) {
                         /* Restore the mlock which got missed */
                         if (!folio_test_large(folio))
                                 mlock_vma_folio(folio, vma);
                         page_vma_mapped_walk_done(&pvmw);
                         ret = false;
                         break;
                 }

Re: [PATCH v4 3/3] mm/vmscan: avoid split lazyfree THP during shrink_folio_list()

[Lance Yang]

On Tue, May 7, 2024 at 5:33 PM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 2024/5/7 16:26, Lance Yang wrote:
> > On Tue, May 7, 2024 at 2:32 PM Lance Yang <ioworker0@gmail.com> wrote:
> >>
> >> Hey Baolin,
> >>
> >> Thanks a lot for taking time to review!
> >>
> >> On Tue, May 7, 2024 at 12:01 PM Baolin Wang
> >> <baolin.wang@linux.alibaba.com> wrote:
> >>>
> >>>
> >>>
> >>> On 2024/5/1 12:27, Lance Yang wrote:
> >>>> When the user no longer requires the pages, they would use
> >>>> madvise(MADV_FREE) to mark the pages as lazy free. Subsequently, they
> >>>> typically would not re-write to that memory again.
> >>>>
> >>>> During memory reclaim, if we detect that the large folio and its PMD are
> >>>> both still marked as clean and there are no unexpected references
> >>>> (such as GUP), so we can just discard the memory lazily, improving the
> >>>> efficiency of memory reclamation in this case.  On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
> >>>> mem_cgroup_force_empty() results in the following runtimes in seconds
> >>>> (shorter is better):
> >>>>
> >>>> --------------------------------------------
> >>>> |     Old       |      New       |  Change  |
> >>>> --------------------------------------------
> >>>> |   0.683426    |    0.049197    |  -92.80% |
> >>>> --------------------------------------------
> >>>>
> >>>> Suggested-by: Zi Yan <ziy@nvidia.com>
> >>>> Suggested-by: David Hildenbrand <david@redhat.com>
> >>>> Signed-off-by: Lance Yang <ioworker0@gmail.com>
> >>>> ---
> >>>>    include/linux/huge_mm.h |  9 +++++
> >>>>    mm/huge_memory.c        | 73 +++++++++++++++++++++++++++++++++++++++++
> >>>>    mm/rmap.c               |  3 ++
> >>>>    3 files changed, 85 insertions(+)
> >>>>
> >>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >>>> index 38c4b5537715..017cee864080 100644
> >>>> --- a/include/linux/huge_mm.h
> >>>> +++ b/include/linux/huge_mm.h
> >>>> @@ -411,6 +411,8 @@ static inline bool thp_migration_supported(void)
> >>>>
> >>>>    void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> >>>>                           pmd_t *pmd, bool freeze, struct folio *folio);
> >>>> +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> >>>> +                        pmd_t *pmdp, struct folio *folio);
> >>>>
> >>>>    static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> >>>>                                        unsigned long *start,
> >>>> @@ -492,6 +494,13 @@ static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> >>>>                                        unsigned long *start,
> >>>>                                        unsigned long *end) {}
> >>>>
> >>>> +static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
> >>>> +                                      unsigned long addr, pmd_t *pmdp,
> >>>> +                                      struct folio *folio)
> >>>> +{
> >>>> +     return false;
> >>>> +}
> >>>> +
> >>>>    #define split_huge_pud(__vma, __pmd, __address)     \
> >>>>        do { } while (0)
> >>>>
> >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >>>> index 145505a1dd05..90fdef847a88 100644
> >>>> --- a/mm/huge_memory.c
> >>>> +++ b/mm/huge_memory.c
> >>>> @@ -2690,6 +2690,79 @@ static void unmap_folio(struct folio *folio)
> >>>>        try_to_unmap_flush();
> >>>>    }
> >>>>
> >>>> +static bool __discard_trans_pmd_locked(struct vm_area_struct *vma,
> >>>> +                                    unsigned long addr, pmd_t *pmdp,
> >>>> +                                    struct folio *folio)
> >>>> +{
> >>>> +     struct mm_struct *mm = vma->vm_mm;
> >>>> +     int ref_count, map_count;
> >>>> +     pmd_t orig_pmd = *pmdp;
> >>>> +     struct mmu_gather tlb;
> >>>> +     struct page *page;
> >>>> +
> >>>> +     if (pmd_dirty(orig_pmd) || folio_test_dirty(folio))
> >>>> +             return false;
> >>>> +     if (unlikely(!pmd_present(orig_pmd) || !pmd_trans_huge(orig_pmd)))
> >>>> +             return false;
> >>>> +
> >>>> +     page = pmd_page(orig_pmd);
> >>>> +     if (unlikely(page_folio(page) != folio))
> >>>> +             return false;
> >>>> +
> >>>> +     tlb_gather_mmu(&tlb, mm);
> >>>> +     orig_pmd = pmdp_huge_get_and_clear(mm, addr, pmdp);
> >>>> +     tlb_remove_pmd_tlb_entry(&tlb, pmdp, addr);
> >>>> +
> >>>> +     /*
> >>>> +      * Syncing against concurrent GUP-fast:
> >>>> +      * - clear PMD; barrier; read refcount
> >>>> +      * - inc refcount; barrier; read PMD
> >>>> +      */
> >>>> +     smp_mb();
> >>>> +
> >>>> +     ref_count = folio_ref_count(folio);
> >>>> +     map_count = folio_mapcount(folio);
> >>>> +
> >>>> +     /*
> >>>> +      * Order reads for folio refcount and dirty flag
> >>>> +      * (see comments in __remove_mapping()).
> >>>> +      */
> >>>> +     smp_rmb();
> >>>> +
> >>>> +     /*
> >>>> +      * If the PMD or folio is redirtied at this point, or if there are
> >>>> +      * unexpected references, we will give up to discard this folio
> >>>> +      * and remap it.
> >>>> +      *
> >>>> +      * The only folio refs must be one from isolation plus the rmap(s).
> >>>> +      */
> >>>> +     if (ref_count != map_count + 1 || folio_test_dirty(folio) ||
> >>>> +         pmd_dirty(orig_pmd)) {
> >>>> +             set_pmd_at(mm, addr, pmdp, orig_pmd);
> >>>> +             return false;
> >>>> +     }
> >>>> +
> >>>> +     folio_remove_rmap_pmd(folio, page, vma);
> >>>> +     zap_deposited_table(mm, pmdp);
> >>>> +     add_mm_counter(mm, MM_ANONPAGES, -HPAGE_PMD_NR);
> >>>> +     folio_put(folio);
> >>>
> >>> IIUC, you missed handling mlock vma, see mlock_drain_local() in
> >>> try_to_unmap_one().
> >>
> >> Good spot!
> >>
> >> I suddenly realized that I overlooked another thing: If we detect that a
> >> PMD-mapped THP is within the range of the VM_LOCKED VMA, we
> >> should check whether the TTU_IGNORE_MLOCK flag is set in
> >> try_to_unmap_one(). If the flag is set, we will remove the PMD mapping
> >> from the folio. Otherwise, the folio should be mlocked, which avoids
> >> splitting the folio and then mlocking each page again.
> >
> > My previous response above is flawed - sorry :(
> >
> > If we detect that a PMD-mapped THP is within the range of the
> > VM_LOCKED VMA.
> >
> > 1) If the TTU_IGNORE_MLOCK flag is set, we will try to remove the
> > PMD mapping from the folio, as this series has done.
>
> Right.
>
> > 2) If the flag is not set, the large folio should be mlocked to prevent it
> > from being picked during memory reclaim? Currently, we just leave it
>
> Yes. From commit 1acbc3f93614 ("mm: handle large folio when large folio
> in VM_LOCKED VMA range"), large folios of the mlocked VMA will be
> handled during page reclaim phase.
>
> > as is and do not to mlock it, IIUC.
>
> Original code already handle the mlock case after the PMD-mapped THP is
> split in try_to_unmap_one():

Yep. But this series doesn't do the TTU_SPLIT_HUGE_PMD immediately.

>                  /*
>                   * If the folio is in an mlock()d vma, we must not swap
> it out.
>                   */
>                  if (!(flags & TTU_IGNORE_MLOCK) &&
>                      (vma->vm_flags & VM_LOCKED)) {
>                          /* Restore the mlock which got missed */

IIUC, we could detect a PMD-mapped THP here. So, I'm not sure if we
need to mlock it to prevent it from being picked again during memory
reclaim. The change is as follows:

diff --git a/mm/rmap.c b/mm/rmap.c
index ed7f82036986..2a9d037ab23c 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1673,7 +1673,8 @@ static bool try_to_unmap_one(struct folio
*folio, struct vm_area_struct *vma,
                if (!(flags & TTU_IGNORE_MLOCK) &&
                    (vma->vm_flags & VM_LOCKED)) {
                        /* Restore the mlock which got missed */
-                       if (!folio_test_large(folio))
+                       if (!folio_test_large(folio) ||
+                           (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)))
                                mlock_vma_folio(folio, vma);
                        goto walk_done_err;
                }

Thanks,
Lance


>                          if (!folio_test_large(folio))
>                                  mlock_vma_folio(folio, vma);
>                          page_vma_mapped_walk_done(&pvmw);
>                          ret = false;
>                          break;
>                  }

Re: [PATCH v4 3/3] mm/vmscan: avoid split lazyfree THP during shrink_folio_list()

[Baolin Wang]

On 2024/5/7 19:37, Lance Yang wrote:
> On Tue, May 7, 2024 at 5:33 PM Baolin Wang
> <baolin.wang@linux.alibaba.com> wrote:
>>
>>
>>
>> On 2024/5/7 16:26, Lance Yang wrote:
>>> On Tue, May 7, 2024 at 2:32 PM Lance Yang <ioworker0@gmail.com> wrote:
>>>>
>>>> Hey Baolin,
>>>>
>>>> Thanks a lot for taking time to review!
>>>>
>>>> On Tue, May 7, 2024 at 12:01 PM Baolin Wang
>>>> <baolin.wang@linux.alibaba.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 2024/5/1 12:27, Lance Yang wrote:
>>>>>> When the user no longer requires the pages, they would use
>>>>>> madvise(MADV_FREE) to mark the pages as lazy free. Subsequently, they
>>>>>> typically would not re-write to that memory again.
>>>>>>
>>>>>> During memory reclaim, if we detect that the large folio and its PMD are
>>>>>> both still marked as clean and there are no unexpected references
>>>>>> (such as GUP), so we can just discard the memory lazily, improving the
>>>>>> efficiency of memory reclamation in this case.  On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
>>>>>> mem_cgroup_force_empty() results in the following runtimes in seconds
>>>>>> (shorter is better):
>>>>>>
>>>>>> --------------------------------------------
>>>>>> |     Old       |      New       |  Change  |
>>>>>> --------------------------------------------
>>>>>> |   0.683426    |    0.049197    |  -92.80% |
>>>>>> --------------------------------------------
>>>>>>
>>>>>> Suggested-by: Zi Yan <ziy@nvidia.com>
>>>>>> Suggested-by: David Hildenbrand <david@redhat.com>
>>>>>> Signed-off-by: Lance Yang <ioworker0@gmail.com>
>>>>>> ---
>>>>>>     include/linux/huge_mm.h |  9 +++++
>>>>>>     mm/huge_memory.c        | 73 +++++++++++++++++++++++++++++++++++++++++
>>>>>>     mm/rmap.c               |  3 ++
>>>>>>     3 files changed, 85 insertions(+)
>>>>>>
>>>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>>>> index 38c4b5537715..017cee864080 100644
>>>>>> --- a/include/linux/huge_mm.h
>>>>>> +++ b/include/linux/huge_mm.h
>>>>>> @@ -411,6 +411,8 @@ static inline bool thp_migration_supported(void)
>>>>>>
>>>>>>     void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
>>>>>>                            pmd_t *pmd, bool freeze, struct folio *folio);
>>>>>> +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
>>>>>> +                        pmd_t *pmdp, struct folio *folio);
>>>>>>
>>>>>>     static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>>>>>>                                         unsigned long *start,
>>>>>> @@ -492,6 +494,13 @@ static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>>>>>>                                         unsigned long *start,
>>>>>>                                         unsigned long *end) {}
>>>>>>
>>>>>> +static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
>>>>>> +                                      unsigned long addr, pmd_t *pmdp,
>>>>>> +                                      struct folio *folio)
>>>>>> +{
>>>>>> +     return false;
>>>>>> +}
>>>>>> +
>>>>>>     #define split_huge_pud(__vma, __pmd, __address)     \
>>>>>>         do { } while (0)
>>>>>>
>>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>>>> index 145505a1dd05..90fdef847a88 100644
>>>>>> --- a/mm/huge_memory.c
>>>>>> +++ b/mm/huge_memory.c
>>>>>> @@ -2690,6 +2690,79 @@ static void unmap_folio(struct folio *folio)
>>>>>>         try_to_unmap_flush();
>>>>>>     }
>>>>>>
>>>>>> +static bool __discard_trans_pmd_locked(struct vm_area_struct *vma,
>>>>>> +                                    unsigned long addr, pmd_t *pmdp,
>>>>>> +                                    struct folio *folio)
>>>>>> +{
>>>>>> +     struct mm_struct *mm = vma->vm_mm;
>>>>>> +     int ref_count, map_count;
>>>>>> +     pmd_t orig_pmd = *pmdp;
>>>>>> +     struct mmu_gather tlb;
>>>>>> +     struct page *page;
>>>>>> +
>>>>>> +     if (pmd_dirty(orig_pmd) || folio_test_dirty(folio))
>>>>>> +             return false;
>>>>>> +     if (unlikely(!pmd_present(orig_pmd) || !pmd_trans_huge(orig_pmd)))
>>>>>> +             return false;
>>>>>> +
>>>>>> +     page = pmd_page(orig_pmd);
>>>>>> +     if (unlikely(page_folio(page) != folio))
>>>>>> +             return false;
>>>>>> +
>>>>>> +     tlb_gather_mmu(&tlb, mm);
>>>>>> +     orig_pmd = pmdp_huge_get_and_clear(mm, addr, pmdp);
>>>>>> +     tlb_remove_pmd_tlb_entry(&tlb, pmdp, addr);
>>>>>> +
>>>>>> +     /*
>>>>>> +      * Syncing against concurrent GUP-fast:
>>>>>> +      * - clear PMD; barrier; read refcount
>>>>>> +      * - inc refcount; barrier; read PMD
>>>>>> +      */
>>>>>> +     smp_mb();
>>>>>> +
>>>>>> +     ref_count = folio_ref_count(folio);
>>>>>> +     map_count = folio_mapcount(folio);
>>>>>> +
>>>>>> +     /*
>>>>>> +      * Order reads for folio refcount and dirty flag
>>>>>> +      * (see comments in __remove_mapping()).
>>>>>> +      */
>>>>>> +     smp_rmb();
>>>>>> +
>>>>>> +     /*
>>>>>> +      * If the PMD or folio is redirtied at this point, or if there are
>>>>>> +      * unexpected references, we will give up to discard this folio
>>>>>> +      * and remap it.
>>>>>> +      *
>>>>>> +      * The only folio refs must be one from isolation plus the rmap(s).
>>>>>> +      */
>>>>>> +     if (ref_count != map_count + 1 || folio_test_dirty(folio) ||
>>>>>> +         pmd_dirty(orig_pmd)) {
>>>>>> +             set_pmd_at(mm, addr, pmdp, orig_pmd);
>>>>>> +             return false;
>>>>>> +     }
>>>>>> +
>>>>>> +     folio_remove_rmap_pmd(folio, page, vma);
>>>>>> +     zap_deposited_table(mm, pmdp);
>>>>>> +     add_mm_counter(mm, MM_ANONPAGES, -HPAGE_PMD_NR);
>>>>>> +     folio_put(folio);
>>>>>
>>>>> IIUC, you missed handling mlock vma, see mlock_drain_local() in
>>>>> try_to_unmap_one().
>>>>
>>>> Good spot!
>>>>
>>>> I suddenly realized that I overlooked another thing: If we detect that a
>>>> PMD-mapped THP is within the range of the VM_LOCKED VMA, we
>>>> should check whether the TTU_IGNORE_MLOCK flag is set in
>>>> try_to_unmap_one(). If the flag is set, we will remove the PMD mapping
>>>> from the folio. Otherwise, the folio should be mlocked, which avoids
>>>> splitting the folio and then mlocking each page again.
>>>
>>> My previous response above is flawed - sorry :(
>>>
>>> If we detect that a PMD-mapped THP is within the range of the
>>> VM_LOCKED VMA.
>>>
>>> 1) If the TTU_IGNORE_MLOCK flag is set, we will try to remove the
>>> PMD mapping from the folio, as this series has done.
>>
>> Right.
>>
>>> 2) If the flag is not set, the large folio should be mlocked to prevent it
>>> from being picked during memory reclaim? Currently, we just leave it
>>
>> Yes. From commit 1acbc3f93614 ("mm: handle large folio when large folio
>> in VM_LOCKED VMA range"), large folios of the mlocked VMA will be
>> handled during page reclaim phase.
>>
>>> as is and do not to mlock it, IIUC.
>>
>> Original code already handle the mlock case after the PMD-mapped THP is
>> split in try_to_unmap_one():
> 
> Yep. But this series doesn't do the TTU_SPLIT_HUGE_PMD immediately.
> 
>>                   /*
>>                    * If the folio is in an mlock()d vma, we must not swap
>> it out.
>>                    */
>>                   if (!(flags & TTU_IGNORE_MLOCK) &&
>>                       (vma->vm_flags & VM_LOCKED)) {
>>                           /* Restore the mlock which got missed */
> 
> IIUC, we could detect a PMD-mapped THP here. So, I'm not sure if we
> need to mlock it to prevent it from being picked again during memory
> reclaim. The change is as follows:

For the page reclaim path, folio_check_references() should be able to 
help restore the mlock of the PMD-mapped THP. However, for other paths 
that call try_to_unmap(), I believe it is still necessary to check 
whether the mlock of the PMD-mapped THP was missed.

Below code looks reasonable to me from a quick glance.

> diff --git a/mm/rmap.c b/mm/rmap.c
> index ed7f82036986..2a9d037ab23c 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1673,7 +1673,8 @@ static bool try_to_unmap_one(struct folio
> *folio, struct vm_area_struct *vma,
>                  if (!(flags & TTU_IGNORE_MLOCK) &&
>                      (vma->vm_flags & VM_LOCKED)) {
>                          /* Restore the mlock which got missed */
> -                       if (!folio_test_large(folio))
> +                       if (!folio_test_large(folio) ||
> +                           (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)))
>                                  mlock_vma_folio(folio, vma);
>                          goto walk_done_err;
>                  }
> 

Re: [PATCH v4 3/3] mm/vmscan: avoid split lazyfree THP during shrink_folio_list()

[Lance Yang]

On Thu, May 9, 2024 at 5:36 PM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 2024/5/7 19:37, Lance Yang wrote:
> > On Tue, May 7, 2024 at 5:33 PM Baolin Wang
> > <baolin.wang@linux.alibaba.com> wrote:
> >>
> >>
> >>
> >> On 2024/5/7 16:26, Lance Yang wrote:
> >>> On Tue, May 7, 2024 at 2:32 PM Lance Yang <ioworker0@gmail.com> wrote:
> >>>>
> >>>> Hey Baolin,
> >>>>
> >>>> Thanks a lot for taking time to review!
> >>>>
> >>>> On Tue, May 7, 2024 at 12:01 PM Baolin Wang
> >>>> <baolin.wang@linux.alibaba.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 2024/5/1 12:27, Lance Yang wrote:
> >>>>>> When the user no longer requires the pages, they would use
> >>>>>> madvise(MADV_FREE) to mark the pages as lazy free. Subsequently, they
> >>>>>> typically would not re-write to that memory again.
> >>>>>>
> >>>>>> During memory reclaim, if we detect that the large folio and its PMD are
> >>>>>> both still marked as clean and there are no unexpected references
> >>>>>> (such as GUP), so we can just discard the memory lazily, improving the
> >>>>>> efficiency of memory reclamation in this case.  On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
> >>>>>> mem_cgroup_force_empty() results in the following runtimes in seconds
> >>>>>> (shorter is better):
> >>>>>>
> >>>>>> --------------------------------------------
> >>>>>> |     Old       |      New       |  Change  |
> >>>>>> --------------------------------------------
> >>>>>> |   0.683426    |    0.049197    |  -92.80% |
> >>>>>> --------------------------------------------
> >>>>>>
> >>>>>> Suggested-by: Zi Yan <ziy@nvidia.com>
> >>>>>> Suggested-by: David Hildenbrand <david@redhat.com>
> >>>>>> Signed-off-by: Lance Yang <ioworker0@gmail.com>
> >>>>>> ---
> >>>>>>     include/linux/huge_mm.h |  9 +++++
> >>>>>>     mm/huge_memory.c        | 73 +++++++++++++++++++++++++++++++++++++++++
> >>>>>>     mm/rmap.c               |  3 ++
> >>>>>>     3 files changed, 85 insertions(+)
> >>>>>>
> >>>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> >>>>>> index 38c4b5537715..017cee864080 100644
> >>>>>> --- a/include/linux/huge_mm.h
> >>>>>> +++ b/include/linux/huge_mm.h
> >>>>>> @@ -411,6 +411,8 @@ static inline bool thp_migration_supported(void)
> >>>>>>
> >>>>>>     void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> >>>>>>                            pmd_t *pmd, bool freeze, struct folio *folio);
> >>>>>> +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> >>>>>> +                        pmd_t *pmdp, struct folio *folio);
> >>>>>>
> >>>>>>     static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> >>>>>>                                         unsigned long *start,
> >>>>>> @@ -492,6 +494,13 @@ static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> >>>>>>                                         unsigned long *start,
> >>>>>>                                         unsigned long *end) {}
> >>>>>>
> >>>>>> +static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
> >>>>>> +                                      unsigned long addr, pmd_t *pmdp,
> >>>>>> +                                      struct folio *folio)
> >>>>>> +{
> >>>>>> +     return false;
> >>>>>> +}
> >>>>>> +
> >>>>>>     #define split_huge_pud(__vma, __pmd, __address)     \
> >>>>>>         do { } while (0)
> >>>>>>
> >>>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >>>>>> index 145505a1dd05..90fdef847a88 100644
> >>>>>> --- a/mm/huge_memory.c
> >>>>>> +++ b/mm/huge_memory.c
> >>>>>> @@ -2690,6 +2690,79 @@ static void unmap_folio(struct folio *folio)
> >>>>>>         try_to_unmap_flush();
> >>>>>>     }
> >>>>>>
> >>>>>> +static bool __discard_trans_pmd_locked(struct vm_area_struct *vma,
> >>>>>> +                                    unsigned long addr, pmd_t *pmdp,
> >>>>>> +                                    struct folio *folio)
> >>>>>> +{
> >>>>>> +     struct mm_struct *mm = vma->vm_mm;
> >>>>>> +     int ref_count, map_count;
> >>>>>> +     pmd_t orig_pmd = *pmdp;
> >>>>>> +     struct mmu_gather tlb;
> >>>>>> +     struct page *page;
> >>>>>> +
> >>>>>> +     if (pmd_dirty(orig_pmd) || folio_test_dirty(folio))
> >>>>>> +             return false;
> >>>>>> +     if (unlikely(!pmd_present(orig_pmd) || !pmd_trans_huge(orig_pmd)))
> >>>>>> +             return false;
> >>>>>> +
> >>>>>> +     page = pmd_page(orig_pmd);
> >>>>>> +     if (unlikely(page_folio(page) != folio))
> >>>>>> +             return false;
> >>>>>> +
> >>>>>> +     tlb_gather_mmu(&tlb, mm);
> >>>>>> +     orig_pmd = pmdp_huge_get_and_clear(mm, addr, pmdp);
> >>>>>> +     tlb_remove_pmd_tlb_entry(&tlb, pmdp, addr);
> >>>>>> +
> >>>>>> +     /*
> >>>>>> +      * Syncing against concurrent GUP-fast:
> >>>>>> +      * - clear PMD; barrier; read refcount
> >>>>>> +      * - inc refcount; barrier; read PMD
> >>>>>> +      */
> >>>>>> +     smp_mb();
> >>>>>> +
> >>>>>> +     ref_count = folio_ref_count(folio);
> >>>>>> +     map_count = folio_mapcount(folio);
> >>>>>> +
> >>>>>> +     /*
> >>>>>> +      * Order reads for folio refcount and dirty flag
> >>>>>> +      * (see comments in __remove_mapping()).
> >>>>>> +      */
> >>>>>> +     smp_rmb();
> >>>>>> +
> >>>>>> +     /*
> >>>>>> +      * If the PMD or folio is redirtied at this point, or if there are
> >>>>>> +      * unexpected references, we will give up to discard this folio
> >>>>>> +      * and remap it.
> >>>>>> +      *
> >>>>>> +      * The only folio refs must be one from isolation plus the rmap(s).
> >>>>>> +      */
> >>>>>> +     if (ref_count != map_count + 1 || folio_test_dirty(folio) ||
> >>>>>> +         pmd_dirty(orig_pmd)) {
> >>>>>> +             set_pmd_at(mm, addr, pmdp, orig_pmd);
> >>>>>> +             return false;
> >>>>>> +     }
> >>>>>> +
> >>>>>> +     folio_remove_rmap_pmd(folio, page, vma);
> >>>>>> +     zap_deposited_table(mm, pmdp);
> >>>>>> +     add_mm_counter(mm, MM_ANONPAGES, -HPAGE_PMD_NR);
> >>>>>> +     folio_put(folio);
> >>>>>
> >>>>> IIUC, you missed handling mlock vma, see mlock_drain_local() in
> >>>>> try_to_unmap_one().
> >>>>
> >>>> Good spot!
> >>>>
> >>>> I suddenly realized that I overlooked another thing: If we detect that a
> >>>> PMD-mapped THP is within the range of the VM_LOCKED VMA, we
> >>>> should check whether the TTU_IGNORE_MLOCK flag is set in
> >>>> try_to_unmap_one(). If the flag is set, we will remove the PMD mapping
> >>>> from the folio. Otherwise, the folio should be mlocked, which avoids
> >>>> splitting the folio and then mlocking each page again.
> >>>
> >>> My previous response above is flawed - sorry :(
> >>>
> >>> If we detect that a PMD-mapped THP is within the range of the
> >>> VM_LOCKED VMA.
> >>>
> >>> 1) If the TTU_IGNORE_MLOCK flag is set, we will try to remove the
> >>> PMD mapping from the folio, as this series has done.
> >>
> >> Right.
> >>
> >>> 2) If the flag is not set, the large folio should be mlocked to prevent it
> >>> from being picked during memory reclaim? Currently, we just leave it
> >>
> >> Yes. From commit 1acbc3f93614 ("mm: handle large folio when large folio
> >> in VM_LOCKED VMA range"), large folios of the mlocked VMA will be
> >> handled during page reclaim phase.
> >>
> >>> as is and do not to mlock it, IIUC.
> >>
> >> Original code already handle the mlock case after the PMD-mapped THP is
> >> split in try_to_unmap_one():
> >
> > Yep. But this series doesn't do the TTU_SPLIT_HUGE_PMD immediately.
> >
> >>                   /*
> >>                    * If the folio is in an mlock()d vma, we must not swap
> >> it out.
> >>                    */
> >>                   if (!(flags & TTU_IGNORE_MLOCK) &&
> >>                       (vma->vm_flags & VM_LOCKED)) {
> >>                           /* Restore the mlock which got missed */
> >
> > IIUC, we could detect a PMD-mapped THP here. So, I'm not sure if we
> > need to mlock it to prevent it from being picked again during memory
> > reclaim. The change is as follows:
>
> For the page reclaim path, folio_check_references() should be able to
> help restore the mlock of the PMD-mapped THP. However, for other paths

I understood, thanks for clarifying!

> that call try_to_unmap(), I believe it is still necessary to check
> whether the mlock of the PMD-mapped THP was missed.

Yeah, agreed!

The TTU_SPLIT_HUGE_PMD will no longer perform immediately, so we
might encounter a PMD-mapped THP missing the mlock in the VM_LOCKED
range during the pagewalk. It's likely necessary to mlock this THP to prevent
it from being picked up during page reclaim.

Given this, I'll include the change below in the next version.

>
> Below code looks reasonable to me from a quick glance.

Thanks again for the review!
Lance

>
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index ed7f82036986..2a9d037ab23c 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1673,7 +1673,8 @@ static bool try_to_unmap_one(struct folio
> > *folio, struct vm_area_struct *vma,
> >                  if (!(flags & TTU_IGNORE_MLOCK) &&
> >                      (vma->vm_flags & VM_LOCKED)) {
> >                          /* Restore the mlock which got missed */
> > -                       if (!folio_test_large(folio))
> > +                       if (!folio_test_large(folio) ||
> > +                           (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)))
> >                                  mlock_vma_folio(folio, vma);
> >                          goto walk_done_err;
> >                  }
> >
>

Re: [PATCH v4 3/3] mm/vmscan: avoid split lazyfree THP during shrink_folio_list()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 5753 bytes --]

On 1 May 2024, at 0:27, Lance Yang wrote:

> When the user no longer requires the pages, they would use
> madvise(MADV_FREE) to mark the pages as lazy free. Subsequently, they
> typically would not re-write to that memory again.
>
> During memory reclaim, if we detect that the large folio and its PMD are
> both still marked as clean and there are no unexpected references
> (such as GUP), so we can just discard the memory lazily, improving the
> efficiency of memory reclamation in this case.  On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
> mem_cgroup_force_empty() results in the following runtimes in seconds
> (shorter is better):
>
> --------------------------------------------
> |     Old       |      New       |  Change  |
> --------------------------------------------
> |   0.683426    |    0.049197    |  -92.80% |
> --------------------------------------------
>
> Suggested-by: Zi Yan <ziy@nvidia.com>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Lance Yang <ioworker0@gmail.com>
> ---
>  include/linux/huge_mm.h |  9 +++++
>  mm/huge_memory.c        | 73 +++++++++++++++++++++++++++++++++++++++++
>  mm/rmap.c               |  3 ++
>  3 files changed, 85 insertions(+)
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index 38c4b5537715..017cee864080 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -411,6 +411,8 @@ static inline bool thp_migration_supported(void)
>
>  void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
>  			   pmd_t *pmd, bool freeze, struct folio *folio);
> +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> +			   pmd_t *pmdp, struct folio *folio);
>
>  static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>  					unsigned long *start,
> @@ -492,6 +494,13 @@ static inline void align_huge_pmd_range(struct vm_area_struct *vma,
>  					unsigned long *start,
>  					unsigned long *end) {}
>
> +static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
> +					 unsigned long addr, pmd_t *pmdp,
> +					 struct folio *folio)
> +{
> +	return false;
> +}
> +
>  #define split_huge_pud(__vma, __pmd, __address)	\
>  	do { } while (0)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 145505a1dd05..90fdef847a88 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2690,6 +2690,79 @@ static void unmap_folio(struct folio *folio)
>  	try_to_unmap_flush();
>  }
>
> +static bool __discard_trans_pmd_locked(struct vm_area_struct *vma,
> +				       unsigned long addr, pmd_t *pmdp,
> +				       struct folio *folio)
> +{
> +	struct mm_struct *mm = vma->vm_mm;
> +	int ref_count, map_count;
> +	pmd_t orig_pmd = *pmdp;
> +	struct mmu_gather tlb;
> +	struct page *page;
> +
> +	if (pmd_dirty(orig_pmd) || folio_test_dirty(folio))
> +		return false;
> +	if (unlikely(!pmd_present(orig_pmd) || !pmd_trans_huge(orig_pmd)))
> +		return false;
> +
> +	page = pmd_page(orig_pmd);
> +	if (unlikely(page_folio(page) != folio))
> +		return false;
> +
> +	tlb_gather_mmu(&tlb, mm);
> +	orig_pmd = pmdp_huge_get_and_clear(mm, addr, pmdp);
> +	tlb_remove_pmd_tlb_entry(&tlb, pmdp, addr);
> +
> +	/*
> +	 * Syncing against concurrent GUP-fast:
> +	 * - clear PMD; barrier; read refcount
> +	 * - inc refcount; barrier; read PMD
> +	 */
> +	smp_mb();
> +
> +	ref_count = folio_ref_count(folio);
> +	map_count = folio_mapcount(folio);
> +
> +	/*
> +	 * Order reads for folio refcount and dirty flag
> +	 * (see comments in __remove_mapping()).
> +	 */
> +	smp_rmb();
> +
> +	/*
> +	 * If the PMD or folio is redirtied at this point, or if there are
> +	 * unexpected references, we will give up to discard this folio
> +	 * and remap it.
> +	 *
> +	 * The only folio refs must be one from isolation plus the rmap(s).
> +	 */
> +	if (ref_count != map_count + 1 || folio_test_dirty(folio) ||
> +	    pmd_dirty(orig_pmd)) {
> +		set_pmd_at(mm, addr, pmdp, orig_pmd);
> +		return false;
> +	}
> +
> +	folio_remove_rmap_pmd(folio, page, vma);
> +	zap_deposited_table(mm, pmdp);
> +	add_mm_counter(mm, MM_ANONPAGES, -HPAGE_PMD_NR);
> +	folio_put(folio);
> +
> +	return true;
> +}
> +
> +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> +			   pmd_t *pmdp, struct folio *folio)
> +{
> +	VM_WARN_ON_FOLIO(!folio_test_pmd_mappable(folio), folio);
> +	VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
> +	VM_WARN_ON_ONCE(!IS_ALIGNED(addr, HPAGE_PMD_SIZE));
> +
> +	if (folio_test_anon(folio) && !folio_test_swapbacked(folio))
> +		return __discard_trans_pmd_locked(vma, addr, pmdp, folio);
> +
> +	return false;
> +}
> +
>  static void remap_page(struct folio *folio, unsigned long nr)
>  {
>  	int i = 0;
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 432601154583..1d3d30cb752c 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1675,6 +1675,9 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>  		}
>
>  		if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
> +			if (unmap_huge_pmd_locked(vma, range.start, pvmw.pmd,
> +						  folio))
> +				goto walk_done;

You might not need to check (flags & TTU_SPLIT_HUGE_PMD) for
unmap_huge_pmd_locked(), since you are unmapping a PMD here.
TTU_SPLIT_HUGE_PMD is here because try_to_unmap_one() was not able to unmap
a PMD. You probably can remove it for callers that are unmapping
the folio but not the ones are swapping.



>  			/*
>  			 * We temporarily have to drop the PTL and start once
>  			 * again from that now-PTE-mapped page table.
> -- 
> 2.33.1


--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [PATCH v4 3/3] mm/vmscan: avoid split lazyfree THP during shrink_folio_list()

[Lance Yang]

Hey Zi,

Thanks for taking time to review!

On Wed, May 8, 2024 at 12:20 AM Zi Yan <ziy@nvidia.com> wrote:
>
> On 1 May 2024, at 0:27, Lance Yang wrote:
>
> > When the user no longer requires the pages, they would use
> > madvise(MADV_FREE) to mark the pages as lazy free. Subsequently, they
> > typically would not re-write to that memory again.
> >
> > During memory reclaim, if we detect that the large folio and its PMD are
> > both still marked as clean and there are no unexpected references
> > (such as GUP), so we can just discard the memory lazily, improving the
> > efficiency of memory reclamation in this case.  On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
> > mem_cgroup_force_empty() results in the following runtimes in seconds
> > (shorter is better):
> >
> > --------------------------------------------
> > |     Old       |      New       |  Change  |
> > --------------------------------------------
> > |   0.683426    |    0.049197    |  -92.80% |
> > --------------------------------------------
> >
> > Suggested-by: Zi Yan <ziy@nvidia.com>
> > Suggested-by: David Hildenbrand <david@redhat.com>
> > Signed-off-by: Lance Yang <ioworker0@gmail.com>
> > ---
> >  include/linux/huge_mm.h |  9 +++++
> >  mm/huge_memory.c        | 73 +++++++++++++++++++++++++++++++++++++++++
> >  mm/rmap.c               |  3 ++
> >  3 files changed, 85 insertions(+)
> >
> > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > index 38c4b5537715..017cee864080 100644
> > --- a/include/linux/huge_mm.h
> > +++ b/include/linux/huge_mm.h
> > @@ -411,6 +411,8 @@ static inline bool thp_migration_supported(void)
> >
> >  void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> >                          pmd_t *pmd, bool freeze, struct folio *folio);
> > +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> > +                        pmd_t *pmdp, struct folio *folio);
> >
> >  static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> >                                       unsigned long *start,
> > @@ -492,6 +494,13 @@ static inline void align_huge_pmd_range(struct vm_area_struct *vma,
> >                                       unsigned long *start,
> >                                       unsigned long *end) {}
> >
> > +static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
> > +                                      unsigned long addr, pmd_t *pmdp,
> > +                                      struct folio *folio)
> > +{
> > +     return false;
> > +}
> > +
> >  #define split_huge_pud(__vma, __pmd, __address)      \
> >       do { } while (0)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 145505a1dd05..90fdef847a88 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2690,6 +2690,79 @@ static void unmap_folio(struct folio *folio)
> >       try_to_unmap_flush();
> >  }
> >
> > +static bool __discard_trans_pmd_locked(struct vm_area_struct *vma,
> > +                                    unsigned long addr, pmd_t *pmdp,
> > +                                    struct folio *folio)
> > +{
> > +     struct mm_struct *mm = vma->vm_mm;
> > +     int ref_count, map_count;
> > +     pmd_t orig_pmd = *pmdp;
> > +     struct mmu_gather tlb;
> > +     struct page *page;
> > +
> > +     if (pmd_dirty(orig_pmd) || folio_test_dirty(folio))
> > +             return false;
> > +     if (unlikely(!pmd_present(orig_pmd) || !pmd_trans_huge(orig_pmd)))
> > +             return false;
> > +
> > +     page = pmd_page(orig_pmd);
> > +     if (unlikely(page_folio(page) != folio))
> > +             return false;
> > +
> > +     tlb_gather_mmu(&tlb, mm);
> > +     orig_pmd = pmdp_huge_get_and_clear(mm, addr, pmdp);
> > +     tlb_remove_pmd_tlb_entry(&tlb, pmdp, addr);
> > +
> > +     /*
> > +      * Syncing against concurrent GUP-fast:
> > +      * - clear PMD; barrier; read refcount
> > +      * - inc refcount; barrier; read PMD
> > +      */
> > +     smp_mb();
> > +
> > +     ref_count = folio_ref_count(folio);
> > +     map_count = folio_mapcount(folio);
> > +
> > +     /*
> > +      * Order reads for folio refcount and dirty flag
> > +      * (see comments in __remove_mapping()).
> > +      */
> > +     smp_rmb();
> > +
> > +     /*
> > +      * If the PMD or folio is redirtied at this point, or if there are
> > +      * unexpected references, we will give up to discard this folio
> > +      * and remap it.
> > +      *
> > +      * The only folio refs must be one from isolation plus the rmap(s).
> > +      */
> > +     if (ref_count != map_count + 1 || folio_test_dirty(folio) ||
> > +         pmd_dirty(orig_pmd)) {
> > +             set_pmd_at(mm, addr, pmdp, orig_pmd);
> > +             return false;
> > +     }
> > +
> > +     folio_remove_rmap_pmd(folio, page, vma);
> > +     zap_deposited_table(mm, pmdp);
> > +     add_mm_counter(mm, MM_ANONPAGES, -HPAGE_PMD_NR);
> > +     folio_put(folio);
> > +
> > +     return true;
> > +}
> > +
> > +bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
> > +                        pmd_t *pmdp, struct folio *folio)
> > +{
> > +     VM_WARN_ON_FOLIO(!folio_test_pmd_mappable(folio), folio);
> > +     VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
> > +     VM_WARN_ON_ONCE(!IS_ALIGNED(addr, HPAGE_PMD_SIZE));
> > +
> > +     if (folio_test_anon(folio) && !folio_test_swapbacked(folio))
> > +             return __discard_trans_pmd_locked(vma, addr, pmdp, folio);
> > +
> > +     return false;
> > +}
> > +
> >  static void remap_page(struct folio *folio, unsigned long nr)
> >  {
> >       int i = 0;
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index 432601154583..1d3d30cb752c 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -1675,6 +1675,9 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
> >               }
> >
> >               if (!pvmw.pte && (flags & TTU_SPLIT_HUGE_PMD)) {
> > +                     if (unmap_huge_pmd_locked(vma, range.start, pvmw.pmd,
> > +                                               folio))
> > +                             goto walk_done;
>
> You might not need to check (flags & TTU_SPLIT_HUGE_PMD) for
> unmap_huge_pmd_locked(), since you are unmapping a PMD here.
> TTU_SPLIT_HUGE_PMD is here because try_to_unmap_one() was not able to unmap
> a PMD. You probably can remove it for callers that are unmapping
> the folio but not the ones are swapping.

Thanks for the suggestion!

Ageed. For unmap_huge_pmd_locked(), there is no need to check the
TTU_SPLIT_HUGE_PMD flag. We only need to check the flag for
split_huge_pmd_locked().

Given this, if we fail to remove the PMD mapping and the flag is not set,
I think we should stop the walk. So we can also remove the
VM_BUG_ON_FOLIO() below.

/* Unexpected PMD-mapped THP? */
VM_BUG_ON_FOLIO(!pvmw.pte, folio);

Zi, what do you think?

Thanks,
Lance

>
>
>
> >                       /*
> >                        * We temporarily have to drop the PTL and start once
> >                        * again from that now-PTE-mapped page table.
> > --
> > 2.33.1
>
>
> --
> Best Regards,
> Yan, Zi

Re: [PATCH v4 0/3] Reclaim lazyfree THP without splitting

[SeongJae Park]

Hi Lance,

On Wed,  1 May 2024 12:26:57 +0800 Lance Yang <ioworker0@gmail.com> wrote:

> Hi all,
> 
> This series adds support for reclaiming PMD-mapped THP marked as lazyfree
> without needing to first split the large folio via split_huge_pmd_address().
> 
> When the user no longer requires the pages, they would use madvise(MADV_FREE)
> to mark the pages as lazy free. Subsequently, they typically would not re-write
> to that memory again.
> 
> During memory reclaim, if we detect that the large folio and its PMD are both
> still marked as clean and there are no unexpected references(such as GUP), so we
> can just discard the memory lazily, improving the efficiency of memory
> reclamation in this case.
> 
> Performance Testing
> ===================
> 
> On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
> mem_cgroup_force_empty() results in the following runtimes in seconds
> (shorter is better):
> 
> --------------------------------------------
> |     Old       |      New       |  Change  |
> --------------------------------------------
> |   0.683426    |    0.049197    |  -92.80% |
> --------------------------------------------
> 
> ---
> 
> Changes since v3 [3]
> ====================
>  - mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop
>     - Resolve compilation errors by handling the case where
>       CONFIG_PGTABLE_HAS_HUGE_LEAVES is undefined (thanks to SeongJae Park)

I confirmed that the issue I reported before is disappeared with this version
of the patchset.  For the fix,

Tested-by: SeongJae Park <sj@kernel.org>


Thanks,
SJ

[...]

Re: [PATCH v4 0/3] Reclaim lazyfree THP without splitting

[Lance Yang]

On Thu, May 2, 2024 at 12:08 AM SeongJae Park <sj@kernel.org> wrote:
>
> Hi Lance,
>
> On Wed,  1 May 2024 12:26:57 +0800 Lance Yang <ioworker0@gmail.com> wrote:
>
> > Hi all,
> >
> > This series adds support for reclaiming PMD-mapped THP marked as lazyfree
> > without needing to first split the large folio via split_huge_pmd_address().
> >
> > When the user no longer requires the pages, they would use madvise(MADV_FREE)
> > to mark the pages as lazy free. Subsequently, they typically would not re-write
> > to that memory again.
> >
> > During memory reclaim, if we detect that the large folio and its PMD are both
> > still marked as clean and there are no unexpected references(such as GUP), so we
> > can just discard the memory lazily, improving the efficiency of memory
> > reclamation in this case.
> >
> > Performance Testing
> > ===================
> >
> > On an Intel i5 CPU, reclaiming 1GiB of lazyfree THPs using
> > mem_cgroup_force_empty() results in the following runtimes in seconds
> > (shorter is better):
> >
> > --------------------------------------------
> > |     Old       |      New       |  Change  |
> > --------------------------------------------
> > |   0.683426    |    0.049197    |  -92.80% |
> > --------------------------------------------
> >
> > ---
> >
> > Changes since v3 [3]
> > ====================
> >  - mm/rmap: integrate PMD-mapped folio splitting into pagewalk loop
> >     - Resolve compilation errors by handling the case where
> >       CONFIG_PGTABLE_HAS_HUGE_LEAVES is undefined (thanks to SeongJae Park)
>
> I confirmed that the issue I reported before is disappeared with this version
> of the patchset.  For the fix,
>
> Tested-by: SeongJae Park <sj@kernel.org>

Hey SJ,

Thanks for taking time to confirm! I appreciate your feedback!

Thanks,
Lance

>
>
> Thanks,
> SJ
>
> [...]