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 Mon, May 04, 2026 at 02:44:43PM +0200, David Hildenbrand (Arm) wrote:
>On 5/3/26 02:38, Wei Yang wrote:
>> On Wed, Apr 29, 2026 at 08:55:07AM +0200, David Hildenbrand (Arm) wrote:
>>> On 4/29/26 04:49, Wei Yang wrote:
>>>>
>>>> Below is my proposed change:
>>>>
>>>> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
>>>> index a4d52fdb3056..6e915d35ae54 100644
>>>> --- a/mm/page_vma_mapped.c
>>>> +++ b/mm/page_vma_mapped.c
>>>> @@ -273,17 +273,21 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
>>>>  
>>>>  			if (softleaf_is_device_private(entry)) {
>>>>  				pvmw->ptl = pmd_lock(mm, pvmw->pmd);
>>>> -				return true;
>>>> +				if (pmd_same(pmde, pmdp_get_lockless(pvmw->pmd)))
>>>> +					return true;
>>>
>>> As we have a softleaf entry, I assume we wouldn't expect to get any other bits
>>> (access/dirty) set until we grab the lock. Verifying
>>> softleaf_is_device_private() again would be better cleaner, though.
>>>
>> 
>> Got it.
>> 
>>> But really, I do wonder if we should just have a "goto retry" back to the "pmde
>>> = pmdp_get_lockless(pvmw->pmd);" instead?
>>>
>> 
>> Sounds reasonable. See below.
>> 
>>>
>>> And now I wonder why we don't have a check_pmd() handling in there? :/
>>>
>>> Should we check for the pfn here?
>> 
>> Thanks for pointing out. I think you are right.
>> 
>> After re-read the code, more questions come up my mind. I am afraid we need
>> more cleanup for page_vma_mapped_walk().
>> 
>> Below is my finding based on current understanding:
>> 
>> 1. thp_migration_supported() seems not necessary
>> 
>>    The code reaches here means pmd_is_migration_entry() return true, which
>>    means CONFIG_ARCH_ENABLE_THP_MIGRATION is set, otherwise
>>    softleaf_from_pmd() should return softleaf_mk_none() which is not a
>>    migration softleaf.
>>    
>>    CONFIG_ARCH_ENABLE_THP_MIGRATION is set in turn means
>>    CONFIG_TRANSPARENT_HUGEPAGE is set, so thp_migration_supported() must
>>    returns true.
>> 
>> 2. if migration entry change under us, we may need to handle on pte level
>> 
>>    In pmd_is_migration_entry() -> !pmd_present() branch, we have:
>> 
>> 		if (!softleaf_is_migration(entry) ||
>> 		    !check_pmd(softleaf_to_pfn(entry), pvmw))
>> 			return not_found(pvmw);
>> 		return true;
>> 
>>    But I think we need do this:
>> 
>> 		if (softleaf_is_migration(entry)) {
>> 			if (check_pmd(softleaf_to_pfn(entry), pvmw))
>> 				return not_found(pvmw);
>> 			return true;
>> 		}
>> 
>>    Per my understanding, if the pmd_is_migration_entry() change under us, we
>>    need to handle on pte level. Just like pmd_trans_huge() case. Break the
>>    loop and return false seems not consistent.
>> 
>> 3. add proper check for device private entry
>> 
>>    For device private entry, currently we just grab lock and return. While
>>    according to the handling to pmd_trans_huge() and pmd_is_migration_entry(),
>>    we should:
>> 
>>      * re-validate it is still device private entry after pmd_lock()
>>      * check PVMW_MIGRATION
>>      * check_pmd()
>> 
>> 4. consolidate pmd entry handling
>> 
>>    Per my understanding, there are 4 cases for pmd entry handling:
>> 
>>      * pmd_trans_huge()
>>      * pmd_is_migration_entry()
>>      * pmd_is_device_private_entry()
>>      * !pmd_present()
>> 
>>    Now we handle them in a mixed state check, which complicates the logic. And
>>    the first three share similar logic. (If my above analysis is correct.)
>> 
>>      * grab pmd_lock()
>>      * re-validate pmde
>>      * check PVMW_MIGRATION
>>      * check_pmd
>> 
>>    Here I would like to take a more bold step: consolidate handling for these
>>    three cases.
>> 
>>    Below is what it would look like.
>> 
>> 	pmde = pmdp_get_lockless(pvmw->pmd);
>> 
>> 	if (pmd_trans_huge(pmde) || pmd_is_valid_softleaf(pmde)) {
>> 		unsigned long pfn;
>> 		bool is_migration;
>> 		bool for_migration;
>> 
>> 		pvmw->ptl = pmd_lock(mm, pvmw->pmd);
>> 		if (pmd_same(pmde, pmdp_get_lockless(pvmw->pmd))) {
>> 			is_migration = pmd_is_migration_entry(pmde);
>> 			for_migration = !!(pvmw->flags & PVMW_MIGRATION);
>> 
>> 			if (is_migration != for_migration)
>> 				return not_found(pvmw);
>> 
>> 			if (pmd_trans_huge(pmde))
>> 				pfn = pmd_pfn(pmde);
>> 			else
>> 				pfn = softleaf_to_pfn(softleaf_from_pmd(pmde));
>> 
>> 			if (!check_pmd(pfn, pvmw))
>> 				return not_found(pvmw);
>> 
>> 			return true;
>> 		}
>> 		/* THP pmd was split under us: handle on pte level */
>> 		spin_unlock(pvmw->ptl);
>> 		pvmw->ptl = NULL;
>> 	} else if (!pmd_present(pmde)) {
>> 		if ((pvmw->flags & PVMW_SYNC) &&
>> 		    thp_vma_suitable_order(vma, pvmw->address,
>> 					   PMD_ORDER) &&
>> 		    (pvmw->nr_pages >= HPAGE_PMD_NR))
>> 			sync_with_folio_pmd_zap(mm, pvmw->pmd);
>> 
>> 		step_forward(pvmw, PMD_SIZE);
>> 		continue;
>> 	}
>> 
>> 5. use "goto retry"
>> 
>>    As you mentioned above. Instead of "handle on pte level", go to
>>    pmdp_get_lockless() for retry. This looks more reasonable to me.
>>>
>>>> +				/* THP pmd was split under us: handle on pte level */
>>>> +				spin_unlock(pvmw->ptl);
>>>> +				pvmw->ptl = NULL;
>>>
>>>
>>>
>>>> +			} else {
>>>> +				if ((pvmw->flags & PVMW_SYNC) &&
>>>> +				    thp_vma_suitable_order(vma, pvmw->address,
>>>> +							   PMD_ORDER) &&
>>>> +				    (pvmw->nr_pages >= HPAGE_PMD_NR))
>>>> +					sync_with_folio_pmd_zap(mm, pvmw->pmd);
>>>> +
>>>> +				step_forward(pvmw, PMD_SIZE);
>>>> +				continue;
>>>>  			}
>>>> -
>>>> -			if ((pvmw->flags & PVMW_SYNC) &&
>>>> -			    thp_vma_suitable_order(vma, pvmw->address,
>>>> -						   PMD_ORDER) &&
>>>> -			    (pvmw->nr_pages >= HPAGE_PMD_NR))
>>>> -				sync_with_folio_pmd_zap(mm, pvmw->pmd);
>>>> -
>>>> -			step_forward(pvmw, PMD_SIZE);
>>>> -			continue;
>>>>  		}
>>>>  		if (!map_pte(pvmw, &pmde, &ptl)) {
>>>>  			if (!pvmw->pte)
>>>>
>>>> After this, we could simplify the logic in try_to_migrate_one() as:
>>>>
>>>> @@ -2471,14 +2471,10 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma,
>>>>                                  * so we can detect this scenario and properly
>>>>                                  * abort the walk.
>>>>                                  */
>>>> -                               if (split_huge_pmd_locked(vma, pvmw.address,
>>>> -                                                     pvmw.pmd, true)) {
>>>> -                                       page_vma_mapped_walk_done(&pvmw);
>>>> -                                       break;
>>>> -                               }
>>>> -                               flags &= ~TTU_SPLIT_HUGE_PMD;
>>>> -                               page_vma_mapped_walk_restart(&pvmw);
>>>> -                               continue;
>>>> +                               ret = split_huge_pmd_locked(vma, pvmw.address,
>>>> +                                                     pvmw.pmd, true);
>>>> +                               page_vma_mapped_walk_done(&pvmw);
>>>> +                               break;
>>>>                         }
>>>
>>> Right. But just to be clear: Let's split the page_vma_mapped_walk() validation
>>> (which looks like a bugfix to me) from the other optimization.
>>>
>> 
>> Sure, maybe we can split the page_vma_mapped_walk() cleanup out to another
>> patch set for better reviewing?
>
>Yes, but I assume it could even be fixes?

Agree.

For those above proposed changes, #2 and #3 is proper to be fixes.

 2. if migration entry change under us, we may need to handle on pte level
 3. add proper check for device private entry

The corresponding commit to fix are

   commit 616b8371539a6c487404c3b8fb04078016dab4ba
   Author: Zi Yan <zi.yan@cs.rutgers.edu>
   Date:   Fri Sep 8 16:10:57 2017 -0700
   
       mm: thp: enable thp migration in generic path

   commit 65edfda6f3f2e58f757485a056e4f1775a1404a8
   Author: Balbir Singh <balbirs@nvidia.com>
   Date:   Wed Oct 1 16:56:55 2025 +1000

       mm/rmap: extend rmap and migration support device-private entries

As Andrew suggested, I would send fixes and cleanup separately.

After all these settled down, I will respin this thread.

-- 
Wei Yang
Help you, Help me