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/26/26 11:19, Wei Yang wrote:
> 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.


So what you are saying is that we should re-validate in page_vma_mapped_walk()
that we indeed still have a device-private entry after grabbing the lock?

That's what we do in map_pte() through pmd_same() check.

Likely we should apply the same model here!


-- 
Cheers,

David