[RFC PATCH 0/3] Improve the migration stats

[RFC PATCH 0/3] Improve the migration stats

[Baolin Wang]

Hi,

According to talk with Zi Yan [1], this patch set changes the return
value of migrate_pages() to avoid returning a number which is larger
than the number of pages the users tried to migrate by move_pages() syscall.
Also fix the hugetlb migration stats and migration stats in
trace_mm_compaction_migratepages(). Please help to review. Thanks.

[1] https://lore.kernel.org/linux-mm/7E44019D-2A5D-4BA7-B4D5-00D4712F1687@nvidia.com/

Baolin Wang (3):
  mm: migrate: Fix the return value of migrate_pages()
  mm: migrate: Correct the hugetlb migration stats
  mm: compaction: Fix the migration stats in
    trace_mm_compaction_migratepages()

 include/trace/events/compaction.h | 24 ++++--------------------
 mm/compaction.c                   |  7 ++++---
 mm/migrate.c                      | 25 ++++++++++++++-----------
 3 files changed, 22 insertions(+), 34 deletions(-)

-- 
1.8.3.1

[RFC PATCH 1/3] mm: migrate: Fix the return value of migrate_pages()

[Baolin Wang]

As Zi Yan pointed out, the syscall move_pages() can return a non-migrated
number larger than the number of pages the users tried to migrate, when a
THP page is failed to migrate. This is confusing for users.

Since other migration scenarios do not care about the actual non-migrated
number of pages except the memory compaction migration which will fix in
following patch. Thus we can change the return value to return the number
of {normal page, THP, hugetlb} instead to avoid this issue, meanwhile we
should still keep the migration counters using the number of normal pages.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/migrate.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index a11e948..00b8922 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1428,7 +1428,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
  * @mode:		The migration mode that specifies the constraints for
  *			page migration, if any.
  * @reason:		The reason for page migration.
- * @ret_succeeded:	Set to the number of pages migrated successfully if
+ * @ret_succeeded:	Set to the number of normal pages migrated successfully if
  *			the caller passes a non-NULL pointer.
  *
  * The function returns after 10 attempts or if no pages are movable any more
@@ -1436,7 +1436,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
  * It is caller's responsibility to call putback_movable_pages() to return pages
  * to the LRU or free list only if ret != 0.
  *
- * Returns the number of pages that were not migrated, or an error code.
+ * Returns the number of {normal page, THP} that were not migrated, or an error code.
  */
 int migrate_pages(struct list_head *from, new_page_t get_new_page,
 		free_page_t put_new_page, unsigned long private,
@@ -1445,6 +1445,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 	int retry = 1;
 	int thp_retry = 1;
 	int nr_failed = 0;
+	int nr_failed_pages = 0;
 	int nr_succeeded = 0;
 	int nr_thp_succeeded = 0;
 	int nr_thp_failed = 0;
@@ -1517,7 +1518,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 					}
 
 					nr_thp_failed++;
-					nr_failed += nr_subpages;
+					nr_failed_pages += nr_subpages;
 					break;
 				}
 
@@ -1537,7 +1538,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 					}
 
 					nr_thp_failed++;
-					nr_failed += nr_subpages;
+					nr_failed_pages += nr_subpages;
 					goto out;
 				}
 				nr_failed++;
@@ -1566,7 +1567,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				 */
 				if (is_thp) {
 					nr_thp_failed++;
-					nr_failed += nr_subpages;
+					nr_failed_pages += nr_subpages;
 					break;
 				}
 				nr_failed++;
@@ -1575,8 +1576,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 		}
 	}
 	nr_failed += retry + thp_retry;
+	nr_failed_pages += nr_failed;
 	nr_thp_failed += thp_retry;
-	rc = nr_failed;
+	rc = nr_failed + nr_thp_failed;
 out:
 	/*
 	 * Put the permanent failure page back to migration list, they
@@ -1585,11 +1587,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 	list_splice(&ret_pages, from);
 
 	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
-	count_vm_events(PGMIGRATE_FAIL, nr_failed);
+	count_vm_events(PGMIGRATE_FAIL, nr_failed_pages);
 	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
 	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
 	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
-	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
+	trace_mm_migrate_pages(nr_succeeded, nr_failed_pages, nr_thp_succeeded,
 			       nr_thp_failed, nr_thp_split, mode, reason);
 
 	if (!swapwrite)
-- 
1.8.3.1

[RFC PATCH 2/3] mm: migrate: Correct the hugetlb migration stats

[Baolin Wang]

Correct the migration stats for hugetlb with using compound_nr() instead
of thp_nr_pages(), meanwhile change 'nr_failed_pages' to record the
number of normal pages failed to migrate, including THP and hugetlb,
and 'nr_succeeded' will record the number of normal pages migrated
successfully.

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 mm/migrate.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 00b8922..00e231c 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1436,7 +1436,8 @@ static inline int try_split_thp(struct page *page, struct page **page2,
  * It is caller's responsibility to call putback_movable_pages() to return pages
  * to the LRU or free list only if ret != 0.
  *
- * Returns the number of {normal page, THP} that were not migrated, or an error code.
+ * Returns the number of {normal page, THP, hugetlb} that were not migrated,
+ * or an error code.
  */
 int migrate_pages(struct list_head *from, new_page_t get_new_page,
 		free_page_t put_new_page, unsigned long private,
@@ -1476,7 +1477,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 			 * during migration.
 			 */
 			is_thp = PageTransHuge(page) && !PageHuge(page);
-			nr_subpages = thp_nr_pages(page);
+			nr_subpages = compound_nr(page);
 			cond_resched();
 
 			if (PageHuge(page))
@@ -1523,6 +1524,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				}
 
 				/* Hugetlb migration is unsupported */
+				nr_failed_pages++;
 				nr_failed++;
 				break;
 			case -ENOMEM:
@@ -1541,6 +1543,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 					nr_failed_pages += nr_subpages;
 					goto out;
 				}
+				nr_failed_pages += nr_subpages;
 				nr_failed++;
 				goto out;
 			case -EAGAIN:
@@ -1551,12 +1554,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				retry++;
 				break;
 			case MIGRATEPAGE_SUCCESS:
+				nr_succeeded += nr_subpages;
 				if (is_thp) {
 					nr_thp_succeeded++;
-					nr_succeeded += nr_subpages;
 					break;
 				}
-				nr_succeeded++;
 				break;
 			default:
 				/*
@@ -1565,9 +1567,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				 * removed from migration page list and not
 				 * retried in the next outer loop.
 				 */
+				nr_failed_pages += nr_subpages;
 				if (is_thp) {
 					nr_thp_failed++;
-					nr_failed_pages += nr_subpages;
 					break;
 				}
 				nr_failed++;
@@ -1576,7 +1578,6 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 		}
 	}
 	nr_failed += retry + thp_retry;
-	nr_failed_pages += nr_failed;
 	nr_thp_failed += thp_retry;
 	rc = nr_failed + nr_thp_failed;
 out:
-- 
1.8.3.1

[RFC PATCH 3/3] mm: compaction: Fix the migration stats in trace_mm_compaction_migratepages()

