Re: [v1 resend 08/12] mm/thp: add split during migration support

[Balbir Singh <balbirs@nvidia.com>]

On 7/5/25 13:17, Mika Penttilä wrote:
> 
>>>>> +static void migrate_vma_split_pages(struct migrate_vma *migrate,
>>>>> +					unsigned long idx, unsigned long addr,
>>>>> +					struct folio *folio)
>>>>> +{
>>>>> +	unsigned long i;
>>>>> +	unsigned long pfn;
>>>>> +	unsigned long flags;
>>>>> +
>>>>> +	folio_get(folio);
>>>>> +	split_huge_pmd_address(migrate->vma, addr, true);
>>>>> +	__split_huge_page_to_list_to_order(folio_page(folio, 0), NULL, 0, true);
>>>> We already have reference to folio, why is folio_get() needed ?
>>>>
>>>> Splitting the page splits pmd for anon folios, why is there split_huge_pmd_address() ?
>>> Oh I see 
>>> +	if (!isolated)
>>> +		unmap_folio(folio);
>>>
>>> which explains the explicit split_huge_pmd_address(migrate->vma, addr, true);
>>>
>>> Still, why the folio_get(folio);?
>>>  
>>>
>> That is for split_huge_pmd_address, when called with freeze=true, it drops the
>> ref count on the page
>>
>> 	if (freeze)
>> 		put_page(page);
>>
>> Balbir
>>
> yeah I guess you could have used the pmd_migration path in __split_huge_pmd_locked, and not use freeze because you have installed the migration pmd entry already.
> Which brings to a bigger concern, that you do need the freeze semantics, like clear PageAnonExclusive (which may fail). I think you did not get this part
> right in the 3/12 patch. And in this patch, you can't assume the split succeeds, which would mean you can't migrate the range at all.
> Doing the split this late is quite problematic all in all.
> 

Clearing PageAnonExclusive will *not* fail for device private pages from what I can see in __folio_try_share_anon_rmap().
Doing the split late is a requirement due to the nature of the three stage migration operation, the other side
might fail to allocate THP sized pages and so the code needs to deal with it

Balbir Singh