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 9:35 pm, Dev Jain wrote:
> 
> 
> On 11/04/26 2:25 pm, Barry Song wrote:
>> On Fri, Apr 10, 2026 at 6:32 PM Dev Jain <dev.jain@arm.com> 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)
>>> +{
>>
>> Can we add a comment before the function explaining what
>> the return value means?
> 
> Yes I can add that.
> 
> 
>>
>>> +       /*
>>> +        * 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.
> 
> I'll rename this to exit_walk, so it doesn't collide with
> the label names.

But then

if (exit_walk)
	goto walk_done;

also looks weird.

I think I should do

if (exit_walk) {
	page_vma_mapped_walk_done();
	break;
}

The mess here is that we have a label walk_abort which
is literally ret = false + walk_done.

Perhaps we can remove one of the labels, have a single label
exit_walk and inline the "set ret = false/true and goto exit_label"
for the discarded label. I hesitated in doing this because both
labels are being currently used at a good amount of places.

> 
>>
>> 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.
> 
> I thought a lot about how to refactor try_to_unmap_one() as
> a whole, and couldn't come up with a good solution.
> 
> There are these conditions:
> 
> 1. ret = false => page_vma_mapped_walk_done(), break
> 2. ret not decided, "continue"
> 3. ret = true
>    a) exit the while loop naturally
>    b) exit prematurely -> page_vma_mapped_walk_done(), break
> 
> I had thought about the refactoring method to have an enum for
> all conditions. So we can refactor bits of code, return an
> enum, but we will still retain ugliness like
> 
> if (ret == WALK_DONE)
> 	goto walk_done;
> if (ret == WALK_ABORT)
> 	goto walk_abort;
> if (ret == WALK_CONTINUE)
> 	continue;
> 
> This seemed more of a forced-refactoring to me, IMHO doesn't
> reduce the complexity of the function at all.
> 
> I don't have a clever solution to get rid of all the label
> jumping, so I refactored what I could.
> 
>>
>> Thanks
>> Barry
> 
>