[Baolin Wang]

Now the migrate_pages() has changed to return the number of {normal page,
THP, hugetlb} instead, thus we should not use the return value to calculate
the number of pages migrated successfully. Instead we can just use the
'nr_succeeded' which indicates the number of normal pages migrated successfully
to calculate the non-migrated pages in trace_mm_compaction_migratepages().

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
---
 include/trace/events/compaction.h | 24 ++++--------------------
 mm/compaction.c                   |  7 ++++---
 2 files changed, 8 insertions(+), 23 deletions(-)

diff --git a/include/trace/events/compaction.h b/include/trace/events/compaction.h
index 54e5bf0..7d48e70 100644
--- a/include/trace/events/compaction.h
+++ b/include/trace/events/compaction.h
@@ -68,10 +68,9 @@
 TRACE_EVENT(mm_compaction_migratepages,
 
 	TP_PROTO(unsigned long nr_all,
-		int migrate_rc,
-		struct list_head *migratepages),
+		unsigned int nr_succeeded),
 
-	TP_ARGS(nr_all, migrate_rc, migratepages),
+	TP_ARGS(nr_all, nr_succeeded),
 
 	TP_STRUCT__entry(
 		__field(unsigned long, nr_migrated)
@@ -79,23 +78,8 @@
 	),
 
 	TP_fast_assign(
-		unsigned long nr_failed = 0;
-		struct list_head *page_lru;
-
-		/*
-		 * migrate_pages() returns either a non-negative number
-		 * with the number of pages that failed migration, or an
-		 * error code, in which case we need to count the remaining
-		 * pages manually
-		 */
-		if (migrate_rc >= 0)
-			nr_failed = migrate_rc;
-		else
-			list_for_each(page_lru, migratepages)
-				nr_failed++;
-
-		__entry->nr_migrated = nr_all - nr_failed;
-		__entry->nr_failed = nr_failed;
+		__entry->nr_migrated = nr_succeeded;
+		__entry->nr_failed = nr_all - nr_succeeded;
 	),
 
 	TP_printk("nr_migrated=%lu nr_failed=%lu",
diff --git a/mm/compaction.c b/mm/compaction.c
index 6e44609..b4e94cd 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2280,6 +2280,7 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
 	unsigned long last_migrated_pfn;
 	const bool sync = cc->mode != MIGRATE_ASYNC;
 	bool update_cached;
+	unsigned int nr_succeeded = 0;
 
 	/*
 	 * These counters track activities during zone compaction.  Initialize
@@ -2398,10 +2399,10 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
 
 		err = migrate_pages(&cc->migratepages, compaction_alloc,
 				compaction_free, (unsigned long)cc, cc->mode,
-				MR_COMPACTION, NULL);
+				MR_COMPACTION, &nr_succeeded);
 
-		trace_mm_compaction_migratepages(cc->nr_migratepages, err,
-							&cc->migratepages);
+		trace_mm_compaction_migratepages(cc->nr_migratepages,
+						 nr_succeeded);
 
 		/* All pages were either migrated or will be released */
 		cc->nr_migratepages = 0;
-- 
1.8.3.1

Re: [RFC PATCH 3/3] mm: compaction: Fix the migration stats in trace_mm_compaction_migratepages()

[Steven Rostedt]

On Wed,  3 Nov 2021 18:51:16 +0800
Baolin Wang <baolin.wang@linux.alibaba.com> wrote:

> @@ -2398,10 +2399,10 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
>  
>  		err = migrate_pages(&cc->migratepages, compaction_alloc,
>  				compaction_free, (unsigned long)cc, cc->mode,
> -				MR_COMPACTION, NULL);
> +				MR_COMPACTION, &nr_succeeded);
>  
> -		trace_mm_compaction_migratepages(cc->nr_migratepages, err,
> -							&cc->migratepages);
> +		trace_mm_compaction_migratepages(cc->nr_migratepages,
> +						 nr_succeeded);

Also, I'm surprised you don't just pass in 'cc' and do the dereferencing in
the trace event macro. Accessing the pointers from the trace event and not
dereferencing them to the tracepoint function moves the changes out of line
here and helps with I$.

You could improve some of the other tracepoints that dereference 'cc' as
well in that file.

But as for this change, I'm good with it.

For the tracing aspect:

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve


>  
>  		/* All pages were either migrated or will be released */
>  		cc->nr_migratepages = 0;

Re: [RFC PATCH 3/3] mm: compaction: Fix the migration stats in trace_mm_compaction_migratepages()

[Baolin Wang]

On 2021/11/3 21:42, Steven Rostedt wrote:
> On Wed,  3 Nov 2021 18:51:16 +0800
> Baolin Wang <baolin.wang@linux.alibaba.com> wrote:
> 
>> @@ -2398,10 +2399,10 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
>>   
>>   		err = migrate_pages(&cc->migratepages, compaction_alloc,
>>   				compaction_free, (unsigned long)cc, cc->mode,
>> -				MR_COMPACTION, NULL);
>> +				MR_COMPACTION, &nr_succeeded);
>>   
>> -		trace_mm_compaction_migratepages(cc->nr_migratepages, err,
>> -							&cc->migratepages);
>> +		trace_mm_compaction_migratepages(cc->nr_migratepages,
>> +						 nr_succeeded);
> 
> Also, I'm surprised you don't just pass in 'cc' and do the dereferencing in
> the trace event macro. Accessing the pointers from the trace event and not
> dereferencing them to the tracepoint function moves the changes out of line
> here and helps with I$.
> 
> You could improve some of the other tracepoints that dereference 'cc' as
> well in that file.

Sure. Will do in a separate patch.

> 
> But as for this change, I'm good with it.
> 
> For the tracing aspect:
> 
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 

Thanks.

Re: [RFC PATCH 1/3] mm: migrate: Fix the return value of migrate_pages()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 8840 bytes --]

On 3 Nov 2021, at 6:51, Baolin Wang wrote:

