Re: [v5 05/15] mm/migrate_device: handle partially mapped folios during collection

[Chris Mason <clm@meta.com>]

On 9/19/25 4:36 AM, Balbir Singh wrote:
> On 9/19/25 02:42, Chris Mason wrote:
>> On Mon,  8 Sep 2025 10:04:38 +1000 Balbir Singh <balbirs@nvidia.com> wrote:
>>
>>> Extend migrate_vma_collect_pmd() to handle partially mapped large
>>> folios that require splitting before migration can proceed.
>>>
>>> During PTE walk in the collection phase, if a large folio is only
>>> partially mapped in the migration range, it must be split to ensure
>>> the folio is correctly migrated.
>>>
>>> Signed-off-by: Balbir Singh <balbirs@nvidia.com>
>>> ---
>>>  mm/migrate_device.c | 94 +++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 94 insertions(+)
>>>
>>> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
>>> index abd9f6850db6..f45ef182287d 100644
>>> --- a/mm/migrate_device.c
>>> +++ b/mm/migrate_device.c
>>> @@ -54,6 +54,53 @@ static int migrate_vma_collect_hole(unsigned long start,
>>>  	return 0;
>>>  }
>>>  
>>> +/**
>>> + * migrate_vma_split_folio() - Helper function to split a THP folio
>>> + * @folio: the folio to split
>>> + * @fault_page: struct page associated with the fault if any
>>> + *
>>> + * Returns 0 on success
>>> + */
>>> +static int migrate_vma_split_folio(struct folio *folio,
>>> +				   struct page *fault_page)
>>> +{
>>> +	int ret;
>>> +	struct folio *fault_folio = fault_page ? page_folio(fault_page) : NULL;
>>> +	struct folio *new_fault_folio = NULL;
>>> +
>>> +	if (folio != fault_folio) {
>>> +		folio_get(folio);
>>> +		folio_lock(folio);
>>> +	}
>>
>> Can fault_folio ever be non-null and different from folio? Apologies for
>> not knowing the lock ordering rules but this jumps out.
>>
> 
> Yes, migration can occur in fault context or be driver driven
> 
>>> +
>>> +	ret = split_folio(folio);
>>> +	if (ret) {
>>> +		if (folio != fault_folio) {
>>> +			folio_unlock(folio);
>>> +			folio_put(folio);
>>> +		}
>>> +		return ret;
>>> +	}
>>> +
>>> +	new_fault_folio = fault_page ? page_folio(fault_page) : NULL;
>>> +
>>> +	/*
>>> +	 * Ensure the lock is held on the correct
>>> +	 * folio after the split
>>> +	 */
>>> +	if (!new_fault_folio) {
>>> +		folio_unlock(folio);
>>> +		folio_put(folio);
>>> +	} else if (folio != new_fault_folio) {
>>> +		folio_get(new_fault_folio);
>>> +		folio_lock(new_fault_folio);
>>> +		folio_unlock(folio);
>>> +		folio_put(folio);
>>> +	}
>>
>> Same question here, do we need trylocks?
>>
> 
> Since we had the folio lock before, the assumption is that we can
> still grab the lock after split and it's OK to wait, since this
> is not a hot-path.

I think the lock ordering rules either let us take two folios without
trylock or they don't...holding the lock in the past shouldn't change
things?  The same holds true above, two locks + no ordering, one or both
of these locking sites should deadlock.

But obviously I'm a tourist here, and I need to refresh the review
queue, so I'll move on ;)  Thanks for taking a look at it.

-chris