Re: [PATCH v2 4/4] mm: Avoid splitting pmd for lazyfree pmd-mapped THP in try_to_unmap

[Barry Song <21cnbao@gmail.com>]

> > >               if (!pvmw.pte) {
> > > +                     lazyfree = folio_test_anon(folio) && !folio_test_swapbacked(folio);
> >
> > You've checked lazyfree here, so can we remove the duplicate check in
> > unmap_huge_pmd_locked()? Then the code should be:
> >
> >                 if (lazyfree && unmap_huge_pmd_locked(...))
> >                         goto walk_done;
>
>
> right. it seems unmap_huge_pmd_locked() only handles lazyfree pmd-mapped
> thp. so i guess the code could be:
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index aea49f7125f1..c4c3a7896de4 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3131,11 +3131,10 @@ bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
>         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));
> +       VM_WARN_ON_FOLIO(!folio_test_anon(folio), folio);
> +       VM_WARN_ON_FOLIO(folio_test_swapbacked(folio), folio);
>
> -       if (folio_test_anon(folio) && !folio_test_swapbacked(folio))
> -               return __discard_anon_folio_pmd_locked(vma, addr, pmdp, folio);
> -
> -       return false;
> +       return __discard_anon_folio_pmd_locked(vma, addr, pmdp, folio);
>  }
>
>  static void remap_page(struct folio *folio, unsigned long nr, int flags)
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 02c4e4b2cd7b..72907eb1b8fe 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1671,7 +1671,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>         DEFINE_FOLIO_VMA_WALK(pvmw, folio, vma, address, 0);
>         pte_t pteval;
>         struct page *subpage;
> -       bool anon_exclusive, lazyfree, ret = true;
> +       bool anon_exclusive, ret = true;
>         struct mmu_notifier_range range;
>         enum ttu_flags flags = (enum ttu_flags)(long)arg;
>         int nr_pages = 1;
> @@ -1724,18 +1724,16 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>                 }
>
>                 if (!pvmw.pte) {
> -                       lazyfree = folio_test_anon(folio) && !folio_test_swapbacked(folio);
> -
> -                       if (unmap_huge_pmd_locked(vma, pvmw.address, pvmw.pmd,
> -                                                 folio))
> -                               goto walk_done;
> -                       /*
> -                        * unmap_huge_pmd_locked has either already marked
> -                        * the folio as swap-backed or decided to retain it
> -                        * due to GUP or speculative references.
> -                        */
> -                       if (lazyfree)
> +                       if (folio_test_anon(folio) && !folio_test_swapbacked(folio)) {
> +                               if (unmap_huge_pmd_locked(vma, pvmw.address, pvmw.pmd, folio))
> +                                       goto walk_done;
> +                               /*
> +                                * unmap_huge_pmd_locked has either already marked
> +                                * the folio as swap-backed or decided to retain it
> +                                * due to GUP or speculative references.
> +                                */
>                                 goto walk_abort;
> +                       }
>
>                         if (flags & TTU_SPLIT_HUGE_PMD) {
>                                 /*
>
> >
> > >                       if (unmap_huge_pmd_locked(vma, pvmw.address, pvmw.pmd,
> > >                                                 folio))
> > >                               goto walk_done;
> > > +                     /*
> > > +                      * unmap_huge_pmd_locked has either already marked
> > > +                      * the folio as swap-backed or decided to retain it
> > > +                      * due to GUP or speculative references.
> > > +                      */
> > > +                     if (lazyfree)
> > > +                             goto walk_abort;
> > > 
> > >                       if (flags & TTU_SPLIT_HUGE_PMD) {
> > >                               /*



The final diff is as follows.
Baolin, do you have any additional comments before I send out v3? 

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 3d3ebdc002d5..47cc8c3f8f80 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3070,8 +3070,12 @@ static bool __discard_anon_folio_pmd_locked(struct vm_area_struct *vma,
 	int ref_count, map_count;
 	pmd_t orig_pmd = *pmdp;
 
-	if (folio_test_dirty(folio) || pmd_dirty(orig_pmd))
+	if (pmd_dirty(orig_pmd))
+		folio_set_dirty(folio);
+	if (folio_test_dirty(folio) && !(vma->vm_flags & VM_DROPPABLE)) {
+		folio_set_swapbacked(folio);
 		return false;
+	}
 
 	orig_pmd = pmdp_huge_clear_flush(vma, addr, pmdp);
 
@@ -3098,8 +3102,15 @@ static bool __discard_anon_folio_pmd_locked(struct vm_area_struct *vma,
 	 *
 	 * The only folio refs must be one from isolation plus the rmap(s).
 	 */
-	if (folio_test_dirty(folio) || pmd_dirty(orig_pmd) ||
-	    ref_count != map_count + 1) {
+	if (pmd_dirty(orig_pmd))
+		folio_set_dirty(folio);
+	if (folio_test_dirty(folio) && !(vma->vm_flags & VM_DROPPABLE)) {
+		folio_set_swapbacked(folio);
+		set_pmd_at(mm, addr, pmdp, orig_pmd);
+		return false;
+	}
+
+	if (ref_count != map_count + 1) {
 		set_pmd_at(mm, addr, pmdp, orig_pmd);
 		return false;
 	}
@@ -3119,12 +3130,11 @@ bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
 {
 	VM_WARN_ON_FOLIO(!folio_test_pmd_mappable(folio), folio);
 	VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
+	VM_WARN_ON_FOLIO(!folio_test_anon(folio), folio);
+	VM_WARN_ON_FOLIO(folio_test_swapbacked(folio), folio);
 	VM_WARN_ON_ONCE(!IS_ALIGNED(addr, HPAGE_PMD_SIZE));
 
-	if (folio_test_anon(folio) && !folio_test_swapbacked(folio))
-		return __discard_anon_folio_pmd_locked(vma, addr, pmdp, folio);
-
-	return false;
+	return __discard_anon_folio_pmd_locked(vma, addr, pmdp, folio);
 }
 
 static void remap_page(struct folio *folio, unsigned long nr, int flags)
diff --git a/mm/rmap.c b/mm/rmap.c
index 3ef659310797..72907eb1b8fe 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1724,9 +1724,16 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
 		}
 
 		if (!pvmw.pte) {
-			if (unmap_huge_pmd_locked(vma, pvmw.address, pvmw.pmd,
-						  folio))
-				goto walk_done;
+			if (folio_test_anon(folio) && !folio_test_swapbacked(folio)) {
+				if (unmap_huge_pmd_locked(vma, pvmw.address, pvmw.pmd, folio))
+					goto walk_done;
+				/*
+				 * unmap_huge_pmd_locked has either already marked
+				 * the folio as swap-backed or decided to retain it
+				 * due to GUP or speculative references.
+				 */
+				goto walk_abort;
+			}
 
 			if (flags & TTU_SPLIT_HUGE_PMD) {
 				/*
-- 
2.39.3 (Apple Git-146)