> As Zi Yan pointed out, the syscall move_pages() can return a non-migrated
> number larger than the number of pages the users tried to migrate, when a
> THP page is failed to migrate. This is confusing for users.
>
> Since other migration scenarios do not care about the actual non-migrated
> number of pages except the memory compaction migration which will fix in
> following patch. Thus we can change the return value to return the number
> of {normal page, THP, hugetlb} instead to avoid this issue, meanwhile we
> should still keep the migration counters using the number of normal pages.
>
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/migrate.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index a11e948..00b8922 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1428,7 +1428,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>   * @mode:		The migration mode that specifies the constraints for
>   *			page migration, if any.
>   * @reason:		The reason for page migration.
> - * @ret_succeeded:	Set to the number of pages migrated successfully if
> + * @ret_succeeded:	Set to the number of normal pages migrated successfully if
>   *			the caller passes a non-NULL pointer.
>   *
>   * The function returns after 10 attempts or if no pages are movable any more
> @@ -1436,7 +1436,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>   * It is caller's responsibility to call putback_movable_pages() to return pages
>   * to the LRU or free list only if ret != 0.
>   *
> - * Returns the number of pages that were not migrated, or an error code.
> + * Returns the number of {normal page, THP} that were not migrated, or an error code.
>   */
>  int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  		free_page_t put_new_page, unsigned long private,
> @@ -1445,6 +1445,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  	int retry = 1;
>  	int thp_retry = 1;
>  	int nr_failed = 0;
> +	int nr_failed_pages = 0;
>  	int nr_succeeded = 0;
>  	int nr_thp_succeeded = 0;
>  	int nr_thp_failed = 0;
> @@ -1517,7 +1518,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  					}
>
>  					nr_thp_failed++;
> -					nr_failed += nr_subpages;
> +					nr_failed_pages += nr_subpages;
>  					break;
>  				}
>
> @@ -1537,7 +1538,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  					}
>
>  					nr_thp_failed++;
> -					nr_failed += nr_subpages;
> +					nr_failed_pages += nr_subpages;
>  					goto out;
>  				}
>  				nr_failed++;
> @@ -1566,7 +1567,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  				 */
>  				if (is_thp) {
>  					nr_thp_failed++;
> -					nr_failed += nr_subpages;
> +					nr_failed_pages += nr_subpages;
>  					break;
>  				}
>  				nr_failed++;
> @@ -1575,8 +1576,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  		}
>  	}
>  	nr_failed += retry + thp_retry;

This line can probably go away, since we do not want to count retried pages.

> +	nr_failed_pages += nr_failed;
>  	nr_thp_failed += thp_retry;
> -	rc = nr_failed;
> +	rc = nr_failed + nr_thp_failed;
>  out:
>  	/*
>  	 * Put the permanent failure page back to migration list, they
> @@ -1585,11 +1587,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  	list_splice(&ret_pages, from);
>
>  	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
> -	count_vm_events(PGMIGRATE_FAIL, nr_failed);
> +	count_vm_events(PGMIGRATE_FAIL, nr_failed_pages);
>  	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
>  	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
>  	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
> -	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
> +	trace_mm_migrate_pages(nr_succeeded, nr_failed_pages, nr_thp_succeeded,
>  			       nr_thp_failed, nr_thp_split, mode, reason);
>
>  	if (!swapwrite)
> -- 
> 1.8.3.1

Thank you for the patch!

In general, this looks good to me. But like you said in other email, when a THP fails to
migrate and gets split, the number of nr_failed will still be inflated by the number of
failed subpage migrations. What I can think of is to split THPs to a separate list and
stop increasing nr_failed when the pages from the new list is under migration. Let me
know how it sounds to you.

An untested but compiled patch (please apply it before this one) looks like:

diff --git a/mm/migrate.c b/mm/migrate.c
index 1852d787e6ab..f7e424f8e647 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1457,13 +1457,16 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
        int swapwrite = current->flags & PF_SWAPWRITE;
        int rc, nr_subpages;
        LIST_HEAD(ret_pages);
+       LIST_HEAD(thp_split_pages);
        bool nosplit = (reason == MR_NUMA_MISPLACED);
+       bool no_failed_counting = false;

        trace_mm_migrate_pages_start(mode, reason);

        if (!swapwrite)
                current->flags |= PF_SWAPWRITE;

+thp_subpage_migration:
        for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
                retry = 0;
                thp_retry = 0;
@@ -1512,7 +1515,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
                        case -ENOSYS:
                                /* THP migration is unsupported */
                                if (is_thp) {
-                                       if (!try_split_thp(page, &page2, from)) {
+                                       if (!try_split_thp(page, &page2, &thp_split_pages)) {
                                                nr_thp_split++;
                                                goto retry;
                                        }
@@ -1523,7 +1526,8 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
                                }

                                /* Hugetlb migration is unsupported */
-                               nr_failed++;
+                               if (!no_failed_counting)
+                                       nr_failed++;
                                break;
                        case -ENOMEM:
                                /*
@@ -1532,7 +1536,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
                                 * THP NUMA faulting doesn't split THP to retry.
                                 */
                                if (is_thp && !nosplit) {
-                                       if (!try_split_thp(page, &page2, from)) {
+                                       if (!try_split_thp(page, &page2, &thp_split_pages)) {
                                                nr_thp_split++;
                                                goto retry;
                                        }
@@ -1541,7 +1545,8 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
                                        nr_failed += nr_subpages;
                                        goto out;
                                }
-                               nr_failed++;
+                               if (!no_failed_counting)
+                                       nr_failed++;
                                goto out;
                        case -EAGAIN:
                                if (is_thp) {
@@ -1570,13 +1575,24 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
                                        nr_failed += nr_subpages;
                                        break;
                                }
-                               nr_failed++;
+                               if (!no_failed_counting)
+                                       nr_failed++;
                                break;
                        }
                }
        }
-       nr_failed += retry + thp_retry;
+       /* get thp_retry before it can be reset in THP subpage migration. */
        nr_thp_failed += thp_retry;
+       /* try to migrate subpages of fail-to-migrate THPs, no nr_failed
+        * counting in this round, since all subpages of a THP is counted as
+        * 1 failure in the first round. */
+       if (!list_empty(&thp_split_pages)) {
+               list_splice(from, &thp_split_pages);
+               no_failed_counting = true;
+               goto thp_subpage_migration;
+       }
+
+       nr_failed += retry + thp_retry;
        rc = nr_failed;
 out:
        /*
--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [RFC PATCH 2/3] mm: migrate: Correct the hugetlb migration stats

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 3127 bytes --]

On 3 Nov 2021, at 6:51, Baolin Wang wrote:

> Correct the migration stats for hugetlb with using compound_nr() instead
> of thp_nr_pages(), meanwhile change 'nr_failed_pages' to record the
> number of normal pages failed to migrate, including THP and hugetlb,
> and 'nr_succeeded' will record the number of normal pages migrated
> successfully.
>
> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  mm/migrate.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 00b8922..00e231c 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1436,7 +1436,8 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>   * It is caller's responsibility to call putback_movable_pages() to return pages
>   * to the LRU or free list only if ret != 0.
>   *
> - * Returns the number of {normal page, THP} that were not migrated, or an error code.
> + * Returns the number of {normal page, THP, hugetlb} that were not migrated,
> + * or an error code.
>   */
>  int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  		free_page_t put_new_page, unsigned long private,
> @@ -1476,7 +1477,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  			 * during migration.
>  			 */
>  			is_thp = PageTransHuge(page) && !PageHuge(page);
> -			nr_subpages = thp_nr_pages(page);
> +			nr_subpages = compound_nr(page);
>  			cond_resched();
>
>  			if (PageHuge(page))
> @@ -1523,6 +1524,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  				}
>
>  				/* Hugetlb migration is unsupported */
> +				nr_failed_pages++;
>  				nr_failed++;
>  				break;
>  			case -ENOMEM:
> @@ -1541,6 +1543,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  					nr_failed_pages += nr_subpages;
>  					goto out;
>  				}
> +				nr_failed_pages += nr_subpages;
>  				nr_failed++;
>  				goto out;
>  			case -EAGAIN:
> @@ -1551,12 +1554,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  				retry++;
>  				break;
>  			case MIGRATEPAGE_SUCCESS:
> +				nr_succeeded += nr_subpages;
>  				if (is_thp) {
>  					nr_thp_succeeded++;
> -					nr_succeeded += nr_subpages;
>  					break;
>  				}
> -				nr_succeeded++;
>  				break;
>  			default:
>  				/*
> @@ -1565,9 +1567,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  				 * removed from migration page list and not
>  				 * retried in the next outer loop.
>  				 */
> +				nr_failed_pages += nr_subpages;
>  				if (is_thp) {
>  					nr_thp_failed++;
> -					nr_failed_pages += nr_subpages;
>  					break;
>  				}
>  				nr_failed++;
> @@ -1576,7 +1578,6 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  		}
>  	}
>  	nr_failed += retry + thp_retry;
> -	nr_failed_pages += nr_failed;
>  	nr_thp_failed += thp_retry;
>  	rc = nr_failed + nr_thp_failed;
>  out:
> -- 
> 1.8.3.1

