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

[Zi Yan]

On 8 Jun 2026, at 11:25, David Hildenbrand (Arm) wrote:

> 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@xxxxxxx>
>>>> ---
>>>>  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.

Maybe do this optimization in a separate patch to make it simpler to review.

BTW, the whole migration code has become more and more complicated due to
batching at different levels, I wonder if it is worth the complication to
optimize for the rare memory error case.

Best Regards,
Yan, Zi