linux-kernel - Re: [PATCH] mm/huge_memory: Simplify the split_huge_pmd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <91599a3c-e69e-4d79-bac5-5013c96203d7@redhat.com>
Date: Thu, 24 Apr 2025 22:44:12 +0200
From: David Hildenbrand <david@...hat.com>
To: Gavin Guo <gavinguo@...lia.com>, linux-mm@...ck.org,
 akpm@...ux-foundation.org
Cc: gshan@...hat.com, willy@...radead.org, ziy@...dia.com,
 linmiaohe@...wei.com, hughd@...gle.com, revest@...gle.com,
 kernel-dev@...lia.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/huge_memory: Simplify the split_huge_pmd_locked
 function

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@...hat.com>
> Link: https://lore.kernel.org/all/98d1d195-7821-4627-b518-83103ade56c0@redhat.com/
> Signed-off-by: Gavin Guo <gavinguo@...lia.com>
 > ---

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@...hat.com>

-- 
Cheers,

David / dhildenb