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

[Mika Penttilä]

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



--Mika