[3/3] migrate_pages: try migrate in batch asynchronously firstly

  When we have locked more than one folios, we cannot wait the lock or
bit (e.g., page lock, buffer head lock, writeback bit) synchronously.
Otherwise deadlock may be triggered.  This make it hard to batch the
synchronous migration directly.

This patch re-enables batching synchronous migration via trying to
migrate in batch asynchronously firstly.  And any folios that are
failed to be migrated asynchronously will be migrated synchronously
one by one.

Test shows that this can restore the TLB flushing batching performance
for synchronous migration effectively.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: "Xu, Pengfei" <pengfei.xu@intel.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Stefan Roesch <shr@devkernel.io>
Cc: Tejun Heo <tj@kernel.org>
Cc: Xin Hao <xhao@linux.alibaba.com>
Cc: Zi Yan <ziy@nvidia.com>
Cc: Yang Shi <shy828301@gmail.com>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/migrate.c | 65 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 10 deletions(-)
  

On Fri, 24 Feb 2023, Huang Ying wrote:

> When we have locked more than one folios, we cannot wait the lock or
> bit (e.g., page lock, buffer head lock, writeback bit) synchronously.
> Otherwise deadlock may be triggered.  This make it hard to batch the
> synchronous migration directly.
> 
> This patch re-enables batching synchronous migration via trying to
> migrate in batch asynchronously firstly.  And any folios that are
> failed to be migrated asynchronously will be migrated synchronously
> one by one.
> 
> Test shows that this can restore the TLB flushing batching performance
> for synchronous migration effectively.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Hugh Dickins <hughd@google.com>

I'm not sure whether my 48 hours on two machines counts for a
Tested-by: Hugh Dickins <hughd@google.com>
or not; but it certainly looks like you've fixed my deadlock.

> Cc: "Xu, Pengfei" <pengfei.xu@intel.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Stefan Roesch <shr@devkernel.io>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Xin Hao <xhao@linux.alibaba.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Yang Shi <shy828301@gmail.com>
> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>  mm/migrate.c | 65 ++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 55 insertions(+), 10 deletions(-)

I was initially disappointed, that this was more complicated than I had
thought it should be; but came to understand why.  My "change the mode
to MIGRATE_ASYNC after the first" model would have condemned most of the
MIGRATE_SYNC batch of pages to be handled as lightly as MIGRATE_ASYNC:
not good enough, you're right be trying harder here.

> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 91198b487e49..c17ce5ee8d92 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1843,6 +1843,51 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>  	return rc;
>  }
>  
> +static int migrate_pages_sync(struct list_head *from, new_page_t get_new_page,
> +		free_page_t put_new_page, unsigned long private,
> +		enum migrate_mode mode, int reason, struct list_head *ret_folios,
> +		struct list_head *split_folios, struct migrate_pages_stats *stats)
> +{
> +	int rc, nr_failed = 0;
> +	LIST_HEAD(folios);
> +	struct migrate_pages_stats astats;
> +
> +	memset(&astats, 0, sizeof(astats));
> +	/* Try to migrate in batch with MIGRATE_ASYNC mode firstly */
> +	rc = migrate_pages_batch(from, get_new_page, put_new_page, private, MIGRATE_ASYNC,
> +				 reason, &folios, split_folios, &astats,
> +				 NR_MAX_MIGRATE_PAGES_RETRY);

I wonder if that and below would better be NR_MAX_MIGRATE_PAGES_RETRY / 2.

Though I've never got down to adjusting that number (and it's not a job
to be done in this set of patches), those 10 retries sometimes terrify
me, from a latency point of view.  They can have such different weights:
in the unmapped case, 10 retries is okay; but when a pinned page is mapped
into 1000 processes, the thought of all that unmapping and TLB flushing
and remapping is terrifying.

Since you're retrying below, halve both numbers of retries for now?