LGTM. Reviewed-by: Zi Yan <ziy@nvidia.com>

--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [RFC PATCH 1/3] mm: migrate: Fix the return value of migrate_pages()

[Baolin Wang]

On 2021/11/4 23:33, Zi Yan wrote:
> On 3 Nov 2021, at 6:51, Baolin Wang wrote:
> 
>> As Zi Yan pointed out, the syscall move_pages() can return a non-migrated
>> number larger than the number of pages the users tried to migrate, when a
>> THP page is failed to migrate. This is confusing for users.
>>
>> Since other migration scenarios do not care about the actual non-migrated
>> number of pages except the memory compaction migration which will fix in
>> following patch. Thus we can change the return value to return the number
>> of {normal page, THP, hugetlb} instead to avoid this issue, meanwhile we
>> should still keep the migration counters using the number of normal pages.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>   mm/migrate.c | 18 ++++++++++--------
>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index a11e948..00b8922 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1428,7 +1428,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>>    * @mode:		The migration mode that specifies the constraints for
>>    *			page migration, if any.
>>    * @reason:		The reason for page migration.
>> - * @ret_succeeded:	Set to the number of pages migrated successfully if
>> + * @ret_succeeded:	Set to the number of normal pages migrated successfully if
>>    *			the caller passes a non-NULL pointer.
>>    *
>>    * The function returns after 10 attempts or if no pages are movable any more
>> @@ -1436,7 +1436,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>>    * It is caller's responsibility to call putback_movable_pages() to return pages
>>    * to the LRU or free list only if ret != 0.
>>    *
>> - * Returns the number of pages that were not migrated, or an error code.
>> + * Returns the number of {normal page, THP} that were not migrated, or an error code.
>>    */
>>   int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   		free_page_t put_new_page, unsigned long private,
>> @@ -1445,6 +1445,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   	int retry = 1;
>>   	int thp_retry = 1;
>>   	int nr_failed = 0;
>> +	int nr_failed_pages = 0;
>>   	int nr_succeeded = 0;
>>   	int nr_thp_succeeded = 0;
>>   	int nr_thp_failed = 0;
>> @@ -1517,7 +1518,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   					}
>>
>>   					nr_thp_failed++;
>> -					nr_failed += nr_subpages;
>> +					nr_failed_pages += nr_subpages;
>>   					break;
>>   				}
>>
>> @@ -1537,7 +1538,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   					}
>>
>>   					nr_thp_failed++;
>> -					nr_failed += nr_subpages;
>> +					nr_failed_pages += nr_subpages;
>>   					goto out;
>>   				}
>>   				nr_failed++;
>> @@ -1566,7 +1567,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   				 */
>>   				if (is_thp) {
>>   					nr_thp_failed++;
>> -					nr_failed += nr_subpages;
>> +					nr_failed_pages += nr_subpages;
>>   					break;
>>   				}
>>   				nr_failed++;
>> @@ -1575,8 +1576,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   		}
>>   	}
>>   	nr_failed += retry + thp_retry;
> 
> This line can probably go away, since we do not want to count retried pages.

OK.

