[Reforwarding - not sure if the original response made it back]
On 11/19/25 07:36, David Hildenbrand (Red Hat) wrote:
> On 15.11.25 09:40, Balbir Singh wrote:
>> Unmapped was added as a parameter to __folio_split() and related
>> call sites to support splitting of folios already in the midst
>> of a migration. This special case arose for device private folio
>> migration since during migration there could be a disconnect between
>> source and destination on the folio size.
>>
>> Introduce folio_split_unmapped() to handle this special case. Also
>> refactor code and add __folio_freeze_and_split_unmapped() helper that
>> is common to both __folio_split() and folio_split_unmapped().
>>
>> This in turn removes the special casing introduced by the unmapped
>> parameter in __folio_split().
>>
>
> I was briefly wondering: why can't we just detect at the beginning of the
> __folio_split() that the folio is unmapped (!folio_mapped()) and then
> continue assuming the folio is unmapped?
>
> The folio is locked, so it shouldn't just become mapped again.
>
> But then I looked into the details and figured that we will also not try to
> remap (replace migration entries) and focus on anon folios only.
>
> I think we really have to document this properly. See below.
>
> [...]
>
Your observations are correct.
>> +/*
>
> Can we have proper kerneldoc?
>
>> + * This function is a helper for splitting folios that have already been
>> unmapped.
>> + * The use case is that the device or the CPU can refuse to migrate THP
>> pages in
>> + * the middle of migration, due to allocation issues on either side
>> + *
>> + * The high level code is copied from __folio_split, since the pages are
>> anonymous
>> + * and are already isolated from the LRU, the code has been simplified to
>> not
>> + * burden __folio_split with unmapped sprinkled into the code.
>
> Please drop the history of how this code was obtained 🙂 Focus on ducmenting
> what the function does, what it expects from the caller, and what the result
> of the operation will be.
>
>> + *
>> + * None of the split folios are unlocked
>
>
> Looking into the details, I think this function also does not
>
> (a) remap the folio
>
> (b) call things like free_folio_and_swap_cache()
>
> Which locks do have to be held by the caller? I'd assume the anon vma lock
> and the folio lock?
>
> Would this function currently work for anon folios that are in the swapcache?
>
> And I assume this function works for ZONE_DEVICE and !ZONE_DEVICE?
>
>
> Please carefully document all that (locks held, folio isolated, folio
> unmapped, will not remap the folio, anon folios only, etc).
Something like this below?
/**
* folio_split_unmapped() - split a large anon folio that is already unmapped
* @folio: folio to split
* @new_order: the order of folios after split
*
* This function is a helper for splitting folios that have already been
* unmapped. The use case is that the device or the CPU can refuse to migrate
* THP pages in the middle of migration, due to allocation issues on either
* side.
*
* anon_vma_lock is not required to be held, mmap_read_lock() or
* mmap_write_lock() should be held. @folio is expected to be locked by the
* caller. device-private and non device-private folios are supported along
* with folios that are in the swapcache. @folio should also be unmapped and
* isolated from LRU (if applicable)
*
* Upon return, the folio is not remapped, split folios are not added to LRU,
* free_folio_and_swap_cache() is not called, and new folios remain locked.
*
* Return: 0 on success, -EAGAIN if the folio cannot be split (e.g., due to
* insufficient reference count or extra pins).
*/
>
>> + */
>> +int folio_split_unmapped(struct folio *folio, unsigned int new_order)
>> +{
>> + int extra_pins, ret = 0;
>> +
>> + VM_WARN_ON_ONCE_FOLIO(folio_mapped(folio), folio);
>> + VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
>> + VM_WARN_ON_ONCE_FOLIO(!folio_test_large(folio), folio);
>> + VM_WARN_ON_ONCE_FOLIO(!folio_test_anon(folio), folio);
>> +
>> + if (!can_split_folio(folio, 1, &extra_pins))
>> + return -EAGAIN;
>> +
>> + local_irq_disable();
>> + ret = __folio_freeze_and_split_unmapped(folio, new_order, &folio->page,
>> NULL,
>> + NULL, false, NULL, SPLIT_TYPE_UNIFORM,
>> + 0, NULL, extra_pins);
>> + local_irq_enable();
>> + return ret;
>> +}
>> +
>> /*
>> * This function splits a large folio into smaller folios of order
>> @new_order.
>> * @page can point to any page of the large folio to split. The split
>> operation
>> @@ -4127,12 +4171,12 @@ static int __folio_split(struct folio *folio,
>> unsigned int new_order,
>> * with the folio. Splitting to order 0 is compatible with all folios.
>> */
>> int __split_huge_page_to_list_to_order(struct page *page, struct list_head
>> *list,
>> - unsigned int new_order, bool unmapped)
>> + unsigned int new_order)
>> {
>> struct folio *folio = page_folio(page);
>> return __folio_split(folio, new_order, &folio->page, page, list,
>> - SPLIT_TYPE_UNIFORM, unmapped);
>> + SPLIT_TYPE_UNIFORM);
>> }
>> /**
>> @@ -4163,7 +4207,7 @@ int folio_split(struct folio *folio, unsigned int
>> new_order,
>> struct page *split_at, struct list_head *list)
>> {
>> return __folio_split(folio, new_order, split_at, &folio->page, list,
>> - SPLIT_TYPE_NON_UNIFORM, false);
>> + SPLIT_TYPE_NON_UNIFORM);
>> }
>> int min_order_for_split(struct folio *folio)
>> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
>> index 46dd68cfc503..05ce95f43ab9 100644
>> --- a/mm/migrate_device.c
>> +++ b/mm/migrate_device.c
>> @@ -909,8 +909,7 @@ static int migrate_vma_split_unmapped_folio(struct
>> migrate_vma *migrate,
>> folio_get(folio);
>> split_huge_pmd_address(migrate->vma, addr, true);
>> - ret = __split_huge_page_to_list_to_order(folio_page(folio, 0), NULL,
>> - 0, true);
>> + ret = folio_split_unmapped(folio, 0);
>> if (ret)
>> return ret;
>> migrate->src[idx] &= ~MIGRATE_PFN_COMPOUND;
>
> This is clearly a win.
>
Thanks for the review!
Balbir
