Re: [RESEND v7 10/29] mm: make PMD migration-entry splitting explicit

[David Hildenbrand (Arm)]

On 9/14/26 14:28, Usama Arif wrote:
> __split_huge_pmd() and friends take a "freeze" boolean that every caller
> has to pass and almost every caller passes as false. The name says nothing
> about what it selects, and the one thing it does select - PTE migration
> entries instead of PTE mappings - is only ever wanted by the rmap migration
> path.
> 
> Rename it to use_migration_entries, keep it private to mm/huge_memory.c,
> and add split_pmd_to_migration_entries() for try_to_migrate_one(), the only
> caller that wants it.
> 
> migrate_vma_split_unmapped_folio() also passed freeze=true, but only ever
> runs on a PMD that is already a migration entry, which the generic helper
> expands into PTE migration entries either way. Its folio_get() only existed
> to balance the put_page() that freeze=true performs, so both go.
> 
> No functional change intended.
> 
> Suggested-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> Signed-off-by: Usama Arif <usama.arif@xxxxxxxxx>

[...]

> +void split_pmd_to_migration_entries(struct vm_area_struct *vma,
> +				    unsigned long address, pmd_t *pmd);

Two tele tabbies please.

>  bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
>  			   pmd_t *pmdp, struct folio *folio);
>  void map_anon_folio_pmd_nopf(struct folio *folio, pmd_t *pmd,
> @@ -690,12 +690,14 @@ static inline void deferred_split_folio(struct folio *folio, bool partially_mapp
>  	do { } while (0)
>  
>  static inline void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> -		unsigned long address, bool freeze) {}
> +		unsigned long address) {}
>  static inline void split_huge_pmd_address(struct vm_area_struct *vma,
> -		unsigned long address, bool freeze) {}
> +		unsigned long address) {}
>  static inline void split_huge_pmd_locked(struct vm_area_struct *vma,
> -					 unsigned long address, pmd_t *pmd,
> -					 bool freeze) {}
> +					 unsigned long address, pmd_t *pmd) {}
> +static inline void
> +split_pmd_to_migration_entries(struct vm_area_struct *vma,
> +			       unsigned long address, pmd_t *pmd) {}

Dito.

>  
>  static inline bool unmap_huge_pmd_locked(struct vm_area_struct *vma,
>  					 unsigned long addr, pmd_t *pmdp,
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index ee8d46827ffdc..873887aed0bc2 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2033,7 +2033,7 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  		pte_free(dst_mm, pgtable);
>  		spin_unlock(src_ptl);
>  		spin_unlock(dst_ptl);
> -		__split_huge_pmd(src_vma, src_pmd, addr, false);
> +		__split_huge_pmd(src_vma, src_pmd, addr);
>  		return -EAGAIN;
>  	}
>  	add_mm_counter(dst_mm, MM_ANONPAGES, HPAGE_PMD_NR);
> @@ -2257,7 +2257,7 @@ vm_fault_t do_huge_pmd_wp_page(struct vm_fault *vmf)
>  	folio_unlock(folio);
>  	spin_unlock(vmf->ptl);
>  fallback:
> -	__split_huge_pmd(vma, vmf->pmd, vmf->address, false);
> +	__split_huge_pmd(vma, vmf->pmd, vmf->address);
>  	return VM_FAULT_FALLBACK;
>  }
>  
> @@ -3190,7 +3190,7 @@ static void __split_huge_zero_page_pmd(struct vm_area_struct *vma,
>  }
>  
>  static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> -		unsigned long haddr, bool freeze)
> +		unsigned long haddr, bool use_migration_entries)


Just curious: s/use_migration_entries/to_migration_entries/

