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

[Dev Jain <dev.jain@arm.com>]

On 11/04/26 5:15 pm, Jie Gan wrote:
> 
> 
> On 4/10/2026 6:31 PM, Dev Jain 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@arm.com>
>> ---
>>   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)
>> +{
>> +    /*
>> +     * 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);
> 
> As you mentioned "No functional change is intended." in commit message, but
> you have changed VM_BUG_ON_PAGE to VM_WARN_ON_PAGE here?

Forgot to mention this in the description : )

... "While at it, as BUG_ONs are discouraged, convert them
to WARN_ON."


> 
>> +    /*
>> +     * 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));
> 
> ditto
> 
> Thanks,
> Jie
> 
>> +        if (!hugetlb_vma_trylock_write(vma)) {
>> +            *walk_done = true;
>> +            return false;
>> +        }
>> +
>> +        tlb_gather_mmu_vma(&tlb, vma);
>> +        if (huge_pmd_unshare(&tlb, vma, pvmw->address, pvmw->pte)) {
>> +            hugetlb_vma_unlock_write(vma);
>> +            huge_pmd_unshare_flush(&tlb, vma);
>> +            tlb_finish_mmu(&tlb);
>> +            /*
>> +             * The PMD table was unmapped,
>> +             * consequently unmapping the folio.
>> +             */
>> +            *walk_done = true;
>> +            return true;
>> +        }
>> +        hugetlb_vma_unlock_write(vma);
>> +        tlb_finish_mmu(&tlb);
>> +    }
>> +    *pteval = huge_ptep_clear_flush(vma, pvmw->address, pvmw->pte);
>> +    if (pte_dirty(*pteval))
>> +        folio_mark_dirty(folio);
>> +
>> +    *walk_done = false;
>> +    return true;
>> +}
>> +
>>   /*
>>    * @arg: enum ttu_flags will be passed to this argument
>>    */
>> @@ -2115,56 +2176,13 @@ static bool try_to_unmap_one(struct folio *folio,
>> struct vm_area_struct *vma,
>>                    PageAnonExclusive(subpage);
>>             if (folio_test_hugetlb(folio)) {
>> -            bool anon = folio_test_anon(folio);
>> -
>> -            /*
>> -             * The try_to_unmap() is only passed a hugetlb page
>> -             * in the case where the hugetlb page is poisoned.
>> -             */
>> -            VM_BUG_ON_PAGE(!PageHWPoison(subpage), subpage);
>> -            /*
>> -             * 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);
>> +            bool walk_done;
>>   -            /*
>> -             * 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 (!anon) {
>> -                struct mmu_gather tlb;
>> -
>> -                VM_BUG_ON(!(flags & TTU_RMAP_LOCKED));
>> -                if (!hugetlb_vma_trylock_write(vma))
>> -                    goto walk_abort;
>> -
>> -                tlb_gather_mmu_vma(&tlb, vma);
>> -                if (huge_pmd_unshare(&tlb, vma, address, pvmw.pte)) {
>> -                    hugetlb_vma_unlock_write(vma);
>> -                    huge_pmd_unshare_flush(&tlb, vma);
>> -                    tlb_finish_mmu(&tlb);
>> -                    /*
>> -                     * The PMD table was unmapped,
>> -                     * consequently unmapping the folio.
>> -                     */
>> -                    goto walk_done;
>> -                }
>> -                hugetlb_vma_unlock_write(vma);
>> -                tlb_finish_mmu(&tlb);
>> -            }
>> -            pteval = huge_ptep_clear_flush(vma, address, pvmw.pte);
>> -            if (pte_dirty(pteval))
>> -                folio_mark_dirty(folio);
>> +            ret = unmap_hugetlb_folio(vma, folio, &pvmw, subpage,
>> +                          flags, &pteval, &range,
>> +                          &walk_done);
>> +            if (walk_done)
>> +                goto walk_done;
>>           } else if (likely(pte_present(pteval))) {
>>               nr_pages = folio_unmap_pte_batch(folio, &pvmw, flags, pteval);
>>               end_addr = address + nr_pages * PAGE_SIZE;
>