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

[Wei Yang <richard.weiyang@gmail.com>]

On Fri, Apr 24, 2026 at 09:29:18PM +0200, David Hildenbrand (Arm) wrote:
>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?
>

Will adjust related places.

>>  		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".
>

Got it.

>>  }
>>  
>>  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?
>

Here is my understanding.

We get here when page_vma_mapped_walk() touch a pmd entry, with three cases:

  * pmd_trans_huge()
  * pmd_is_migration_entry()
  * pmd_is_device_private_entry()

For the first two cases, we grab pmd_lock() and then check the condition is
still valid before return. But for case 3, after grab pmd_lock(), it return
directly.

This may give chance for another thread to split pmd_is_device_private_entry()
to pte mapped, IIUC. For this case, we should restart the walk here.

Not sure I got it correctly.

-- 
Wei Yang
Help you, Help me