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

["Mika Penttilä" <mpenttil@redhat.com>]

On 7/7/25 05:35, Balbir Singh wrote:
> 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

Yes seems clearing PageAnonExclusive doesn't fail for device private pages in the end, 
but the 3/12 patch doesn't even try to clear PageAnonExclusive with your changes afaics,
which is a separate issue.

And __split_huge_page_to_list_to_order() (return value is not checked) can fail for out of memory.
So think you can not just assume split just works. If late split is a requirement (which I can understand is),
you should be prepared to rollback somehow the operation.

>
--Mika