> 
>> +	nr_failed_pages += nr_failed;
>>   	nr_thp_failed += thp_retry;
>> -	rc = nr_failed;
>> +	rc = nr_failed + nr_thp_failed;
>>   out:
>>   	/*
>>   	 * Put the permanent failure page back to migration list, they
>> @@ -1585,11 +1587,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>   	list_splice(&ret_pages, from);
>>
>>   	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
>> -	count_vm_events(PGMIGRATE_FAIL, nr_failed);
>> +	count_vm_events(PGMIGRATE_FAIL, nr_failed_pages);
>>   	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
>>   	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
>>   	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
>> -	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
>> +	trace_mm_migrate_pages(nr_succeeded, nr_failed_pages, nr_thp_succeeded,
>>   			       nr_thp_failed, nr_thp_split, mode, reason);
>>
>>   	if (!swapwrite)
>> -- 
>> 1.8.3.1
> 
> Thank you for the patch!
> 
> In general, this looks good to me. But like you said in other email, when a THP fails to
> migrate and gets split, the number of nr_failed will still be inflated by the number of
> failed subpage migrations. What I can think of is to split THPs to a separate list and
> stop increasing nr_failed when the pages from the new list is under migration. Let me
> know how it sounds to you.

Thanks for your patch, but I think it does not cover all the cases.

Firstly, what confuses me is that if we return 1 to users when failed to 
migrate 1 THP page, but actually we may have migrated some normal pages 
successfaully if the THP page is split. Anyway we can add some comments 
for migrate_pages() to explain it if this is acceptable.

Another concern about your patch is that, if the first round migration 
all pages are migrated successfaully (nr_failed = 0), but if failed to 
migrate the subpages of the THP in the second round, we will still 
return 0 to users, which is incorrect. Further more, if the subpages of 
the THP are migrated partially in the second round, what the number of 
non-migrated should be returned?  Suppose multiple THP pages have been 
split?

Last concern is that, we will try to migrate subpages of the THP in the 
second round, but if some non-migrated pages are still remained in the 
'from' list, we will do another redundant migration for these 
failed-migration pages and no failed counting for them. We can fix this 
issue by moving the remained non-migrated pages to 'ret_pages' list, 
which will put back to 'from' list when returning from this function.

if (!list_empty(&thp_split_pages)) {
+	list_splice(from, &ret_pages);
	list_splice(&thp_split_pages, from);
	no_failed_counting = true;
	goto thp_subpage_migration;
}

The concern 2 is more complicated, or we can just use 'nr_thp_split' to 
return if some subpages are failed to be migrated to simplify the case, 
no matter how many subpages are failed?

rc = nr_failed + nr_thp_failed + nr_thp_split;

> An untested but compiled patch (please apply it before this one) looks like:
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 1852d787e6ab..f7e424f8e647 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1457,13 +1457,16 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>          int swapwrite = current->flags & PF_SWAPWRITE;
>          int rc, nr_subpages;
>          LIST_HEAD(ret_pages);
> +       LIST_HEAD(thp_split_pages);
>          bool nosplit = (reason == MR_NUMA_MISPLACED);
> +       bool no_failed_counting = false;
> 
>          trace_mm_migrate_pages_start(mode, reason);
> 
>          if (!swapwrite)
>                  current->flags |= PF_SWAPWRITE;
> 
> +thp_subpage_migration:
>          for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
>                  retry = 0;
>                  thp_retry = 0;
> @@ -1512,7 +1515,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                          case -ENOSYS:
>                                  /* THP migration is unsupported */
>                                  if (is_thp) {
> -                                       if (!try_split_thp(page, &page2, from)) {
> +                                       if (!try_split_thp(page, &page2, &thp_split_pages)) {
>                                                  nr_thp_split++;
>                                                  goto retry;
>                                          }
> @@ -1523,7 +1526,8 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                                  }
> 
>                                  /* Hugetlb migration is unsupported */
> -                               nr_failed++;
> +                               if (!no_failed_counting)
> +                                       nr_failed++;
>                                  break;
>                          case -ENOMEM:
>                                  /*
> @@ -1532,7 +1536,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                                   * THP NUMA faulting doesn't split THP to retry.
>                                   */
>                                  if (is_thp && !nosplit) {
> -                                       if (!try_split_thp(page, &page2, from)) {
> +                                       if (!try_split_thp(page, &page2, &thp_split_pages)) {
>                                                  nr_thp_split++;
>                                                  goto retry;
>                                          }
> @@ -1541,7 +1545,8 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                                          nr_failed += nr_subpages;
>                                          goto out;
>                                  }
> -                               nr_failed++;
> +                               if (!no_failed_counting)
> +                                       nr_failed++;
>                                  goto out;
>                          case -EAGAIN:
>                                  if (is_thp) {
> @@ -1570,13 +1575,24 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                                          nr_failed += nr_subpages;
>                                          break;
>                                  }
> -                               nr_failed++;
> +                               if (!no_failed_counting)
> +                                       nr_failed++;
>                                  break;
>                          }
>                  }
>          }
> -       nr_failed += retry + thp_retry;
> +       /* get thp_retry before it can be reset in THP subpage migration. */
>          nr_thp_failed += thp_retry;
> +       /* try to migrate subpages of fail-to-migrate THPs, no nr_failed
> +        * counting in this round, since all subpages of a THP is counted as
> +        * 1 failure in the first round. */
> +       if (!list_empty(&thp_split_pages)) {
> +               list_splice(from, &thp_split_pages);

Should be list_splice(&thp_split_pages, from)?

> +               no_failed_counting = true;
> +               goto thp_subpage_migration;
> +       }
> +
> +       nr_failed += retry + thp_retry;
>          rc = nr_failed;
>   out:
>          /*
> --
> Best Regards,
> Yan, Zi
> 

Re: [RFC PATCH 1/3] mm: migrate: Fix the return value of migrate_pages()

[Zi Yan]

[-- Attachment #1: Type: text/plain, Size: 14974 bytes --]

On 5 Nov 2021, at 6:17, Baolin Wang wrote:

> On 2021/11/4 23:33, Zi Yan wrote:
>> On 3 Nov 2021, at 6:51, Baolin Wang wrote:
>>
>>> As Zi Yan pointed out, the syscall move_pages() can return a non-migrated
>>> number larger than the number of pages the users tried to migrate, when a
>>> THP page is failed to migrate. This is confusing for users.
>>>
>>> Since other migration scenarios do not care about the actual non-migrated
>>> number of pages except the memory compaction migration which will fix in
>>> following patch. Thus we can change the return value to return the number
>>> of {normal page, THP, hugetlb} instead to avoid this issue, meanwhile we
>>> should still keep the migration counters using the number of normal pages.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>> ---
>>>   mm/migrate.c | 18 ++++++++++--------
>>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>> index a11e948..00b8922 100644
>>> --- a/mm/migrate.c
>>> +++ b/mm/migrate.c
>>> @@ -1428,7 +1428,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>>>    * @mode:		The migration mode that specifies the constraints for
>>>    *			page migration, if any.
>>>    * @reason:		The reason for page migration.
>>> - * @ret_succeeded:	Set to the number of pages migrated successfully if
>>> + * @ret_succeeded:	Set to the number of normal pages migrated successfully if
>>>    *			the caller passes a non-NULL pointer.
>>>    *
>>>    * The function returns after 10 attempts or if no pages are movable any more
>>> @@ -1436,7 +1436,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>>>    * It is caller's responsibility to call putback_movable_pages() to return pages
>>>    * to the LRU or free list only if ret != 0.
>>>    *
>>> - * Returns the number of pages that were not migrated, or an error code.
>>> + * Returns the number of {normal page, THP} that were not migrated, or an error code.
>>>    */
>>>   int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>   		free_page_t put_new_page, unsigned long private,
>>> @@ -1445,6 +1445,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>   	int retry = 1;
>>>   	int thp_retry = 1;
>>>   	int nr_failed = 0;
>>> +	int nr_failed_pages = 0;
>>>   	int nr_succeeded = 0;
>>>   	int nr_thp_succeeded = 0;
>>>   	int nr_thp_failed = 0;
>>> @@ -1517,7 +1518,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>   					}
>>>
>>>   					nr_thp_failed++;
>>> -					nr_failed += nr_subpages;
>>> +					nr_failed_pages += nr_subpages;
>>>   					break;
>>>   				}
>>>
>>> @@ -1537,7 +1538,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>   					}
>>>
>>>   					nr_thp_failed++;
>>> -					nr_failed += nr_subpages;
>>> +					nr_failed_pages += nr_subpages;
>>>   					goto out;
>>>   				}
>>>   				nr_failed++;
>>> @@ -1566,7 +1567,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>   				 */
>>>   				if (is_thp) {
>>>   					nr_thp_failed++;
>>> -					nr_failed += nr_subpages;
>>> +					nr_failed_pages += nr_subpages;
>>>   					break;
>>>   				}
>>>   				nr_failed++;
>>> @@ -1575,8 +1576,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>   		}
>>>   	}
>>>   	nr_failed += retry + thp_retry;
>>
>> This line can probably go away, since we do not want to count retried pages.
>
> OK.

My bad, I misread the code. This should stay, since each -EAGIN does not
increase nr_failed or nr_thp_failed and after the for loop, retry and thp_retry
give the number of pages that fail to migrate after 10 retries.

