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

[Balbir Singh <balbirs@nvidia.com>]

On 7/5/25 11:55, Zi Yan wrote:
> On 4 Jul 2025, at 20:58, Balbir Singh wrote:
> 
>> On 7/4/25 21:24, Zi Yan wrote:
>>>
>>> s/pages/folio
>>>
>>
>> Thanks, will make the changes
>>
>>> Why name it isolated if the folio is unmapped? Isolated folios often mean
>>> they are removed from LRU lists. isolated here causes confusion.
>>>
>>
>> Ack, will change the name
>>
>>
>>>>   *
>>>>   * It calls __split_unmapped_folio() to perform uniform and non-uniform split.
>>>>   * It is in charge of checking whether the split is supported or not and
>>>> @@ -3800,7 +3799,7 @@ bool uniform_split_supported(struct folio *folio, unsigned int new_order,
>>>>   */
>>>>  static int __folio_split(struct folio *folio, unsigned int new_order,
>>>>  		struct page *split_at, struct page *lock_at,
>>>> -		struct list_head *list, bool uniform_split)
>>>> +		struct list_head *list, bool uniform_split, bool isolated)
>>>>  {
>>>>  	struct deferred_split *ds_queue = get_deferred_split_queue(folio);
>>>>  	XA_STATE(xas, &folio->mapping->i_pages, folio->index);
>>>> @@ -3846,14 +3845,16 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>>>>  		 * is taken to serialise against parallel split or collapse
>>>>  		 * operations.
>>>>  		 */
>>>> -		anon_vma = folio_get_anon_vma(folio);
>>>> -		if (!anon_vma) {
>>>> -			ret = -EBUSY;
>>>> -			goto out;
>>>> +		if (!isolated) {
>>>> +			anon_vma = folio_get_anon_vma(folio);
>>>> +			if (!anon_vma) {
>>>> +				ret = -EBUSY;
>>>> +				goto out;
>>>> +			}
>>>> +			anon_vma_lock_write(anon_vma);
>>>>  		}
>>>>  		end = -1;
>>>>  		mapping = NULL;
>>>> -		anon_vma_lock_write(anon_vma);
>>>>  	} else {
>>>>  		unsigned int min_order;
>>>>  		gfp_t gfp;
>>>> @@ -3920,7 +3921,8 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>>>>  		goto out_unlock;
>>>>  	}
>>>>
>>>> -	unmap_folio(folio);
>>>> +	if (!isolated)
>>>> +		unmap_folio(folio);
>>>>
>>>>  	/* block interrupt reentry in xa_lock and spinlock */
>>>>  	local_irq_disable();
>>>> @@ -3973,14 +3975,15 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
>>>>
>>>>  		ret = __split_unmapped_folio(folio, new_order,
>>>>  				split_at, lock_at, list, end, &xas, mapping,
>>>> -				uniform_split);
>>>> +				uniform_split, isolated);
>>>>  	} else {
>>>>  		spin_unlock(&ds_queue->split_queue_lock);
>>>>  fail:
>>>>  		if (mapping)
>>>>  			xas_unlock(&xas);
>>>>  		local_irq_enable();
>>>> -		remap_page(folio, folio_nr_pages(folio), 0);
>>>> +		if (!isolated)
>>>> +			remap_page(folio, folio_nr_pages(folio), 0);
>>>>  		ret = -EAGAIN;
>>>>  	}
>>>
>>> These "isolated" special handlings does not look good, I wonder if there
>>> is a way of letting split code handle device private folios more gracefully.
>>> It also causes confusions, since why does "isolated/unmapped" folios
>>> not need to unmap_page(), remap_page(), or unlock?
>>>
>>>
>>
>> There are two reasons for going down the current code path
> 
> After thinking more, I think adding isolated/unmapped is not the right
> way, since unmapped folio is a very generic concept. If you add it,
> one can easily misuse the folio split code by first unmapping a folio
> and trying to split it with unmapped = true. I do not think that is
> supported and your patch does not prevent that from happening in the future.
> 

I don't understand the misuse case you mention, I assume you mean someone can
get the usage wrong? The responsibility is on the caller to do the right thing
if calling the API with unmapped

> You should teach different parts of folio split code path to handle
> device private folios properly. Details are below.
> 
>>
>> 1. if the isolated check is not present, folio_get_anon_vma will fail and cause
>>    the split routine to return with -EBUSY
> 
> You do something below instead.
> 
> if (!anon_vma && !folio_is_device_private(folio)) {
> 	ret = -EBUSY;
> 	goto out;
> } else if (anon_vma) {
> 	anon_vma_lock_write(anon_vma);
> }
> 

folio_get_anon() cannot be called for unmapped folios. In our case the page has
already been unmapped. Is there a reason why you mix anon_vma_lock_write with
the check for device private folios?

> People can know device private folio split needs a special handling.
> 
> BTW, why a device private folio can also be anonymous? Does it mean
> if a page cache folio is migrated to device private, kernel also
> sees it as both device private and file-backed?
> 

FYI: device private folios only work with anonymous private pages, hence
the name device private.

> 
>> 2. Going through unmap_page(), remap_page() causes a full page table walk, which
>>    the migrate_device API has already just done as a part of the migration. The
>>    entries under consideration are already migration entries in this case.
>>    This is wasteful and in some case unexpected.
> 
> unmap_folio() already adds TTU_SPLIT_HUGE_PMD to try to split
> PMD mapping, which you did in migrate_vma_split_pages(). You probably
> can teach either try_to_migrate() or try_to_unmap() to just split
> device private PMD mapping. Or if that is not preferred,
> you can simply call split_huge_pmd_address() when unmap_folio()
> sees a device private folio.
> 
> For remap_page(), you can simply return for device private folios
> like it is currently doing for non anonymous folios.
> 

Doing a full rmap walk does not make sense with unmap_folio() and
remap_folio(), because

1. We need to do a page table walk/rmap walk again
2. We'll need special handling of migration <-> migration entries
   in the rmap handling (set/remove migration ptes)
3. In this context, the code is already in the middle of migration,
   so trying to do that again does not make sense.


> 
> For lru_add_split_folio(), you can skip it if a device private
> folio is seen.
> 
> Last, for unlock part, why do you need to keep all after-split folios
> locked? It should be possible to just keep the to-be-migrated folio
> locked and unlock the rest for a later retry. But I could miss something
> since I am not familiar with device private migration code.
> 

Not sure I follow this comment

Balbir