> +	stats->nr_succeeded += astats.nr_succeeded;
> +	stats->nr_thp_succeeded += astats.nr_thp_succeeded;
> +	stats->nr_thp_split += astats.nr_thp_split;
> +	if (rc < 0) {
> +		stats->nr_failed_pages += astats.nr_failed_pages;
> +		stats->nr_thp_failed += astats.nr_thp_failed;
> +		list_splice_tail(&folios, ret_folios);
> +		return rc;
> +	}
> +	stats->nr_thp_failed += astats.nr_thp_split;
> +	nr_failed += astats.nr_thp_split;
> +	/*
> +	 * Fall back to migrate all failed folios one by one synchronously. All
> +	 * failed folios except split THPs will be retried, so their failure
> +	 * isn't counted
> +	 */
> +	list_splice_tail_init(&folios, from);
> +	while (!list_empty(from)) {
> +		list_move(from->next, &folios);
> +		rc = migrate_pages_batch(&folios, get_new_page, put_new_page,
> +					 private, mode, reason, ret_folios,
> +					 split_folios, stats, NR_MAX_MIGRATE_PAGES_RETRY);

NR_MAX_MIGRATE_PAGES_RETRY / 2 ?

> +		list_splice_tail_init(&folios, ret_folios);
> +		if (rc < 0)
> +			return rc;
> +		nr_failed += rc;
> +	}
> +
> +	return nr_failed;
> +}
> +
>  /*
>   * migrate_pages - migrate the folios specified in a list, to the free folios
>   *		   supplied as the target for the page migration
> @@ -1874,7 +1919,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  		enum migrate_mode mode, int reason, unsigned int *ret_succeeded)
>  {
>  	int rc, rc_gather;
> -	int nr_pages, batch;
> +	int nr_pages;
>  	struct folio *folio, *folio2;
>  	LIST_HEAD(folios);
>  	LIST_HEAD(ret_folios);
> @@ -1890,10 +1935,6 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  	if (rc_gather < 0)
>  		goto out;
>  
> -	if (mode == MIGRATE_ASYNC)
> -		batch = NR_MAX_BATCHED_MIGRATION;
> -	else
> -		batch = 1;
>  again:
>  	nr_pages = 0;
>  	list_for_each_entry_safe(folio, folio2, from, lru) {
> @@ -1904,16 +1945,20 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  		}
>  
>  		nr_pages += folio_nr_pages(folio);
> -		if (nr_pages >= batch)
> +		if (nr_pages >= NR_MAX_BATCHED_MIGRATION)
>  			break;
>  	}
> -	if (nr_pages >= batch)
> +	if (nr_pages >= NR_MAX_BATCHED_MIGRATION)
>  		list_cut_before(&folios, from, &folio2->lru);
>  	else
>  		list_splice_init(from, &folios);
> -	rc = migrate_pages_batch(&folios, get_new_page, put_new_page, private,
> -				 mode, reason, &ret_folios, &split_folios, &stats,
> -				 NR_MAX_MIGRATE_PAGES_RETRY);
> +	if (mode == MIGRATE_ASYNC)
> +		rc = migrate_pages_batch(&folios, get_new_page, put_new_page, private,
> +					 mode, reason, &ret_folios, &split_folios, &stats,
> +					 NR_MAX_MIGRATE_PAGES_RETRY);
> +	else
> +		rc = migrate_pages_sync(&folios, get_new_page, put_new_page, private,
> +					mode, reason, &ret_folios, &split_folios, &stats);
>  	list_splice_tail_init(&folios, &ret_folios);
>  	if (rc < 0) {
>  		rc_gather = rc;
> -- 
> 2.39.1
  

Hugh Dickins <hughd@google.com> writes:

> On Fri, 24 Feb 2023, Huang Ying wrote:
>
>> When we have locked more than one folios, we cannot wait the lock or
>> bit (e.g., page lock, buffer head lock, writeback bit) synchronously.
>> Otherwise deadlock may be triggered.  This make it hard to batch the
>> synchronous migration directly.
>> 
>> This patch re-enables batching synchronous migration via trying to
>> migrate in batch asynchronously firstly.  And any folios that are
>> failed to be migrated asynchronously will be migrated synchronously
>> one by one.
>> 
>> Test shows that this can restore the TLB flushing batching performance
>> for synchronous migration effectively.
>> 
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Cc: Hugh Dickins <hughd@google.com>
>
> I'm not sure whether my 48 hours on two machines counts for a
> Tested-by: Hugh Dickins <hughd@google.com>
> or not; but it certainly looks like you've fixed my deadlock.

Thank you very much for testing the series!

>> Cc: "Xu, Pengfei" <pengfei.xu@intel.com>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: Stefan Roesch <shr@devkernel.io>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: Xin Hao <xhao@linux.alibaba.com>
>> Cc: Zi Yan <ziy@nvidia.com>
>> Cc: Yang Shi <shy828301@gmail.com>
>> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
>> Cc: Matthew Wilcox <willy@infradead.org>
>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>  mm/migrate.c | 65 ++++++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 55 insertions(+), 10 deletions(-)
>
> I was initially disappointed, that this was more complicated than I had
> thought it should be; but came to understand why.  My "change the mode
> to MIGRATE_ASYNC after the first" model would have condemned most of the
> MIGRATE_SYNC batch of pages to be handled as lightly as MIGRATE_ASYNC:
> not good enough, you're right be trying harder here.
>
>> 
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 91198b487e49..c17ce5ee8d92 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1843,6 +1843,51 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>>  	return rc;
>>  }
>>  
>> +static int migrate_pages_sync(struct list_head *from, new_page_t get_new_page,
>> +		free_page_t put_new_page, unsigned long private,
>> +		enum migrate_mode mode, int reason, struct list_head *ret_folios,
>> +		struct list_head *split_folios, struct migrate_pages_stats *stats)
>> +{
>> +	int rc, nr_failed = 0;
>> +	LIST_HEAD(folios);
>> +	struct migrate_pages_stats astats;
>> +
>> +	memset(&astats, 0, sizeof(astats));
>> +	/* Try to migrate in batch with MIGRATE_ASYNC mode firstly */
>> +	rc = migrate_pages_batch(from, get_new_page, put_new_page, private, MIGRATE_ASYNC,
>> +				 reason, &folios, split_folios, &astats,
>> +				 NR_MAX_MIGRATE_PAGES_RETRY);
>
> I wonder if that and below would better be NR_MAX_MIGRATE_PAGES_RETRY / 2.
>
> Though I've never got down to adjusting that number (and it's not a job
> to be done in this set of patches), those 10 retries sometimes terrify
> me, from a latency point of view.  They can have such different weights:
> in the unmapped case, 10 retries is okay; but when a pinned page is mapped
> into 1000 processes, the thought of all that unmapping and TLB flushing
> and remapping is terrifying.
>
> Since you're retrying below, halve both numbers of retries for now?

Yes.  These are reasonable concerns.

And in the original implementation, we only wait to lock page and wait
the writeback to complete if pass > 2.  This is kind of trying to
migrate asynchronously for 3 times before the real synchronous
migration.  So, should we delete the "force" logic (in
migrate_folio_unmap()), and try to migrate asynchronously for 3 times in
batch before migrating synchronously for 7 times one by one?

