Re: [PATCH 4/7] mm/migrate: add batch-copy path in migrate_pages_batch

["David Hildenbrand (Arm)" <david@kernel.org>]

On 5/20/26 17:06, Garg, Shivank wrote:
> 
> 
> On 5/11/2026 9:10 PM, David Hildenbrand (Arm) wrote:
>> On 4/28/26 17:50, Shivank Garg wrote:
>>> Add folios_mc_copy() which walks list of src and dst folios in lockstep,
>>> and copies folio content via folio_mc_copy(). folios_cnt parameter is
>>> unused here, but is part of the offload_copy callback signature used by
>>> later patches in the series.
>>>
>>> Split unmapped folios into batch-eligible (unmap_batch/dst_batch) and
>>> standard (unmap_single/dst_single) lists, gated by the
>>> migrate_offload_enabled which is off by default. So, when no offload
>>> driver is active, the branch is never taken and everything goes
>>> through the standard path.
>>>
>>> After TLB flush, batch copy the eligible folios via folios_mc_copy()
>>> and pass already_copied=true into migrate_folios_move() so
>>> __migrate_folio() skips the per-folio copy.
>>>
>>> On batch copy failure, already_copied flag stays false and each folio
>>> fall back to individual copy.
>>>
>>> Signed-off-by: Shivank Garg <shivankg@amd.com>
>>> ---
>>>  include/linux/mm.h |  2 ++
>>>  mm/migrate.c       | 61 +++++++++++++++++++++++++++++++++++-----------
>>>  mm/util.c          | 30 +++++++++++++++++++++++
>>>  3 files changed, 79 insertions(+), 14 deletions(-)
>>
>> [...]
>>
>>>  
>>> +DEFINE_STATIC_KEY_FALSE(migrate_offload_enabled);
>>> +
>>>  static const struct movable_operations *offline_movable_ops;
>>>  static const struct movable_operations *zsmalloc_movable_ops;
>>>  
>>> @@ -1724,6 +1727,12 @@ static int migrate_hugetlbs(struct list_head *from, new_folio_t get_new_folio,
>>>  	return nr_failed;
>>>  }
>>>  
>>> +/* movable_ops folios have their own migrate path */
>>> +static bool folio_supports_batch_copy(struct folio *folio)
>>> +{
>>> +	return likely(!page_has_movable_ops(&folio->page));
>>> +}
>>
>> As these things are not actually folios (and callers will have to be taught to
>> distinguish them way, way earlier), I guess you should make this
>>
>> /* movable_ops pages have a separate migration path */
>> static bool page_supports_batch_copy(struct page *page)
>> ...
> 
> I'll change this.
> 
>>
>>> +
>>>  static void migrate_folios_move(struct list_head *src_folios,
>>>  		struct list_head *dst_folios,
>>>  		free_folio_t put_new_folio, unsigned long private,
>>> @@ -1752,7 +1761,7 @@ static void migrate_folios_move(struct list_head *src_folios,
>>>  		/*
>>>  		 * The rules are:
>>>  		 *	0: folio will be freed
>>> -		 *	-EAGAIN: stay on the unmap_folios list
>>> +		 *	-EAGAIN: stay on the src_folios list
>>>  		 *	Other errno: put on ret_folios list
>>>  		 */
>>>  		switch (rc) {
>>
>>
>> [...]
>>
>>> --- a/mm/util.c
>>> +++ b/mm/util.c
>>> @@ -778,6 +778,36 @@ int folio_mc_copy(struct folio *dst, struct folio *src)
>>>  }
>>>  EXPORT_SYMBOL(folio_mc_copy);
>>>  
>>> +/**
>>> + * folios_mc_copy - Copy the contents of list of folios.
>>> + * @dst_list: destination folio list.
>>> + * @src_list: source folio list.
>>> + * @folios_cnt: unused here, present for callback signature compatibility.
>>> + *
>>> + * Walks list of src and dst folios in lockstep and copies folio
>>> + * content via folio_mc_copy(). The caller must ensure both lists have
>>> + * the same number of entries. This may sleep.
>>
>> This *function*
>>
> 
> Will add function.
> 
>>> + *
>>> + * Return: 0 on success, negative errno on failure.
>>> + */
>>> +int folios_mc_copy(struct list_head *dst_list, struct list_head *src_list,
>>> +		unsigned int __always_unused folios_cnt)
>>> +{
>>> +	struct folio *src, *dst;
>>> +	int ret;
>>> +
>>> +	dst = list_first_entry(dst_list, struct folio, lru);
>>> +	list_for_each_entry(src, src_list, lru) {
>>> +		ret = folio_mc_copy(dst, src);
>>> +		if (ret)
>>> +			return ret;
>>> +		dst = list_next_entry(dst, lru);
>>> +	}
>>
>> Wouldn't it be cleaner to remember "already copied" immediately after we ...
>> performed the copy? (succeeded with folio_mc_copy)
> 
> IIUC, you mean something like:
> 	list_for_each_entry(src, src_list, lru) {
> 		ret = folio_mc_copy(dst, src);
> 		if (ret)
> 			return ret;
> 		dst->migrate_info |= FOLIO_CONTENT_COPIED;
> 		dst = list_next_entry(dst, lru);
> 	}
> 
> This will be cleaner. However, it will make the folios_mc_copy() specific for
> page migration and can't be reused by other potential callers.
> And offload driver (like DCBM) must set the already copied marker
> themselves for successful copies.

Right, maybe some helper for that would be possible?

> This will have an upside that on partial-success (even a single failed copy),
> it will no longer forces re-copy of the whole batch.
> If that's the intention, I'll move this to migrate.c itself.

True. I'd probably have to take a look at the end result to see if it looks
better that way.

-- 
Cheers,

David