>
>>
>>> +	nr_failed_pages += nr_failed;
>>>   	nr_thp_failed += thp_retry;
>>> -	rc = nr_failed;
>>> +	rc = nr_failed + nr_thp_failed;
>>>   out:
>>>   	/*
>>>   	 * Put the permanent failure page back to migration list, they
>>> @@ -1585,11 +1587,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>   	list_splice(&ret_pages, from);
>>>
>>>   	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
>>> -	count_vm_events(PGMIGRATE_FAIL, nr_failed);
>>> +	count_vm_events(PGMIGRATE_FAIL, nr_failed_pages);
>>>   	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
>>>   	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
>>>   	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
>>> -	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
>>> +	trace_mm_migrate_pages(nr_succeeded, nr_failed_pages, nr_thp_succeeded,
>>>   			       nr_thp_failed, nr_thp_split, mode, reason);
>>>
>>>   	if (!swapwrite)
>>> -- 
>>> 1.8.3.1
>>
>> Thank you for the patch!
>>
>> In general, this looks good to me. But like you said in other email, when a THP fails to
>> migrate and gets split, the number of nr_failed will still be inflated by the number of
>> failed subpage migrations. What I can think of is to split THPs to a separate list and
>> stop increasing nr_failed when the pages from the new list is under migration. Let me
>> know how it sounds to you.
>
> Thanks for your patch, but I think it does not cover all the cases.
>
> Firstly, what confuses me is that if we return 1 to users when failed to migrate 1 THP page, but actually we may have migrated some normal pages successfaully if the THP page is split. Anyway we can add some comments for migrate_pages() to explain it if this is acceptable.

Sure.

>
> Another concern about your patch is that, if the first round migration all pages are migrated successfaully (nr_failed = 0), but if failed to migrate the subpages of the THP in the second round, we will still return 0 to users, which is incorrect. Further more, if the

Ah, I missed this. We should increase nr_thp_failed when THP is split.

> subpages of the THP are migrated partially in the second round, what the number of non-migrated should be returned?  Suppose multiple THP pages have been split?

Assuming users do not know/care what kinds of pages are in their migration list, we can
just return 1 if a THP is split, no matter how many subpages are migrated successfully.
If they do want to know the details, they probably can check the tracepoints.

>
> Last concern is that, we will try to migrate subpages of the THP in the second round, but if some non-migrated pages are still remained in the 'from' list, we will do another redundant migration for these failed-migration pages and no failed counting for them. We can fix this issue by moving the remained non-migrated pages to 'ret_pages' list, which will put back to 'from' list when returning from this function.
>
> if (!list_empty(&thp_split_pages)) {
> +	list_splice(from, &ret_pages);
> 	list_splice(&thp_split_pages, from);
> 	no_failed_counting = true;
> 	goto thp_subpage_migration;
> }

You are right. At this point, pages in the from list have been retried 10 times,
no need to migrate them again.

>
> The concern 2 is more complicated, or we can just use 'nr_thp_split' to return if some subpages are failed to be migrated to simplify the case, no matter how many subpages are failed?
>
> rc = nr_failed + nr_thp_failed + nr_thp_split;

Like I said above, I would just increase nr_thp_failed when a THP is split. Here is the patch
I come up with based on your feedback above. This time I incorporated your patch above. Please
let me know how it looks. Thanks.

diff --git a/mm/migrate.c b/mm/migrate.c
index 1852d787e6ab..a5fad22259a3 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1429,7 +1429,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
  * @mode:              The migration mode that specifies the constraints for
  *                     page migration, if any.
  * @reason:            The reason for page migration.
- * @ret_succeeded:     Set to the number of pages migrated successfully if
+ * @ret_succeeded:     Set to the number of normal pages migrated successfully if
  *                     the caller passes a non-NULL pointer.
  *
  * The function returns after 10 attempts or if no pages are movable any more
@@ -1437,7 +1437,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
  * It is caller's responsibility to call putback_movable_pages() to return pages
  * to the LRU or free list only if ret != 0.
  *
- * Returns the number of pages that were not migrated, or an error code.
+ * Returns the number of {normal page, THP} that were not migrated, or an error code.
  */
 int migrate_pages(struct list_head *from, new_page_t get_new_page,
                free_page_t put_new_page, unsigned long private,
@@ -1446,6 +1446,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
        int retry = 1;
        int thp_retry = 1;
        int nr_failed = 0;
+       int nr_failed_pages = 0;
        int nr_succeeded = 0;
        int nr_thp_succeeded = 0;
        int nr_thp_failed = 0;
@@ -1457,13 +1458,16 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
        int swapwrite = current->flags & PF_SWAPWRITE;
        int rc, nr_subpages;
        LIST_HEAD(ret_pages);
+       LIST_HEAD(thp_split_pages);
        bool nosplit = (reason == MR_NUMA_MISPLACED);
+       bool dont_count_failed_subpage = false;

        trace_mm_migrate_pages_start(mode, reason);

        if (!swapwrite)
                current->flags |= PF_SWAPWRITE;

+thp_subpage_migration:
        for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
                retry = 0;
                thp_retry = 0;
@@ -1512,18 +1516,21 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
                        case -ENOSYS:
                                /* THP migration is unsupported */
                                if (is_thp) {
-                                       if (!try_split_thp(page, &page2, from)) {
+                                       nr_thp_failed++;
+                                       if (!try_split_thp(page, &page2, &thp_split_pages)) {
                                                nr_thp_split++;
                                                goto retry;
                                        }

-                                       nr_thp_failed++;
-                                       nr_failed += nr_subpages;
+                                       nr_failed_pages += nr_subpages;
                                        break;
                                }

                                /* Hugetlb migration is unsupported */
-                               nr_failed++;
+                               if (!dont_count_failed_subpage) {
+                                       nr_failed++;
+                                       nr_failed_pages++;
+                               }
                                break;
                        case -ENOMEM:
                                /*
@@ -1532,16 +1539,19 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
                                 * THP NUMA faulting doesn't split THP to retry.
                                 */
                                if (is_thp && !nosplit) {
-                                       if (!try_split_thp(page, &page2, from)) {
+                                       nr_thp_failed++;
+                                       if (!try_split_thp(page, &page2, &thp_split_pages)) {
                                                nr_thp_split++;
                                                goto retry;
                                        }

-                                       nr_thp_failed++;
-                                       nr_failed += nr_subpages;
+                                       nr_failed_pages += nr_subpages;
                                        goto out;
                                }
-                               nr_failed++;
+                               if (!dont_count_failed_subpage) {
+                                       nr_failed++;
+                                       nr_failed_pages++;
+                               }
                                goto out;
                        case -EAGAIN:
                                if (is_thp) {
@@ -1567,17 +1577,34 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
                                 */
                                if (is_thp) {
                                        nr_thp_failed++;
-                                       nr_failed += nr_subpages;
+                                       nr_failed_pages += nr_subpages;
                                        break;
                                }
-                               nr_failed++;
+                               if (!dont_count_failed_subpage) {
+                                       nr_failed++;
+                                       nr_failed_pages++;
+                               }
                                break;
                        }
                }
        }
        nr_failed += retry + thp_retry;
        nr_thp_failed += thp_retry;
-       rc = nr_failed;
+       /*
+        * try to migrate subpages of fail-to-migrate THPs, no nr_failed
+        * counting in this round, since all subpages of a THP is counted as
+        * 1 failure in the first round.
+        */
+       if (!list_empty(&thp_split_pages)) {
+               /*
+                * move non-migrated pages (after 10 retries) to ret_pages to
+                * avoid migrating them again.
+                */
+               list_splice(from, &ret_pages);
+               list_splice(&thp_split_pages, from);
+               dont_count_failed_subpage = true;
+               goto thp_subpage_migration;
+       }
+
+       rc = nr_failed + nr_thp_failed;
 out:
        /*
         * Put the permanent failure page back to migration list, they
@@ -1586,11 +1613,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
        list_splice(&ret_pages, from);

        count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
-       count_vm_events(PGMIGRATE_FAIL, nr_failed);
+       count_vm_events(PGMIGRATE_FAIL, nr_failed_pages);
        count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
        count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
        count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
-       trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
+       trace_mm_migrate_pages(nr_succeeded, nr_failed_pages, nr_thp_succeeded,
                               nr_thp_failed, nr_thp_split, mode, reason);

        if (!swapwrite)

--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]

Re: [RFC PATCH 1/3] mm: migrate: Fix the return value of migrate_pages()

[Baolin Wang]

On 2021/11/5 23:21, Zi Yan wrote:
> On 5 Nov 2021, at 6:17, Baolin Wang wrote:
> 
>> On 2021/11/4 23:33, Zi Yan wrote:
>>> On 3 Nov 2021, at 6:51, Baolin Wang wrote:
>>>
>>>> As Zi Yan pointed out, the syscall move_pages() can return a non-migrated
>>>> number larger than the number of pages the users tried to migrate, when a
>>>> THP page is failed to migrate. This is confusing for users.
>>>>
>>>> Since other migration scenarios do not care about the actual non-migrated
>>>> number of pages except the memory compaction migration which will fix in
>>>> following patch. Thus we can change the return value to return the number
>>>> of {normal page, THP, hugetlb} instead to avoid this issue, meanwhile we
>>>> should still keep the migration counters using the number of normal pages.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>>> ---
>>>>    mm/migrate.c | 18 ++++++++++--------
>>>>    1 file changed, 10 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/mm/migrate.c b/mm/migrate.c
>>>> index a11e948..00b8922 100644
>>>> --- a/mm/migrate.c
>>>> +++ b/mm/migrate.c
>>>> @@ -1428,7 +1428,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>>>>     * @mode:		The migration mode that specifies the constraints for
>>>>     *			page migration, if any.
>>>>     * @reason:		The reason for page migration.
>>>> - * @ret_succeeded:	Set to the number of pages migrated successfully if
>>>> + * @ret_succeeded:	Set to the number of normal pages migrated successfully if
>>>>     *			the caller passes a non-NULL pointer.
>>>>     *
>>>>     * The function returns after 10 attempts or if no pages are movable any more
>>>> @@ -1436,7 +1436,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>>>>     * It is caller's responsibility to call putback_movable_pages() to return pages
>>>>     * to the LRU or free list only if ret != 0.
>>>>     *
>>>> - * Returns the number of pages that were not migrated, or an error code.
>>>> + * Returns the number of {normal page, THP} that were not migrated, or an error code.
>>>>     */
>>>>    int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    		free_page_t put_new_page, unsigned long private,
>>>> @@ -1445,6 +1445,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    	int retry = 1;
>>>>    	int thp_retry = 1;
>>>>    	int nr_failed = 0;
>>>> +	int nr_failed_pages = 0;
>>>>    	int nr_succeeded = 0;
>>>>    	int nr_thp_succeeded = 0;
>>>>    	int nr_thp_failed = 0;
>>>> @@ -1517,7 +1518,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    					}
>>>>
>>>>    					nr_thp_failed++;
>>>> -					nr_failed += nr_subpages;
>>>> +					nr_failed_pages += nr_subpages;
>>>>    					break;
>>>>    				}
>>>>
>>>> @@ -1537,7 +1538,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    					}
>>>>
>>>>    					nr_thp_failed++;
>>>> -					nr_failed += nr_subpages;
>>>> +					nr_failed_pages += nr_subpages;
>>>>    					goto out;
>>>>    				}
>>>>    				nr_failed++;
>>>> @@ -1566,7 +1567,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    				 */
>>>>    				if (is_thp) {
>>>>    					nr_thp_failed++;
>>>> -					nr_failed += nr_subpages;
>>>> +					nr_failed_pages += nr_subpages;
>>>>    					break;
>>>>    				}
>>>>    				nr_failed++;
>>>> @@ -1575,8 +1576,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    		}
>>>>    	}
>>>>    	nr_failed += retry + thp_retry;
>>>
>>> This line can probably go away, since we do not want to count retried pages.
>>
>> OK.
> 
> My bad, I misread the code. This should stay, since each -EAGIN does not
> increase nr_failed or nr_thp_failed and after the for loop, retry and thp_retry
> give the number of pages that fail to migrate after 10 retries.