>> +	stats->nr_succeeded += astats.nr_succeeded;
>> +	stats->nr_thp_succeeded += astats.nr_thp_succeeded;
>> +	stats->nr_thp_split += astats.nr_thp_split;
>> +	if (rc < 0) {
>> +		stats->nr_failed_pages += astats.nr_failed_pages;
>> +		stats->nr_thp_failed += astats.nr_thp_failed;
>> +		list_splice_tail(&folios, ret_folios);
>> +		return rc;
>> +	}
>> +	stats->nr_thp_failed += astats.nr_thp_split;
>> +	nr_failed += astats.nr_thp_split;
>> +	/*
>> +	 * Fall back to migrate all failed folios one by one synchronously. All
>> +	 * failed folios except split THPs will be retried, so their failure
>> +	 * isn't counted
>> +	 */
>> +	list_splice_tail_init(&folios, from);
>> +	while (!list_empty(from)) {
>> +		list_move(from->next, &folios);
>> +		rc = migrate_pages_batch(&folios, get_new_page, put_new_page,
>> +					 private, mode, reason, ret_folios,
>> +					 split_folios, stats, NR_MAX_MIGRATE_PAGES_RETRY);
>
> NR_MAX_MIGRATE_PAGES_RETRY / 2 ?
>
>> +		list_splice_tail_init(&folios, ret_folios);
>> +		if (rc < 0)
>> +			return rc;
>> +		nr_failed += rc;
>> +	}
>> +
>> +	return nr_failed;
>> +}
>> +
>>  /*
>>   * migrate_pages - migrate the folios specified in a list, to the free folios
>>   *		   supplied as the target for the page migration
>> @@ -1874,7 +1919,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>  		enum migrate_mode mode, int reason, unsigned int *ret_succeeded)
>>  {
>>  	int rc, rc_gather;
>> -	int nr_pages, batch;
>> +	int nr_pages;
>>  	struct folio *folio, *folio2;
>>  	LIST_HEAD(folios);
>>  	LIST_HEAD(ret_folios);
>> @@ -1890,10 +1935,6 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>  	if (rc_gather < 0)
>>  		goto out;
>>  
>> -	if (mode == MIGRATE_ASYNC)
>> -		batch = NR_MAX_BATCHED_MIGRATION;
>> -	else
>> -		batch = 1;
>>  again:
>>  	nr_pages = 0;
>>  	list_for_each_entry_safe(folio, folio2, from, lru) {
>> @@ -1904,16 +1945,20 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>  		}
>>  
>>  		nr_pages += folio_nr_pages(folio);
>> -		if (nr_pages >= batch)
>> +		if (nr_pages >= NR_MAX_BATCHED_MIGRATION)
>>  			break;
>>  	}
>> -	if (nr_pages >= batch)
>> +	if (nr_pages >= NR_MAX_BATCHED_MIGRATION)
>>  		list_cut_before(&folios, from, &folio2->lru);
>>  	else
>>  		list_splice_init(from, &folios);
>> -	rc = migrate_pages_batch(&folios, get_new_page, put_new_page, private,
>> -				 mode, reason, &ret_folios, &split_folios, &stats,
>> -				 NR_MAX_MIGRATE_PAGES_RETRY);
>> +	if (mode == MIGRATE_ASYNC)
>> +		rc = migrate_pages_batch(&folios, get_new_page, put_new_page, private,
>> +					 mode, reason, &ret_folios, &split_folios, &stats,
>> +					 NR_MAX_MIGRATE_PAGES_RETRY);
>> +	else
>> +		rc = migrate_pages_sync(&folios, get_new_page, put_new_page, private,
>> +					mode, reason, &ret_folios, &split_folios, &stats);
>>  	list_splice_tail_init(&folios, &ret_folios);
>>  	if (rc < 0) {
>>  		rc_gather = rc;
>> -- 
>> 2.39.1

Best Regards,
Huang, Ying
  

On Tue, 28 Feb 2023, Huang, Ying wrote:
> Hugh Dickins <hughd@google.com> writes:
> > On Fri, 24 Feb 2023, Huang Ying wrote:
> >> 
> >> diff --git a/mm/migrate.c b/mm/migrate.c
> >> index 91198b487e49..c17ce5ee8d92 100644
> >> --- a/mm/migrate.c
> >> +++ b/mm/migrate.c
> >> @@ -1843,6 +1843,51 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
> >>  	return rc;
> >>  }
> >>  
> >> +static int migrate_pages_sync(struct list_head *from, new_page_t get_new_page,
> >> +		free_page_t put_new_page, unsigned long private,
> >> +		enum migrate_mode mode, int reason, struct list_head *ret_folios,
> >> +		struct list_head *split_folios, struct migrate_pages_stats *stats)
> >> +{
> >> +	int rc, nr_failed = 0;
> >> +	LIST_HEAD(folios);
> >> +	struct migrate_pages_stats astats;
> >> +
> >> +	memset(&astats, 0, sizeof(astats));
> >> +	/* Try to migrate in batch with MIGRATE_ASYNC mode firstly */
> >> +	rc = migrate_pages_batch(from, get_new_page, put_new_page, private, MIGRATE_ASYNC,
> >> +				 reason, &folios, split_folios, &astats,
> >> +				 NR_MAX_MIGRATE_PAGES_RETRY);
> >
> > I wonder if that and below would better be NR_MAX_MIGRATE_PAGES_RETRY / 2.
> >
> > Though I've never got down to adjusting that number (and it's not a job
> > to be done in this set of patches), those 10 retries sometimes terrify
> > me, from a latency point of view.  They can have such different weights:
> > in the unmapped case, 10 retries is okay; but when a pinned page is mapped
> > into 1000 processes, the thought of all that unmapping and TLB flushing
> > and remapping is terrifying.
> >
> > Since you're retrying below, halve both numbers of retries for now?
> 
> Yes.  These are reasonable concerns.
> 
> And in the original implementation, we only wait to lock page and wait
> the writeback to complete if pass > 2.  This is kind of trying to
> migrate asynchronously for 3 times before the real synchronous
> migration.  So, should we delete the "force" logic (in
> migrate_folio_unmap()), and try to migrate asynchronously for 3 times in
> batch before migrating synchronously for 7 times one by one?

Oh, that's a good idea (but please don't imagine I've thought it through):
I hadn't realized the way in which your migrate_pages_sync() addition is
kind of duplicating the way that the "force" argument conditions behaviour,
It would be very appealing to delete the "force" argument now if you can.

But aside from that, you've also made me wonder (again, please remember I
don't have a good picture of the new migrate_pages() sequence in my head)
whether you have already made a *great* strike against my 10 retries
terror.  Am I reading it right, that the unmapping is now done on the
first try, and the remove_migration_ptes after the last try (all the
pages involved having remained locked throughout)?

Hugh
  

