Re: [PATCH 1/2] mm/huge_memory: return true if split_huge_pmd_locked() split PMD to migration entry

["David Hildenbrand (Arm)" <david@kernel.org>]

On 4/15/26 03:08, Wei Yang wrote:
> When @freeze is set to true, split_huge_pmd_locked() is intended to
> split the PMD to migration entry. But if it doesn't manage to clear
> PageAnonExclusive(), it just split PMD and leave the folio mapped
> through PTE.
> 
> This patch let split_huge_pmd_locked() return true to indicate it does
> split PMD to migration entry. With this knowledge, we can return
> directly in try_to_migrate_one() if it does.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Cc: Gavin Guo <gavinguo@igalia.com>
> Cc: "David Hildenbrand (Red Hat)" <david@kernel.org>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Lance Yang <lance.yang@linux.dev>
> ---

[...]

>  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 970e077019b7..ec84bb4a0cc3 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3087,7 +3087,7 @@ static void __split_huge_zero_page_pmd(struct vm_area_struct *vma,
>  	pmd_populate(mm, pmd, pgtable);
>  }
>  
> -static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
> +static bool __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  		unsigned long haddr, bool freeze)
>  {
>  	struct mm_struct *mm = vma->vm_mm;
> @@ -3096,7 +3096,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  	pgtable_t pgtable;
>  	pmd_t old_pmd, _pmd;
>  	bool soft_dirty, uffd_wp = false, young = false, write = false;
> -	bool anon_exclusive = false, dirty = false;
> +	bool anon_exclusive = false, dirty = false, ret = false;
>  	unsigned long addr;
>  	pte_t *pte;
>  	int i;
> @@ -3118,13 +3118,13 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  		if (arch_needs_pgtable_deposit())
>  			zap_deposited_table(mm, pmd);
>  		if (vma_is_special_huge(vma))
> -			return;
> +			return ret;

Why not "return false" in these cases where it really can always only false?

>  		if (unlikely(pmd_is_migration_entry(old_pmd))) {
>  			const softleaf_t old_entry = softleaf_from_pmd(old_pmd);
>  
>  			folio = softleaf_to_folio(old_entry);
>  		} else if (is_huge_zero_pmd(old_pmd)) {
> -			return;
> +			return ret;
>  		} else {
>  			page = pmd_page(old_pmd);
>  			folio = page_folio(page);
> @@ -3136,7 +3136,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  			folio_put(folio);
>  		}
>  		add_mm_counter(mm, mm_counter_file(folio), -HPAGE_PMD_NR);
> -		return;
> +		return ret;
>  	}
>  
>  	if (is_huge_zero_pmd(*pmd)) {
> @@ -3149,7 +3149,8 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  		 * small page also write protected so it does not seems useful
>  		 * to invalidate secondary mmu at this time.
>  		 */
> -		return __split_huge_zero_page_pmd(vma, haddr, pmd);
> +		__split_huge_zero_page_pmd(vma, haddr, pmd);
> +		return ret;
>  	}
>  
>  	if (pmd_is_migration_entry(*pmd)) {
> @@ -3309,6 +3310,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  			VM_WARN_ON(!pte_none(ptep_get(pte + i)));
>  			set_pte_at(mm, addr, pte + i, entry);
>  		}
> +		ret = true;
>  	} else if (pmd_is_device_private_entry(old_pmd)) {
>  		pte_t entry;
>  		swp_entry_t swp_entry;
> @@ -3366,14 +3368,17 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>  
>  	smp_wmb(); /* make pte visible before pmd */
>  	pmd_populate(mm, pmd, pgtable);
> +	return ret;
>  }
>  
> -void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
> +bool split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
>  			   pmd_t *pmd, bool freeze)
>  {
>  	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);
> +		return __split_huge_pmd_locked(vma, pmd, address, freeze);
> +	else
> +		return false;

No need for the "else".

>  }
>  
>  void __split_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 78b7fb5f367c..91fb495bebbe 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -2464,13 +2464,18 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
>  
>  			if (flags & TTU_SPLIT_HUGE_PMD) {
>  				/*
> -				 * split_huge_pmd_locked() might leave the
> +				 * If split_huge_pmd_locked() does split PMD
> +				 * to migration entry, we are done.
> +				 * If split_huge_pmd_locked() leave the
>  				 * folio mapped through PTEs. Retry the walk
>  				 * so we can detect this scenario and properly
>  				 * abort the walk.

Couldn't we just abort right away, based on the return value?

-- 
Cheers,

David