Re: [PATCH v5] mm/migrate: split source folio if it is on deferred split list

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2024/3/23 03:33, Zi Yan wrote:
> From: Zi Yan <ziy@nvidia.com>
> 
> If the source folio is on deferred split list, it is likely some subpages
> are not used. Split it before migration to avoid migrating unused subpages.
> 
> Commit 616b8371539a6 ("mm: thp: enable thp migration in generic path")
> did not check if a THP is on deferred split list before migration, thus,
> the destination THP is never put on deferred split list even if the source
> THP might be. The opportunity of reclaiming free pages in a partially
> mapped THP during deferred list scanning is lost, but no other harmful
> consequence is present[1].
> 
>  From v4:
> 1. Simplify _deferred_list check without locking and do not count as
>     migration failures. (per Matthew Wilcox)
> 
>  From v3:
> 1. Guarded deferred list code behind CONFIG_TRANSPARENT_HUGEPAGE to avoid
>     compilation error (per SeongJae Park).
> 
>  From v2:
> 1. Split the source folio instead of migrating it (per Matthew Wilcox)[2].
> 
>  From v1:
> 1. Used dst to get correct deferred split list after migration
>     (per Ryan Roberts).
> 
> [1]: https://lore.kernel.org/linux-mm/03CE3A00-917C-48CC-8E1C-6A98713C817C@nvidia.com/
> [2]: https://lore.kernel.org/linux-mm/Ze_P6xagdTbcu1Kz@casper.infradead.org/
> 
> Fixes: 616b8371539a ("mm: thp: enable thp migration in generic path")
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
>   mm/migrate.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index ab9856f5931b..6bd9319624a3 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1652,6 +1652,29 @@ static int migrate_pages_batch(struct list_head *from,
>   
>   			cond_resched();
>   
> +			/*
> +			 * The rare folio on the deferred split list should
> +			 * be split now. It should not count as a failure.
> +			 * Only check it without removing it from the list.
> +			 * Since the folio can be on deferred_split_scan()
> +			 * local list and removing it can cause the local list
> +			 * corruption. Folio split process below can handle it
> +			 * with the help of folio_ref_freeze().
> +			 *
> +			 * nr_pages > 2 is needed to avoid checking order-1
> +			 * page cache folios. They exist, in contrast to
> +			 * non-existent order-1 anonymous folios, and do not
> +			 * use _deferred_list.
> +			 */
> +			if (nr_pages > 2 &&
> +			   !list_empty(&folio->_deferred_list)) {
> +				if (try_split_folio(folio, from) == 0) {

IMO, we should move the split folios into the 'split_folios' list 
instead of the 'from' list, otherwise there might be unhandled folios 
remaining in the from list.

> +					stats->nr_thp_split += is_thp;
> +					stats->nr_split++;
> +					continue;
> +				}
> +			}
> +
>   			/*
>   			 * Large folio migration might be unsupported or
>   			 * the allocation might be failed so we should retry
> 
> base-commit: 08a487ab26d541a3bd0adaee144f684b724d233b