On 2/24/2023 10:11 PM, Huang Ying wrote:
> When we have locked more than one folios, we cannot wait the lock or
> bit (e.g., page lock, buffer head lock, writeback bit) synchronously.
> Otherwise deadlock may be triggered.  This make it hard to batch the
> synchronous migration directly.
> 
> This patch re-enables batching synchronous migration via trying to
> migrate in batch asynchronously firstly.  And any folios that are
> failed to be migrated asynchronously will be migrated synchronously
> one by one.
> 
> Test shows that this can restore the TLB flushing batching performance
> for synchronous migration effectively.
> 
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: "Xu, Pengfei" <pengfei.xu@intel.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Stefan Roesch <shr@devkernel.io>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Xin Hao <xhao@linux.alibaba.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Yang Shi <shy828301@gmail.com>
> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Mike Kravetz <mike.kravetz@oracle.com>
> ---
>   mm/migrate.c | 65 ++++++++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 91198b487e49..c17ce5ee8d92 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1843,6 +1843,51 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>   	return rc;
>   }
>   
> +static int migrate_pages_sync(struct list_head *from, new_page_t get_new_page,
> +		free_page_t put_new_page, unsigned long private,
> +		enum migrate_mode mode, int reason, struct list_head *ret_folios,
> +		struct list_head *split_folios, struct migrate_pages_stats *stats)
> +{
> +	int rc, nr_failed = 0;
> +	LIST_HEAD(folios);
> +	struct migrate_pages_stats astats;
> +
> +	memset(&astats, 0, sizeof(astats));
> +	/* Try to migrate in batch with MIGRATE_ASYNC mode firstly */
> +	rc = migrate_pages_batch(from, get_new_page, put_new_page, private, MIGRATE_ASYNC,
> +				 reason, &folios, split_folios, &astats,
> +				 NR_MAX_MIGRATE_PAGES_RETRY);
> +	stats->nr_succeeded += astats.nr_succeeded;
> +	stats->nr_thp_succeeded += astats.nr_thp_succeeded;
> +	stats->nr_thp_split += astats.nr_thp_split;
> +	if (rc < 0) {
> +		stats->nr_failed_pages += astats.nr_failed_pages;
> +		stats->nr_thp_failed += astats.nr_thp_failed;
> +		list_splice_tail(&folios, ret_folios);
> +		return rc;
> +	}
> +	stats->nr_thp_failed += astats.nr_thp_split;
> +	nr_failed += astats.nr_thp_split;
> +	/*
> +	 * Fall back to migrate all failed folios one by one synchronously. All
> +	 * failed folios except split THPs will be retried, so their failure
> +	 * isn't counted
> +	 */
> +	list_splice_tail_init(&folios, from);
> +	while (!list_empty(from)) {
> +		list_move(from->next, &folios);
> +		rc = migrate_pages_batch(&folios, get_new_page, put_new_page,
> +					 private, mode, reason, ret_folios,
> +					 split_folios, stats, NR_MAX_MIGRATE_PAGES_RETRY);
> +		list_splice_tail_init(&folios, ret_folios);
> +		if (rc < 0)
> +			return rc;
> +		nr_failed += rc;
> +	}
> +
> +	return nr_failed;
> +}
> +
>   /*
>    * migrate_pages - migrate the folios specified in a list, to the free folios
>    *		   supplied as the target for the page migration
> @@ -1874,7 +1919,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>   		enum migrate_mode mode, int reason, unsigned int *ret_succeeded)
>   {
>   	int rc, rc_gather;
> -	int nr_pages, batch;
> +	int nr_pages;
>   	struct folio *folio, *folio2;
>   	LIST_HEAD(folios);
>   	LIST_HEAD(ret_folios);
> @@ -1890,10 +1935,6 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>   	if (rc_gather < 0)
>   		goto out;
>   
> -	if (mode == MIGRATE_ASYNC)
> -		batch = NR_MAX_BATCHED_MIGRATION;
> -	else
> -		batch = 1;
>   again:
>   	nr_pages = 0;
>   	list_for_each_entry_safe(folio, folio2, from, lru) {
> @@ -1904,16 +1945,20 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>   		}
>   
>   		nr_pages += folio_nr_pages(folio);
> -		if (nr_pages >= batch)
> +		if (nr_pages >= NR_MAX_BATCHED_MIGRATION)
>   			break;
>   	}
> -	if (nr_pages >= batch)
> +	if (nr_pages >= NR_MAX_BATCHED_MIGRATION)
>   		list_cut_before(&folios, from, &folio2->lru);
>   	else
>   		list_splice_init(from, &folios);
> -	rc = migrate_pages_batch(&folios, get_new_page, put_new_page, private,
> -				 mode, reason, &ret_folios, &split_folios, &stats,
> -				 NR_MAX_MIGRATE_PAGES_RETRY);
> +	if (mode == MIGRATE_ASYNC)
> +		rc = migrate_pages_batch(&folios, get_new_page, put_new_page, private,
> +					 mode, reason, &ret_folios, &split_folios, &stats,
> +					 NR_MAX_MIGRATE_PAGES_RETRY);
> +	else
> +		rc = migrate_pages_sync(&folios, get_new_page, put_new_page, private,
> +					mode, reason, &ret_folios, &split_folios, &stats);

For split folios, it seems also reasonable to use migrate_pages_sync() 
instead of always using fixed MIGRATE_ASYNC mode?

>   	list_splice_tail_init(&folios, &ret_folios);
>   	if (rc < 0) {
>   		rc_gather = rc;
  

Hugh Dickins <hughd@google.com> writes:

> On Tue, 28 Feb 2023, Huang, Ying wrote:
>> Hugh Dickins <hughd@google.com> writes:
>> > On Fri, 24 Feb 2023, Huang Ying wrote:
>> >> 
>> >> diff --git a/mm/migrate.c b/mm/migrate.c
>> >> index 91198b487e49..c17ce5ee8d92 100644
>> >> --- a/mm/migrate.c
>> >> +++ b/mm/migrate.c
>> >> @@ -1843,6 +1843,51 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>> >>  	return rc;
>> >>  }
>> >>  
>> >> +static int migrate_pages_sync(struct list_head *from, new_page_t get_new_page,
>> >> +		free_page_t put_new_page, unsigned long private,
>> >> +		enum migrate_mode mode, int reason, struct list_head *ret_folios,
>> >> +		struct list_head *split_folios, struct migrate_pages_stats *stats)
>> >> +{
>> >> +	int rc, nr_failed = 0;
>> >> +	LIST_HEAD(folios);
>> >> +	struct migrate_pages_stats astats;
>> >> +
>> >> +	memset(&astats, 0, sizeof(astats));
>> >> +	/* Try to migrate in batch with MIGRATE_ASYNC mode firstly */
>> >> +	rc = migrate_pages_batch(from, get_new_page, put_new_page, private, MIGRATE_ASYNC,
>> >> +				 reason, &folios, split_folios, &astats,
>> >> +				 NR_MAX_MIGRATE_PAGES_RETRY);
>> >
>> > I wonder if that and below would better be NR_MAX_MIGRATE_PAGES_RETRY / 2.
>> >
>> > Though I've never got down to adjusting that number (and it's not a job
>> > to be done in this set of patches), those 10 retries sometimes terrify
>> > me, from a latency point of view.  They can have such different weights:
>> > in the unmapped case, 10 retries is okay; but when a pinned page is mapped
>> > into 1000 processes, the thought of all that unmapping and TLB flushing
>> > and remapping is terrifying.
>> >
>> > Since you're retrying below, halve both numbers of retries for now?
>> 
>> Yes.  These are reasonable concerns.
>> 
>> And in the original implementation, we only wait to lock page and wait
>> the writeback to complete if pass > 2.  This is kind of trying to
>> migrate asynchronously for 3 times before the real synchronous
>> migration.  So, should we delete the "force" logic (in
>> migrate_folio_unmap()), and try to migrate asynchronously for 3 times in
>> batch before migrating synchronously for 7 times one by one?
>
> Oh, that's a good idea (but please don't imagine I've thought it through):
> I hadn't realized the way in which your migrate_pages_sync() addition is
> kind of duplicating the way that the "force" argument conditions behaviour,
> It would be very appealing to delete the "force" argument now if you can.