Ah, right.

>>
>>>
>>>> +	nr_failed_pages += nr_failed;
>>>>    	nr_thp_failed += thp_retry;
>>>> -	rc = nr_failed;
>>>> +	rc = nr_failed + nr_thp_failed;
>>>>    out:
>>>>    	/*
>>>>    	 * Put the permanent failure page back to migration list, they
>>>> @@ -1585,11 +1587,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>>>>    	list_splice(&ret_pages, from);
>>>>
>>>>    	count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
>>>> -	count_vm_events(PGMIGRATE_FAIL, nr_failed);
>>>> +	count_vm_events(PGMIGRATE_FAIL, nr_failed_pages);
>>>>    	count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
>>>>    	count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
>>>>    	count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
>>>> -	trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
>>>> +	trace_mm_migrate_pages(nr_succeeded, nr_failed_pages, nr_thp_succeeded,
>>>>    			       nr_thp_failed, nr_thp_split, mode, reason);
>>>>
>>>>    	if (!swapwrite)
>>>> -- 
>>>> 1.8.3.1
>>>
>>> Thank you for the patch!
>>>
>>> In general, this looks good to me. But like you said in other email, when a THP fails to
>>> migrate and gets split, the number of nr_failed will still be inflated by the number of
>>> failed subpage migrations. What I can think of is to split THPs to a separate list and
>>> stop increasing nr_failed when the pages from the new list is under migration. Let me
>>> know how it sounds to you.
>>
>> Thanks for your patch, but I think it does not cover all the cases.
>>
>> Firstly, what confuses me is that if we return 1 to users when failed to migrate 1 THP page, but actually we may have migrated some normal pages successfaully if the THP page is split. Anyway we can add some comments for migrate_pages() to explain it if this is acceptable.
> 
> Sure.
> 
>>
>> Another concern about your patch is that, if the first round migration all pages are migrated successfaully (nr_failed = 0), but if failed to migrate the subpages of the THP in the second round, we will still return 0 to users, which is incorrect. Further more, if the
> 
> Ah, I missed this. We should increase nr_thp_failed when THP is split.
> 
>> subpages of the THP are migrated partially in the second round, what the number of non-migrated should be returned?  Suppose multiple THP pages have been split?
> 
> Assuming users do not know/care what kinds of pages are in their migration list, we can
> just return 1 if a THP is split, no matter how many subpages are migrated successfully.
> If they do want to know the details, they probably can check the tracepoints.
OK.

> 
>>
>> Last concern is that, we will try to migrate subpages of the THP in the second round, but if some non-migrated pages are still remained in the 'from' list, we will do another redundant migration for these failed-migration pages and no failed counting for them. We can fix this issue by moving the remained non-migrated pages to 'ret_pages' list, which will put back to 'from' list when returning from this function.
>>
>> if (!list_empty(&thp_split_pages)) {
>> +	list_splice(from, &ret_pages);
>> 	list_splice(&thp_split_pages, from);
>> 	no_failed_counting = true;
>> 	goto thp_subpage_migration;
>> }
> 
> You are right. At this point, pages in the from list have been retried 10 times,
> no need to migrate them again.
> 
>>
>> The concern 2 is more complicated, or we can just use 'nr_thp_split' to return if some subpages are failed to be migrated to simplify the case, no matter how many subpages are failed?
>>
>> rc = nr_failed + nr_thp_failed + nr_thp_split;
> 
> Like I said above, I would just increase nr_thp_failed when a THP is split. Here is the patch
> I come up with based on your feedback above. This time I incorporated your patch above. Please
> let me know how it looks. Thanks.

Overall it looks good, but I found some issues as below when I did some 
testing, otherwise it looks good to me. I've folded this into my new 
patch set, please help to review again. Thanks.

> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 1852d787e6ab..a5fad22259a3 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1429,7 +1429,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>    * @mode:              The migration mode that specifies the constraints for
>    *                     page migration, if any.
>    * @reason:            The reason for page migration.
> - * @ret_succeeded:     Set to the number of pages migrated successfully if
> + * @ret_succeeded:     Set to the number of normal pages migrated successfully if
>    *                     the caller passes a non-NULL pointer.
>    *
>    * The function returns after 10 attempts or if no pages are movable any more
> @@ -1437,7 +1437,7 @@ static inline int try_split_thp(struct page *page, struct page **page2,
>    * It is caller's responsibility to call putback_movable_pages() to return pages
>    * to the LRU or free list only if ret != 0.
>    *
> - * Returns the number of pages that were not migrated, or an error code.
> + * Returns the number of {normal page, THP} that were not migrated, or an error code.
>    */
>   int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                  free_page_t put_new_page, unsigned long private,
> @@ -1446,6 +1446,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>          int retry = 1;
>          int thp_retry = 1;
>          int nr_failed = 0;
> +       int nr_failed_pages = 0;
>          int nr_succeeded = 0;
>          int nr_thp_succeeded = 0;
>          int nr_thp_failed = 0;
> @@ -1457,13 +1458,16 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>          int swapwrite = current->flags & PF_SWAPWRITE;
>          int rc, nr_subpages;
>          LIST_HEAD(ret_pages);
> +       LIST_HEAD(thp_split_pages);
>          bool nosplit = (reason == MR_NUMA_MISPLACED);
> +       bool dont_count_failed_subpage = false;
> 
>          trace_mm_migrate_pages_start(mode, reason);
> 
>          if (!swapwrite)
>                  current->flags |= PF_SWAPWRITE;
> 
> +thp_subpage_migration:
>          for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
>                  retry = 0;
>                  thp_retry = 0;
> @@ -1512,18 +1516,21 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                          case -ENOSYS:
>                                  /* THP migration is unsupported */
>                                  if (is_thp) {
> -                                       if (!try_split_thp(page, &page2, from)) {
> +                                       nr_thp_failed++;
> +                                       if (!try_split_thp(page, &page2, &thp_split_pages)) {
>                                                  nr_thp_split++;
>                                                  goto retry;
>                                          }
> 
> -                                       nr_thp_failed++;
> -                                       nr_failed += nr_subpages;
> +                                       nr_failed_pages += nr_subpages;
>                                          break;
>                                  }
> 
>                                  /* Hugetlb migration is unsupported */
> -                               nr_failed++;
> +                               if (!dont_count_failed_subpage) {
> +                                       nr_failed++;
> +                                       nr_failed_pages++;
> +                               }

Shoule be:
	if (!dont_count_failed_subpage)
		nr_failed++;
	nr_failed_pages++;

Since we still need count the number of normal pages into 
"nr_failed_pages" in the second round.

>                                  break;
>                          case -ENOMEM:
>                                  /*
> @@ -1532,16 +1539,19 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                                   * THP NUMA faulting doesn't split THP to retry.
>                                   */
>                                  if (is_thp && !nosplit) {
> -                                       if (!try_split_thp(page, &page2, from)) {
> +                                       nr_thp_failed++;
> +                                       if (!try_split_thp(page, &page2, &thp_split_pages)) {
>                                                  nr_thp_split++;
>                                                  goto retry;
>                                          }
> 
> -                                       nr_thp_failed++;
> -                                       nr_failed += nr_subpages;
> +                                       nr_failed_pages += nr_subpages;
>                                          goto out;
>                                  }
> -                               nr_failed++;
> +                               if (!dont_count_failed_subpage) {
> +                                       nr_failed++;
> +                                       nr_failed_pages++;
> +                               }

Ditto

>                                  goto out;
>                          case -EAGAIN:
>                                  if (is_thp) {
> @@ -1567,17 +1577,34 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>                                   */
>                                  if (is_thp) {
>                                          nr_thp_failed++;
> -                                       nr_failed += nr_subpages;
> +                                       nr_failed_pages += nr_subpages;
>                                          break;
>                                  }
> -                               nr_failed++;
> +                               if (!dont_count_failed_subpage) {
> +                                       nr_failed++;
> +                                       nr_failed_pages++;
> +                               }

Ditto.

>                                  break;
>                          }
>                  }
>          }
>          nr_failed += retry + thp_retry;
>          nr_thp_failed += thp_retry;
> -       rc = nr_failed;
> +       /*
> +        * try to migrate subpages of fail-to-migrate THPs, no nr_failed
> +        * counting in this round, since all subpages of a THP is counted as
> +        * 1 failure in the first round.
> +        */
> +       if (!list_empty(&thp_split_pages)) {
> +               /*
> +                * move non-migrated pages (after 10 retries) to ret_pages to
> +                * avoid migrating them again.
> +                */
> +               list_splice(from, &ret_pages);
> +               list_splice(&thp_split_pages, from);

should be:
	list_splice_init(from, &ret_pages);
	list_splice_init(&thp_split_pages, from);

Since the 'from' and  'thp_split_pages' will be reused, otherwise will 
cause dead-loop.

> +               dont_count_failed_subpage = true;

Should add 'retry = 1;' here, otherwise it will not try to migrate subpages.

> +               goto thp_subpage_migration;
> +       }
> +
> +       rc = nr_failed + nr_thp_failed;
>   out:
>          /*
>           * Put the permanent failure page back to migration list, they
> @@ -1586,11 +1613,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>          list_splice(&ret_pages, from);
> 
>          count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded);
> -       count_vm_events(PGMIGRATE_FAIL, nr_failed);
> +       count_vm_events(PGMIGRATE_FAIL, nr_failed_pages);
>          count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded);
>          count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed);
>          count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split);
> -       trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded,
> +       trace_mm_migrate_pages(nr_succeeded, nr_failed_pages, nr_thp_succeeded,
>                                 nr_thp_failed, nr_thp_split, mode, reason);
> 
>          if (!swapwrite)
> 
> --
> Best Regards,
> Yan, Zi
>