Re: [PATCH v2 2/9] mm/rmap: refactor hugetlb pte clearing in try_to_unmap_one

[Barry Song]

On Fri, Apr 10, 2026 at 6:32 PM Dev Jain <dev.jain@xxxxxxx> wrote:
>
> Simplify the code by refactoring the folio_test_hugetlb() branch into
> a new function.
>
> No functional change is intended.
>
> Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
> ---
>  mm/rmap.c | 116 +++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 67 insertions(+), 49 deletions(-)
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 62a8c912fd788..a9c43e2f6e695 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1978,6 +1978,67 @@ static inline unsigned int folio_unmap_pte_batch(struct folio *folio,
>                                      FPB_RESPECT_WRITE | FPB_RESPECT_SOFT_DIRTY);
>  }
>
> +static inline bool unmap_hugetlb_folio(struct vm_area_struct *vma,
> +               struct folio *folio, struct page_vma_mapped_walk *pvmw,
> +               struct page *page, enum ttu_flags flags, pte_t *pteval,
> +               struct mmu_notifier_range *range, bool *walk_done)
> +{

Can we add a comment before the function explaining what
the return value means?

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

Sometimes I feel walk_done is misleading, since walk_done with
ret = false actually means walk_abort.

So another option is to make this function return a tristate:
WALK_DONE, WALK_ABORT, WALK_CONT. Then we could drop the
walk_done argument entirely.

Thanks
Barry