Sure.  Will do that in the next version.

> But aside from that, you've also made me wonder (again, please remember I
> don't have a good picture of the new migrate_pages() sequence in my head)
> whether you have already made a *great* strike against my 10 retries
> terror.  Am I reading it right, that the unmapping is now done on the
> first try, and the remove_migration_ptes after the last try (all the
> pages involved having remained locked throughout)?

Yes.  You are right.  Now, unmapping and moving are two separate steps,
and they are retried separately.  After a folio has been unmapped
successfully, we will not remap/unmap it 10 times if the folio is pinned
so that failed to move (migrate_folio_move()).  So the latency caused by
retrying is much better now.  But I still tend to keep the total retry
number as before.  Do you agree?

Best Regards,
Huang, Ying
  

Baolin Wang <baolin.wang@linux.alibaba.com> writes:

> On 2/24/2023 10:11 PM, Huang Ying wrote:
>> When we have locked more than one folios, we cannot wait the lock or
>> bit (e.g., page lock, buffer head lock, writeback bit) synchronously.
>> Otherwise deadlock may be triggered.  This make it hard to batch the
>> synchronous migration directly.
>> This patch re-enables batching synchronous migration via trying to
>> migrate in batch asynchronously firstly.  And any folios that are
>> failed to be migrated asynchronously will be migrated synchronously
>> one by one.
>> Test shows that this can restore the TLB flushing batching
>> performance
>> for synchronous migration effectively.
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: "Xu, Pengfei" <pengfei.xu@intel.com>
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: Stefan Roesch <shr@devkernel.io>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: Xin Hao <xhao@linux.alibaba.com>
>> Cc: Zi Yan <ziy@nvidia.com>
>> Cc: Yang Shi <shy828301@gmail.com>
>> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
>> Cc: Matthew Wilcox <willy@infradead.org>
>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>> ---
>>   mm/migrate.c | 65 ++++++++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 55 insertions(+), 10 deletions(-)
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 91198b487e49..c17ce5ee8d92 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1843,6 +1843,51 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>>   	return rc;
>>   }
>>   +static int migrate_pages_sync(struct list_head *from, new_page_t
>> get_new_page,
>> +		free_page_t put_new_page, unsigned long private,
>> +		enum migrate_mode mode, int reason, struct list_head *ret_folios,
>> +		struct list_head *split_folios, struct migrate_pages_stats *stats)
>> +{
>> +	int rc, nr_failed = 0;
>> +	LIST_HEAD(folios);
>> +	struct migrate_pages_stats astats;
>> +
>> +	memset(&astats, 0, sizeof(astats));
>> +	/* Try to migrate in batch with MIGRATE_ASYNC mode firstly */
>> +	rc = migrate_pages_batch(from, get_new_page, put_new_page, private, MIGRATE_ASYNC,
>> +				 reason, &folios, split_folios, &astats,
>> +				 NR_MAX_MIGRATE_PAGES_RETRY);
>> +	stats->nr_succeeded += astats.nr_succeeded;
>> +	stats->nr_thp_succeeded += astats.nr_thp_succeeded;
>> +	stats->nr_thp_split += astats.nr_thp_split;
>> +	if (rc < 0) {
>> +		stats->nr_failed_pages += astats.nr_failed_pages;
>> +		stats->nr_thp_failed += astats.nr_thp_failed;
>> +		list_splice_tail(&folios, ret_folios);
>> +		return rc;
>> +	}
>> +	stats->nr_thp_failed += astats.nr_thp_split;
>> +	nr_failed += astats.nr_thp_split;
>> +	/*
>> +	 * Fall back to migrate all failed folios one by one synchronously. All
>> +	 * failed folios except split THPs will be retried, so their failure
>> +	 * isn't counted
>> +	 */
>> +	list_splice_tail_init(&folios, from);
>> +	while (!list_empty(from)) {
>> +		list_move(from->next, &folios);
>> +		rc = migrate_pages_batch(&folios, get_new_page, put_new_page,
>> +					 private, mode, reason, ret_folios,
>> +					 split_folios, stats, NR_MAX_MIGRATE_PAGES_RETRY);
>> +		list_splice_tail_init(&folios, ret_folios);
>> +		if (rc < 0)
>> +			return rc;
>> +		nr_failed += rc;
>> +	}
>> +
>> +	return nr_failed;
>> +}
>> +
>>   /*
>>    * migrate_pages - migrate the folios specified in a list, to the free folios
>>    *		   supplied as the target for the page migration
>> @@ -1874,7 +1919,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   		enum migrate_mode mode, int reason, unsigned int *ret_succeeded)
>>   {
>>   	int rc, rc_gather;
>> -	int nr_pages, batch;
>> +	int nr_pages;
>>   	struct folio *folio, *folio2;
>>   	LIST_HEAD(folios);
>>   	LIST_HEAD(ret_folios);
>> @@ -1890,10 +1935,6 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   	if (rc_gather < 0)
>>   		goto out;
>>   -	if (mode == MIGRATE_ASYNC)
>> -		batch = NR_MAX_BATCHED_MIGRATION;
>> -	else
>> -		batch = 1;
>>   again:
>>   	nr_pages = 0;
>>   	list_for_each_entry_safe(folio, folio2, from, lru) {
>> @@ -1904,16 +1945,20 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   		}
>>     		nr_pages += folio_nr_pages(folio);
>> -		if (nr_pages >= batch)
>> +		if (nr_pages >= NR_MAX_BATCHED_MIGRATION)
>>   			break;
>>   	}
>> -	if (nr_pages >= batch)
>> +	if (nr_pages >= NR_MAX_BATCHED_MIGRATION)
>>   		list_cut_before(&folios, from, &folio2->lru);
>>   	else
>>   		list_splice_init(from, &folios);
>> -	rc = migrate_pages_batch(&folios, get_new_page, put_new_page, private,
>> -				 mode, reason, &ret_folios, &split_folios, &stats,
>> -				 NR_MAX_MIGRATE_PAGES_RETRY);
>> +	if (mode == MIGRATE_ASYNC)
>> +		rc = migrate_pages_batch(&folios, get_new_page, put_new_page, private,
>> +					 mode, reason, &ret_folios, &split_folios, &stats,
>> +					 NR_MAX_MIGRATE_PAGES_RETRY);
>> +	else
>> +		rc = migrate_pages_sync(&folios, get_new_page, put_new_page, private,
>> +					mode, reason, &ret_folios, &split_folios, &stats);
>
> For split folios, it seems also reasonable to use migrate_pages_sync()
> instead of always using fixed MIGRATE_ASYNC mode?

For split folios, we only try to migrate them with minimal effort.
Previously, we decrease the retry number from 10 to 1.  Now, I think
that it's reasonable to change the migration mode to MIGRATE_ASYNC to
reduce latency.  They have been counted as failure anyway.

>>   	list_splice_tail_init(&folios, &ret_folios);
>>   	if (rc < 0) {
>>   		rc_gather = rc;

Best Regards,
Huang, Ying
  

On Wed, 1 Mar 2023, Huang, Ying wrote:
> Hugh Dickins <hughd@google.com> writes:
> > On Tue, 28 Feb 2023, Huang, Ying wrote:
> >> Hugh Dickins <hughd@google.com> writes:
> >> > On Fri, 24 Feb 2023, Huang Ying wrote:
> >> >> 
> >> >> diff --git a/mm/migrate.c b/mm/migrate.c
> >> >> index 91198b487e49..c17ce5ee8d92 100644
> >> >> --- a/mm/migrate.c
> >> >> +++ b/mm/migrate.c
> >> >> @@ -1843,6 +1843,51 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
> >> >>  	return rc;
> >> >>  }
> >> >>  
> >> >> +static int migrate_pages_sync(struct list_head *from, new_page_t get_new_page,
> >> >> +		free_page_t put_new_page, unsigned long private,
> >> >> +		enum migrate_mode mode, int reason, struct list_head *ret_folios,
> >> >> +		struct list_head *split_folios, struct migrate_pages_stats *stats)
> >> >> +{
> >> >> +	int rc, nr_failed = 0;
> >> >> +	LIST_HEAD(folios);
> >> >> +	struct migrate_pages_stats astats;
> >> >> +
> >> >> +	memset(&astats, 0, sizeof(astats));
> >> >> +	/* Try to migrate in batch with MIGRATE_ASYNC mode firstly */
> >> >> +	rc = migrate_pages_batch(from, get_new_page, put_new_page, private, MIGRATE_ASYNC,
> >> >> +				 reason, &folios, split_folios, &astats,
> >> >> +				 NR_MAX_MIGRATE_PAGES_RETRY);
> >> >
> >> > I wonder if that and below would better be NR_MAX_MIGRATE_PAGES_RETRY / 2.
> >> >
> >> > Though I've never got down to adjusting that number (and it's not a job
> >> > to be done in this set of patches), those 10 retries sometimes terrify
> >> > me, from a latency point of view.  They can have such different weights:
> >> > in the unmapped case, 10 retries is okay; but when a pinned page is mapped
> >> > into 1000 processes, the thought of all that unmapping and TLB flushing
> >> > and remapping is terrifying.
> >> >
> >> > Since you're retrying below, halve both numbers of retries for now?
> >> 
> >> Yes.  These are reasonable concerns.
> >> 
> >> And in the original implementation, we only wait to lock page and wait
> >> the writeback to complete if pass > 2.  This is kind of trying to
> >> migrate asynchronously for 3 times before the real synchronous
> >> migration.  So, should we delete the "force" logic (in
> >> migrate_folio_unmap()), and try to migrate asynchronously for 3 times in
> >> batch before migrating synchronously for 7 times one by one?
> >
> > Oh, that's a good idea (but please don't imagine I've thought it through):
> > I hadn't realized the way in which your migrate_pages_sync() addition is
> > kind of duplicating the way that the "force" argument conditions behaviour,
> > It would be very appealing to delete the "force" argument now if you can.
> 
> Sure.  Will do that in the next version.
> 
> > But aside from that, you've also made me wonder (again, please remember I
> > don't have a good picture of the new migrate_pages() sequence in my head)
> > whether you have already made a *great* strike against my 10 retries
> > terror.  Am I reading it right, that the unmapping is now done on the
> > first try, and the remove_migration_ptes after the last try (all the
> > pages involved having remained locked throughout)?
> 
> Yes.  You are right.  Now, unmapping and moving are two separate steps,
> and they are retried separately.  After a folio has been unmapped
> successfully, we will not remap/unmap it 10 times if the folio is pinned
> so that failed to move (migrate_folio_move()).  So the latency caused by
> retrying is much better now.  But I still tend to keep the total retry
> number as before.  Do you agree?

