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

From: Baolin Wang
Date: Sun Nov 07 2021 - 04:07:28 EST

Next message: Krzysztof Kozlowski: "Re: [RFC PATCH] ARM: s3c: mark as deprecated and schedule removal after 2022"
Previous message: Ian Rogers: "Re: [PATCH] perf metric: Fix memory leaks."
In reply to: Zi Yan: "Re: [RFC PATCH 1/3] mm: migrate: Fix the return value of migrate_pages()"
Next in thread: Baolin Wang: "[RFC PATCH 3/3] mm: compaction: Fix the migration stats in trace_mm_compaction_migratepages()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxxxxxx>
---
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

Next message: Krzysztof Kozlowski: "Re: [RFC PATCH] ARM: s3c: mark as deprecated and schedule removal after 2022"
Previous message: Ian Rogers: "Re: [PATCH] perf metric: Fix memory leaks."
In reply to: Zi Yan: "Re: [RFC PATCH 1/3] mm: migrate: Fix the return value of migrate_pages()"
Next in thread: Baolin Wang: "[RFC PATCH 3/3] mm: compaction: Fix the migration stats in trace_mm_compaction_migratepages()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]