>  {
>  	struct mm_struct *mm = vma->vm_mm;
>  	struct folio *folio;
> @@ -3291,10 +3291,10 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  		 * folios w.r.t anon exclusive handling. See the comments for
>  		 * folio handling and anon_exclusive below.
>  		 */
> -		if (freeze && anon_exclusive &&
> +		if (use_migration_entries && anon_exclusive &&
>  		    folio_try_share_anon_rmap_pmd(folio, page))
> -			freeze = false;
> -		if (!freeze) {
> +			use_migration_entries = false;
> +		if (!use_migration_entries) {
>  			rmap_t rmap_flags = RMAP_NONE;
>  
>  			folio_ref_add(folio, HPAGE_PMD_NR - 1);
> @@ -3344,11 +3344,11 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  		VM_WARN_ON_FOLIO(!folio_test_anon(folio), folio);
>  
>  		/*
> -		 * Without "freeze", we'll simply split the PMD, propagating the
> -		 * PageAnonExclusive() flag for each PTE by setting it for
> +		 * Without migration entries, we'll simply split the PMD and

"When not splitting to migration entries .."

> +		 * propagate the PageAnonExclusive() flag for each PTE by setting it for
>  		 * each subpage -- no need to (temporarily) clear.

While at it: s/subpage/page/

>  		 *
> -		 * With "freeze" we want to replace mapped pages by
> +		 * With migration entries we want to replace mapped pages by

"When splitting to migration entries ..."

>  		 * migration entries right away. This is only possible if we
>  		 * managed to clear PageAnonExclusive() -- see
>  		 * set_pmd_migration_entry().
> @@ -3359,10 +3359,10 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  		 * See folio_try_share_anon_rmap_pmd(): invalidate PMD first.
>  		 */
>  		anon_exclusive = PageAnonExclusive(page);
> -		if (freeze && anon_exclusive &&
> +		if (use_migration_entries && anon_exclusive &&
>  		    folio_try_share_anon_rmap_pmd(folio, page))
> -			freeze = false;
> -		if (!freeze) {
> +			use_migration_entries = false;
> +		if (!use_migration_entries) {
>  			rmap_t rmap_flags = RMAP_NONE;
>  

[...]

>  
>  	smp_wmb(); /* make pte visible before pmd */
> @@ -3477,15 +3477,28 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  }
>  
>  void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> -			   pmd_t *pmd, bool freeze)
> +			   pmd_t *pmd)

While at it ...

>  {
>  	VM_WARN_ON_ONCE(!IS_ALIGNED(address, HPAGE_PMD_SIZE));
>  	if (pmd_trans_huge(*pmd) || pmd_is_valid_softleaf(*pmd))
> -		__split_huge_pmd_locked(vma, pmd, address, freeze);
> +		__split_huge_pmd_locked(vma, pmd, address, false);
> +}
> +
> +/*
> + * Split a present PMD into PTE migration entries, for the rmap migration
> + * walker.  Like split_huge_pmd_locked(), the caller must hold the PMD lock and
> + * must already be inside an mmu_notifier invalidate range.
> + */

I'd prefer kerneldoc but I'll let you decide.

> +void split_pmd_to_migration_entries(struct vm_area_struct *vma,
> +				    unsigned long address, pmd_t *pmd)

two tabs ...

[...]

> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
> @@ -918,12 +918,7 @@ static int migrate_vma_split_unmapped_folio(struct migrate_vma *migrate,
>  	unsigned long flags;
>  	int ret = 0;
>  
> -	/*
> -	 * take a reference, since split_huge_pmd_address() with freeze = true
> -	 * drops a reference at the end.
> -	 */
> -	folio_get(folio);
> -	split_huge_pmd_address(migrate->vma, addr, true);
> +	split_huge_pmd_address(migrate->vma, addr);


Everything up to this point was trivial :)

You say that it already is unmapped (which makes sense looking at the
function name).

In VM_WARN_ON_ONCE_FOLIO(folio_mapped(folio), folio) we verify.

Did you run the hmm selftests with DEBUG_VM enabled, just to be sure? I remember
they exercise at least some of the THP logic in here.


>  	ret = folio_split_unmapped(folio, 0);
>  	if (ret)
>  		return ret;
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 2888ee638d872..ee33bbb421008 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -530,7 +530,7 @@ static inline long change_pmd_range(struct mmu_gather *tlb,
>  		if (pmd_is_huge(_pmd)) {
>  			if ((next - addr != HPAGE_PMD_SIZE) ||
>  			    pgtable_split_needed(vma, cp_flags)) {
> -				__split_huge_pmd(vma, pmd, addr, false);
> +				__split_huge_pmd(vma, pmd, addr);
>  				/*
>  				 * For file-backed, the pmd could have been
>  				 * cleared; make sure pmd populated if
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 5332c52909be1..feb751e29b992 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -2290,7 +2290,7 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>  				 * restart so we can process the PTE-mapped THP.
>  				 */
>  				split_huge_pmd_locked(vma, pvmw.address,
> -						      pvmw.pmd, false);
> +						      pvmw.pmd);

You can feel brave and squeeze it into a single line now :)


Overall LGTM.

-- 
Cheers,

David