Re: [PATCH] mm/huge_memory: Simplify the split_huge_pmd_locked function

[David Hildenbrand]

On 24.04.25 12:50, Gavin Guo wrote:
> The split_huge_pmd_locked function currently performs redundant checks
> for migration entries and folio validation that are already handled by
> the page_vma_mapped_walk mechanism in try_to_migrate_one.
>
> Specifically, page_vma_mapped_walk already ensures that:
> - The folio is properly mapped in the given VMA area
> - pmd_trans_huge, pmd_devmap, and migration entry validation are
>    performed
>
> To leverage page_vma_mapped_walk's work, moving TTU_SPLIT_HUGE_PMD
> handling to the while loop checking, removing these duplicate checks
> from split_huge_pmd_locked and also removing the unnecessary folio
> argument since it's no longer needed for the validation.
>
> Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
> Link: https://lore.kernel.org/all/98d1d195-7821-4627-b518-83103ade56c0@xxxxxxxxxx/
> Signed-off-by: Gavin Guo <gavinguo@xxxxxxxxxx>
> ---

One could maybe split up the patch into a

a) Adjust try_to_migrate_one() + split_huge_pmd_locked() internals
b) Remove all the useless folio pointers we pass then around

[...]

>   
>   void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> -			   pmd_t *pmd, bool freeze, struct folio *folio)
> +			   pmd_t *pmd, bool freeze)
>   {
> -	bool pmd_migration = is_pmd_migration_entry(*pmd);
> -
> -	VM_WARN_ON_ONCE(folio && !folio_test_pmd_mappable(folio));
>   	VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE));
> -	VM_WARN_ON_ONCE(folio && !folio_test_locked(folio));
> -	VM_BUG_ON(freeze && !folio);
> -
> -	/*
> -	 * When the caller requests to set up a migration entry, we
> -	 * require a folio to check the PMD against. Otherwise, there
> -	 * is a risk of replacing the wrong folio.
> -	 */
> -	if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) || pmd_migration) {
> -		/*
> -		 * Do not apply pmd_folio() to a migration entry; and folio lock
> -		 * guarantees that it must be of the wrong folio anyway.
> -		 */
> -		if (folio && (pmd_migration || folio != pmd_folio(*pmd)))
> -			return;
> +	if (pmd_trans_huge(*pmd) || pmd_devmap(*pmd) ||
> +	    is_pmd_migration_entry(*pmd)) {
>   		__split_huge_pmd_locked(vma, pmd, address, freeze);

Can drop the {}

>   	}
>   }
>   
>   void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,


[...]

> @@ -2291,13 +2291,6 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
>   	if (flags & TTU_SYNC)
>   		pvmw.flags = PVMW_SYNC;
>   
> -	/*
> -	 * unmap_page() in mm/huge_memory.c is the only user of migration with
> -	 * TTU_SPLIT_HUGE_PMD and it wants to freeze.
> -	 */
> -	if (flags & TTU_SPLIT_HUGE_PMD)
> -		split_huge_pmd_address(vma, address, true, folio);
> -
>   	/*
>   	 * For THP, we have to assume the worse case ie pmd for invalidation.
>   	 * For hugetlb, it could be much worse if we need to do pud
> @@ -2323,9 +2316,21 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
>   	mmu_notifier_invalidate_range_start(&range);
>   
>   	while (page_vma_mapped_walk(&pvmw)) {
> -#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
>   		/* PMD-mapped THP migration entry */
>   		if (!pvmw.pte) {
> +			/*
> +			 * unmap_folio() in mm/huge_memory.c is the only user of
> +			 * migration with TTU_SPLIT_HUGE_PMD and it wants to
> +			 * freeze.
> +			 */

I think we drop that comment. Freezing is really the only reasonable 
thing to do here, because freezing is unmapping a PMD-mapped THP and 
installing a PTE table with PTE migration entries.

(the term freezing is misleading ...)


There is one case where "freezing" might fail (if the THP is exclusive 
and pinned), and that should be handled as expected I think: the caller
must check folio_mapped() either way to make sure everything is really
unmapped.

Thanks!

Acked-by: David Hildenbrand <david@xxxxxxxxxx>

--
Cheers,

David / dhildenb