Re: [PATCH 2/5] mm/rmap: Add try_to_unmap_hugetlb_one

[Dev Jain]

On 07/07/26 7:45 pm, David Hildenbrand (Arm) wrote:
> On 7/7/26 14:11, Dev Jain wrote:
>> Simplify try_to_unmap_one() by separating the hugetlb parts into
>> try_to_unmap_hugetlb_one().
>>
>> To understand the correctness of the refactoring, the following points
>> are noted:
>>
>> 1. try_to_unmap() is called for hugetlb folios only when they are
>>    hwpoisoned.
>>
>> 2. A hugetlb VMA cannot be mlocked.
>>
>> 3. page_vma_mapped_walk() returns at most one hugetlb mapping in a VMA,
>>    and that mapping points at the head PFN.
>>
>> 4. We won't ever process a softleaf entry that encodes a hugetlb folio;
>>    hugetlb folios are never swapped out, migration entries will be
>>    skipped (PVMW_MIGRATION not passed), and device-exclusive does not
>>    work for hugetlb.
>>
>> 5. The hwpoison entry is constructed from the poisoned folio, just as in
>>    the pre-refactor code. Any previous uffd-wp state is deliberately not
>>    preserved for the hwpoison entry.
>>
>> 6. TTU_HWPOISON is always present; for it to not be present, either the
>>    folio has to be in swapcache, or mapping_can_writeback() is true (see
>>    unmap_poisoned_folio), none of which is true for hugetlb folios.
>>
>> 7. Hugetlb uses separate counters from normal rss counters, therefore
>>    update_highwater_rss() need not be called.
>>
>> While at it:
>>
>>  - Change VM_BUG_* to VM_WARN_*.
>>
>>  - Do not declare variables which are only used once.
>>
>>  - Use huge_pte_dirty() instead of pte_dirty().
>>
>>  - Add 3 VM_WARN_ON_ONCE asserting that TTU_HWPOISON should be present,
>>    pte_present() must be true, and the pfn derived from the huge pte
>>    must be that of the head of the hugetlb folio.
>>
>> Except the BUG->WARN change, no functional change intended.
>>
>> Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
>> Signed-off-by: Dev Jain <dev.jain@xxxxxxx>
>> ---
>>  include/linux/hugetlb.h |   1 +
>>  mm/rmap.c               | 177 +++++++++++++++++++++-------------------
>>  2 files changed, 96 insertions(+), 82 deletions(-)
>>
>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>> index 4115076e4922a..bf7e163e3779d 100644
>> --- a/include/linux/hugetlb.h
>> +++ b/include/linux/hugetlb.h
>> @@ -1271,6 +1271,7 @@ static inline void hugetlb_count_sub(long l, struct mm_struct *mm)
>>  }
>>  
>>  pte_t huge_ptep_get(struct mm_struct *mm, unsigned long addr, pte_t *ptep);
>> +unsigned long huge_pte_dirty(pte_t pte);
>>  
>>  static inline pte_t huge_ptep_clear_flush(struct vm_area_struct *vma,
>>  					  unsigned long addr, pte_t *ptep)
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index dbb077f8443e3..934773dfa2f2a 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -1978,6 +1978,95 @@ static inline unsigned int folio_unmap_pte_batch(struct folio *folio,
>>  				     FPB_RESPECT_WRITE | FPB_RESPECT_SOFT_DIRTY);
>>  }
>>  
>> +static bool try_to_unmap_hugetlb_one(struct folio *folio,
>> +		struct vm_area_struct *vma, unsigned long address, void *arg)
>> +{
>> +	DEFINE_FOLIO_VMA_WALK(pvmw, folio, vma, address, 0);
>> +	struct mmu_notifier_range range;
>> +	enum ttu_flags flags = (enum ttu_flags)(long)arg;
>> +	struct mm_struct *mm = vma->vm_mm;
>> +	const unsigned long hsz = huge_page_size(hstate_vma(vma));

Okay I'll move this to the second place. I would prefer DEFINE_FOLIO_VMA_WALK
to be the first declaration.


> 
> I would move const variables all the way up.
> 
>> +	bool ret = true;
>> +	pte_t pteval;
>> +
>> +	/*
>> +	 * The try_to_unmap() is only passed a hugetlb folio in the case
>> +	 * where the hugetlb folio is poisoned.
>> +	 */
>> +	VM_WARN_ON_FOLIO(!folio_test_hwpoison(folio), folio);
>> +	VM_WARN_ON_ONCE(!(flags & TTU_HWPOISON));
>> +
>> +	range.end = vma_address_end(&pvmw);
>> +	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma->vm_mm,
>> +				address, range.end);
>> +	adjust_range_if_pmd_sharing_possible(vma, &range.start, &range.end);
>> +	mmu_notifier_invalidate_range_start(&range);
>> +
>> +	/* There is only a single mapping in a VMA. */
>> +	if (!page_vma_mapped_walk(&pvmw))
>> +		goto range_end;
>> +
>> +	address = pvmw.address;
> 
> Will address ever change due to the page_vma_mapped_walk()? I don't think so, right?
> 
> So this can instead become a VM_WARN_ON_ONCE, maybe?

Yes you are right. address is computed from pgoff_start which for hugetlb folio will
always be aligned.

> 
> 
> 
> Nothing else jumped at me, much clearer to me.
> 
> Acked-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>