Yes, I agree, keep the total retry number 10 as before: maybe someone in
future will show that more than 5 is a waste of time, but there's little
need to get into that now: if you've put an end to that 10 times unmapping
and remapping, that's a great step forward, quite apart from the TLB flush
batching itself.

(I did change "no need" to "little need" above: I do have some some
anxiety about the increased latencies from keeping folios locked and
migration entries in place for significantly longer than before your
batching: I won't be surprised if the maximum batch size has to be
lowered, if reports of latency spikes come in; and that might extend
to the retry count too.)

Hugh
  

Hugh Dickins <hughd@google.com> writes:

> On Wed, 1 Mar 2023, Huang, Ying wrote:
>> Hugh Dickins <hughd@google.com> writes:
>> > On Tue, 28 Feb 2023, Huang, Ying wrote:
>> >> Hugh Dickins <hughd@google.com> writes:
>> >> > On Fri, 24 Feb 2023, Huang Ying wrote:
>> >> >> 
>> >> >> diff --git a/mm/migrate.c b/mm/migrate.c
>> >> >> index 91198b487e49..c17ce5ee8d92 100644
>> >> >> --- a/mm/migrate.c
>> >> >> +++ b/mm/migrate.c
>> >> >> @@ -1843,6 +1843,51 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>> >> >>  	return rc;
>> >> >>  }
>> >> >>  
>> >> >> +static int migrate_pages_sync(struct list_head *from, new_page_t get_new_page,
>> >> >> +		free_page_t put_new_page, unsigned long private,
>> >> >> +		enum migrate_mode mode, int reason, struct list_head *ret_folios,
>> >> >> +		struct list_head *split_folios, struct migrate_pages_stats *stats)
>> >> >> +{
>> >> >> +	int rc, nr_failed = 0;
>> >> >> +	LIST_HEAD(folios);
>> >> >> +	struct migrate_pages_stats astats;
>> >> >> +
>> >> >> +	memset(&astats, 0, sizeof(astats));
>> >> >> +	/* Try to migrate in batch with MIGRATE_ASYNC mode firstly */
>> >> >> +	rc = migrate_pages_batch(from, get_new_page, put_new_page, private, MIGRATE_ASYNC,
>> >> >> +				 reason, &folios, split_folios, &astats,
>> >> >> +				 NR_MAX_MIGRATE_PAGES_RETRY);
>> >> >
>> >> > I wonder if that and below would better be NR_MAX_MIGRATE_PAGES_RETRY / 2.
>> >> >
>> >> > Though I've never got down to adjusting that number (and it's not a job
>> >> > to be done in this set of patches), those 10 retries sometimes terrify
>> >> > me, from a latency point of view.  They can have such different weights:
>> >> > in the unmapped case, 10 retries is okay; but when a pinned page is mapped
>> >> > into 1000 processes, the thought of all that unmapping and TLB flushing
>> >> > and remapping is terrifying.
>> >> >
>> >> > Since you're retrying below, halve both numbers of retries for now?
>> >> 
>> >> Yes.  These are reasonable concerns.
>> >> 
>> >> And in the original implementation, we only wait to lock page and wait
>> >> the writeback to complete if pass > 2.  This is kind of trying to
>> >> migrate asynchronously for 3 times before the real synchronous
>> >> migration.  So, should we delete the "force" logic (in
>> >> migrate_folio_unmap()), and try to migrate asynchronously for 3 times in
>> >> batch before migrating synchronously for 7 times one by one?
>> >
>> > Oh, that's a good idea (but please don't imagine I've thought it through):
>> > I hadn't realized the way in which your migrate_pages_sync() addition is
>> > kind of duplicating the way that the "force" argument conditions behaviour,
>> > It would be very appealing to delete the "force" argument now if you can.
>> 
>> Sure.  Will do that in the next version.
>> 
>> > But aside from that, you've also made me wonder (again, please remember I
>> > don't have a good picture of the new migrate_pages() sequence in my head)
>> > whether you have already made a *great* strike against my 10 retries
>> > terror.  Am I reading it right, that the unmapping is now done on the
>> > first try, and the remove_migration_ptes after the last try (all the
>> > pages involved having remained locked throughout)?
>> 
>> Yes.  You are right.  Now, unmapping and moving are two separate steps,
>> and they are retried separately.  After a folio has been unmapped
>> successfully, we will not remap/unmap it 10 times if the folio is pinned
>> so that failed to move (migrate_folio_move()).  So the latency caused by
>> retrying is much better now.  But I still tend to keep the total retry
>> number as before.  Do you agree?
>
> Yes, I agree, keep the total retry number 10 as before: maybe someone in
> future will show that more than 5 is a waste of time, but there's little
> need to get into that now: if you've put an end to that 10 times unmapping
> and remapping, that's a great step forward, quite apart from the TLB flush
> batching itself.
>
> (I did change "no need" to "little need" above: I do have some some
> anxiety about the increased latencies from keeping folios locked and
> migration entries in place for significantly longer than before your
> batching: I won't be surprised if the maximum batch size has to be
> lowered, if reports of latency spikes come in; and that might extend
> to the retry count too.)

