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

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

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)
...

> +
>  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*

> + *
> + * 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)



-- 
Cheers,

David