Yes.  Latency are always concerns for batching.  We may revisit this
when needed.  Something good now is that we will never wait the lock or
bit in batched mode.  Latency tolerance depends on caller too, for
example, when we migrate some cold pages from DRAM to CXL MEM, we can
tolerate relatively long latency.  If so, we can add a parameter to
migrate_pages() to restrict the batch number and retry number when
necessary too.

Best Regards,
Huang, Ying
  

On 3/1/2023 2:18 PM, Huang, Ying wrote:
> Baolin Wang <baolin.wang@linux.alibaba.com> writes:
> 
>> On 2/24/2023 10:11 PM, Huang Ying wrote:
>>> When we have locked more than one folios, we cannot wait the lock or
>>> bit (e.g., page lock, buffer head lock, writeback bit) synchronously.
>>> Otherwise deadlock may be triggered.  This make it hard to batch the
>>> synchronous migration directly.
>>> This patch re-enables batching synchronous migration via trying to
>>> migrate in batch asynchronously firstly.  And any folios that are
>>> failed to be migrated asynchronously will be migrated synchronously
>>> one by one.
>>> Test shows that this can restore the TLB flushing batching
>>> performance
>>> for synchronous migration effectively.
>>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>>> Cc: Hugh Dickins <hughd@google.com>
>>> Cc: "Xu, Pengfei" <pengfei.xu@intel.com>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Cc: Stefan Roesch <shr@devkernel.io>
>>> Cc: Tejun Heo <tj@kernel.org>
>>> Cc: Xin Hao <xhao@linux.alibaba.com>
>>> Cc: Zi Yan <ziy@nvidia.com>
>>> Cc: Yang Shi <shy828301@gmail.com>
>>> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
>>> Cc: Matthew Wilcox <willy@infradead.org>
>>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>>> ---
>>>    mm/migrate.c | 65 ++++++++++++++++++++++++++++++++++++++++++++--------
>>>    1 file changed, 55 insertions(+), 10 deletions(-)
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index 91198b487e49..c17ce5ee8d92 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1843,6 +1843,51 @@ static int migrate_pages_batch(struct list_head *from, new_page_t get_new_page,
>>>    	return rc;
>>>    }
>>>    +static int migrate_pages_sync(struct list_head *from, new_page_t
>>> get_new_page,
>>> +		free_page_t put_new_page, unsigned long private,
>>> +		enum migrate_mode mode, int reason, struct list_head *ret_folios,
>>> +		struct list_head *split_folios, struct migrate_pages_stats *stats)
>>> +{
>>> +	int rc, nr_failed = 0;
>>> +	LIST_HEAD(folios);
>>> +	struct migrate_pages_stats astats;
>>> +
>>> +	memset(&astats, 0, sizeof(astats));
>>> +	/* Try to migrate in batch with MIGRATE_ASYNC mode firstly */
>>> +	rc = migrate_pages_batch(from, get_new_page, put_new_page, private, MIGRATE_ASYNC,
>>> +				 reason, &folios, split_folios, &astats,
>>> +				 NR_MAX_MIGRATE_PAGES_RETRY);
>>> +	stats->nr_succeeded += astats.nr_succeeded;
>>> +	stats->nr_thp_succeeded += astats.nr_thp_succeeded;
>>> +	stats->nr_thp_split += astats.nr_thp_split;
>>> +	if (rc < 0) {
>>> +		stats->nr_failed_pages += astats.nr_failed_pages;
>>> +		stats->nr_thp_failed += astats.nr_thp_failed;
>>> +		list_splice_tail(&folios, ret_folios);
>>> +		return rc;
>>> +	}
>>> +	stats->nr_thp_failed += astats.nr_thp_split;
>>> +	nr_failed += astats.nr_thp_split;
>>> +	/*
>>> +	 * Fall back to migrate all failed folios one by one synchronously. All
>>> +	 * failed folios except split THPs will be retried, so their failure
>>> +	 * isn't counted
>>> +	 */
>>> +	list_splice_tail_init(&folios, from);
>>> +	while (!list_empty(from)) {
>>> +		list_move(from->next, &folios);
>>> +		rc = migrate_pages_batch(&folios, get_new_page, put_new_page,
>>> +					 private, mode, reason, ret_folios,
>>> +					 split_folios, stats, NR_MAX_MIGRATE_PAGES_RETRY);
>>> +		list_splice_tail_init(&folios, ret_folios);
>>> +		if (rc < 0)
>>> +			return rc;
>>> +		nr_failed += rc;
>>> +	}
>>> +
>>> +	return nr_failed;
>>> +}
>>> +
>>>    /*
>>>     * migrate_pages - migrate the folios specified in a list, to the free folios
>>>     *		   supplied as the target for the page migration
>>> @@ -1874,7 +1919,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>    		enum migrate_mode mode, int reason, unsigned int *ret_succeeded)
>>>    {
>>>    	int rc, rc_gather;
>>> -	int nr_pages, batch;
>>> +	int nr_pages;
>>>    	struct folio *folio, *folio2;
>>>    	LIST_HEAD(folios);
>>>    	LIST_HEAD(ret_folios);
>>> @@ -1890,10 +1935,6 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>    	if (rc_gather < 0)
>>>    		goto out;
>>>    -	if (mode == MIGRATE_ASYNC)
>>> -		batch = NR_MAX_BATCHED_MIGRATION;
>>> -	else
>>> -		batch = 1;
>>>    again:
>>>    	nr_pages = 0;
>>>    	list_for_each_entry_safe(folio, folio2, from, lru) {
>>> @@ -1904,16 +1945,20 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>    		}
>>>      		nr_pages += folio_nr_pages(folio);
>>> -		if (nr_pages >= batch)
>>> +		if (nr_pages >= NR_MAX_BATCHED_MIGRATION)
>>>    			break;
>>>    	}
>>> -	if (nr_pages >= batch)
>>> +	if (nr_pages >= NR_MAX_BATCHED_MIGRATION)
>>>    		list_cut_before(&folios, from, &folio2->lru);
>>>    	else
>>>    		list_splice_init(from, &folios);
>>> -	rc = migrate_pages_batch(&folios, get_new_page, put_new_page, private,
>>> -				 mode, reason, &ret_folios, &split_folios, &stats,
>>> -				 NR_MAX_MIGRATE_PAGES_RETRY);
>>> +	if (mode == MIGRATE_ASYNC)
>>> +		rc = migrate_pages_batch(&folios, get_new_page, put_new_page, private,
>>> +					 mode, reason, &ret_folios, &split_folios, &stats,
>>> +					 NR_MAX_MIGRATE_PAGES_RETRY);
>>> +	else
>>> +		rc = migrate_pages_sync(&folios, get_new_page, put_new_page, private,
>>> +					mode, reason, &ret_folios, &split_folios, &stats);
>>
>> For split folios, it seems also reasonable to use migrate_pages_sync()
>> instead of always using fixed MIGRATE_ASYNC mode?
> 
> For split folios, we only try to migrate them with minimal effort.
> Previously, we decrease the retry number from 10 to 1.  Now, I think
> that it's reasonable to change the migration mode to MIGRATE_ASYNC to
> reduce latency.  They have been counted as failure anyway.

Sounds reasonable. Thanks for explanation. Please feel free to add:
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>