* [PATCH -V2 1/7] migrate: fix syscall move_pages() return value for failure
2022-07-11 8:49 [PATCH -V2 0/7] migrate_pages(): fix several bugs in error path Huang Ying
@ 2022-07-11 8:49 ` Huang Ying
2022-07-11 11:52 ` Oscar Salvador
2022-07-11 8:49 ` [PATCH -V2 2/7] migrate_pages(): remove unnecessary list_safe_reset_next() Huang Ying
` (5 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Huang Ying @ 2022-07-11 8:49 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Huang Ying, Baolin Wang, Zi Yan, Yang Shi
The return value of move_pages() syscall is incorrect when counting
the remaining pages to be migrated. For example, for the following
test program,
"
#define _GNU_SOURCE
#include <stdbool.h>
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <errno.h>
#include <fcntl.h>
#include <sys/uio.h>
#include <sys/mman.h>
#include <sys/types.h>
#include <unistd.h>
#include <numaif.h>
#include <numa.h>
#ifndef MADV_FREE
#define MADV_FREE 8 /* free pages only if memory pressure */
#endif
#define ONE_MB (1024 * 1024)
#define MAP_SIZE (16 * ONE_MB)
#define THP_SIZE (2 * ONE_MB)
#define THP_MASK (THP_SIZE - 1)
#define ERR_EXIT_ON(cond, msg) \
do { \
int __cond_in_macro = (cond); \
if (__cond_in_macro) \
error_exit(__cond_in_macro, (msg)); \
} while (0)
void error_msg(int ret, int nr, int *status, const char *msg)
{
int i;
fprintf(stderr, "Error: %s, ret : %d, error: %s\n",
msg, ret, strerror(errno));
if (!nr)
return;
fprintf(stderr, "status: ");
for (i = 0; i < nr; i++)
fprintf(stderr, "%d ", status[i]);
fprintf(stderr, "\n");
}
void error_exit(int ret, const char *msg)
{
error_msg(ret, 0, NULL, msg);
exit(1);
}
int page_size;
bool do_vmsplice;
bool do_thp;
static int pipe_fds[2];
void *addr;
char *pn;
char *pn1;
void *pages[2];
int status[2];
void prepare()
{
int ret;
struct iovec iov;
if (addr) {
munmap(addr, MAP_SIZE);
close(pipe_fds[0]);
close(pipe_fds[1]);
}
ret = pipe(pipe_fds);
ERR_EXIT_ON(ret, "pipe");
addr = mmap(NULL, MAP_SIZE, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
ERR_EXIT_ON(addr == MAP_FAILED, "mmap");
if (do_thp) {
ret = madvise(addr, MAP_SIZE, MADV_HUGEPAGE);
ERR_EXIT_ON(ret, "advise hugepage");
}
pn = (char *)(((unsigned long)addr + THP_SIZE) & ~THP_MASK);
pn1 = pn + THP_SIZE;
pages[0] = pn;
pages[1] = pn1;
*pn = 1;
if (do_vmsplice) {
iov.iov_base = pn;
iov.iov_len = page_size;
ret = vmsplice(pipe_fds[1], &iov, 1, 0);
ERR_EXIT_ON(ret < 0, "vmsplice");
}
status[0] = status[1] = 1024;
}
void test_migrate()
{
int ret;
int nodes[2] = { 1, 1 };
pid_t pid = getpid();
prepare();
ret = move_pages(pid, 1, pages, nodes, status, MPOL_MF_MOVE_ALL);
error_msg(ret, 1, status, "move 1 page");
prepare();
ret = move_pages(pid, 2, pages, nodes, status, MPOL_MF_MOVE_ALL);
error_msg(ret, 2, status, "move 2 pages, page 1 not mapped");
prepare();
*pn1 = 1;
ret = move_pages(pid, 2, pages, nodes, status, MPOL_MF_MOVE_ALL);
error_msg(ret, 2, status, "move 2 pages");
prepare();
*pn1 = 1;
nodes[1] = 0;
ret = move_pages(pid, 2, pages, nodes, status, MPOL_MF_MOVE_ALL);
error_msg(ret, 2, status, "move 2 pages, page 1 to node 0");
}
int main(int argc, char *argv[])
{
numa_run_on_node(0);
page_size = getpagesize();
test_migrate();
fprintf(stderr, "\nMake page 0 cannot be migrated:\n");
do_vmsplice = true;
test_migrate();
fprintf(stderr, "\nTest THP:\n");
do_thp = true;
do_vmsplice = false;
test_migrate();
fprintf(stderr, "\nTHP: make page 0 cannot be migrated:\n");
do_vmsplice = true;
test_migrate();
return 0;
}
"
The output of the current kernel is,
"
Error: move 1 page, ret : 0, error: Success
status: 1
Error: move 2 pages, page 1 not mapped, ret : 0, error: Success
status: 1 -14
Error: move 2 pages, ret : 0, error: Success
status: 1 1
Error: move 2 pages, page 1 to node 0, ret : 0, error: Success
status: 1 0
Make page 0 cannot be migrated:
Error: move 1 page, ret : 0, error: Success
status: 1024
Error: move 2 pages, page 1 not mapped, ret : 1, error: Success
status: 1024 -14
Error: move 2 pages, ret : 0, error: Success
status: 1024 1024
Error: move 2 pages, page 1 to node 0, ret : 1, error: Success
status: 1024 1024
"
While the expected output is,
"
Error: move 1 page, ret : 0, error: Success
status: 1
Error: move 2 pages, page 1 not mapped, ret : 0, error: Success
status: 1 -14
Error: move 2 pages, ret : 0, error: Success
status: 1 1
Error: move 2 pages, page 1 to node 0, ret : 0, error: Success
status: 1 0
Make page 0 cannot be migrated:
Error: move 1 page, ret : 1, error: Success
status: 1024
Error: move 2 pages, page 1 not mapped, ret : 1, error: Success
status: 1024 -14
Error: move 2 pages, ret : 1, error: Success
status: 1024 1024
Error: move 2 pages, page 1 to node 0, ret : 2, error: Success
status: 1024 1024
"
Fix this via correcting the remaining pages counting. With the fix,
the output for the test program as above is expected.
Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Fixes: 5984fabb6e82 ("mm: move_pages: report the number of non-attempted pages")
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Zi Yan <ziy@nvidia.com>
Cc: Yang Shi <shy828301@gmail.com>
---
mm/migrate.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 6c1ea61f39d8..472335f0aaa3 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1682,7 +1682,7 @@ static int move_pages_and_store_status(struct mm_struct *mm, int node,
* well.
*/
if (err > 0)
- err += nr_pages - i - 1;
+ err += nr_pages - i;
return err;
}
return store_status(status, start, node, i - start);
@@ -1768,8 +1768,12 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
err = move_pages_and_store_status(mm, current_node, &pagelist,
status, start, i, nr_pages);
- if (err)
+ if (err) {
+ /* We have stored status of page i */
+ if (err > 0)
+ err--;
goto out;
+ }
current_node = NUMA_NO_NODE;
}
out_flush:
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH -V2 1/7] migrate: fix syscall move_pages() return value for failure
2022-07-11 8:49 ` [PATCH -V2 1/7] migrate: fix syscall move_pages() return value for failure Huang Ying
@ 2022-07-11 11:52 ` Oscar Salvador
2022-07-12 0:59 ` Huang, Ying
0 siblings, 1 reply; 17+ messages in thread
From: Oscar Salvador @ 2022-07-11 11:52 UTC (permalink / raw)
To: Huang Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Baolin Wang, Zi Yan, Yang Shi
On Mon, Jul 11, 2022 at 04:49:42PM +0800, Huang Ying wrote:
> return store_status(status, start, node, i - start);
> @@ -1768,8 +1768,12 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>
> err = move_pages_and_store_status(mm, current_node, &pagelist,
> status, start, i, nr_pages);
> - if (err)
> + if (err) {
> + /* We have stored status of page i */
IMHO, "We have accounted for page i" gives a more clear statement.
Reviewed-by: Oscar Salvador <osalvador@suse.de>
> + if (err > 0)
> + err--;
> goto out;
> + }
> current_node = NUMA_NO_NODE;
> }
> out_flush:
> --
> 2.30.2
>
>
--
Oscar Salvador
SUSE Labs
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -V2 1/7] migrate: fix syscall move_pages() return value for failure
2022-07-11 11:52 ` Oscar Salvador
@ 2022-07-12 0:59 ` Huang, Ying
0 siblings, 0 replies; 17+ messages in thread
From: Huang, Ying @ 2022-07-12 0:59 UTC (permalink / raw)
To: Oscar Salvador
Cc: Andrew Morton, linux-mm, linux-kernel, Baolin Wang, Zi Yan, Yang Shi
Oscar Salvador <osalvador@suse.de> writes:
> On Mon, Jul 11, 2022 at 04:49:42PM +0800, Huang Ying wrote:
>> return store_status(status, start, node, i - start);
>> @@ -1768,8 +1768,12 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>>
>> err = move_pages_and_store_status(mm, current_node, &pagelist,
>> status, start, i, nr_pages);
>> - if (err)
>> + if (err) {
>> + /* We have stored status of page i */
>
> IMHO, "We have accounted for page i" gives a more clear statement.
Sure. Will change this.
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
Thanks!
Best Regards,
Huang, Ying
>> + if (err > 0)
>> + err--;
>> goto out;
>> + }
>> current_node = NUMA_NO_NODE;
>> }
>> out_flush:
>> --
>> 2.30.2
>>
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH -V2 2/7] migrate_pages(): remove unnecessary list_safe_reset_next()
2022-07-11 8:49 [PATCH -V2 0/7] migrate_pages(): fix several bugs in error path Huang Ying
2022-07-11 8:49 ` [PATCH -V2 1/7] migrate: fix syscall move_pages() return value for failure Huang Ying
@ 2022-07-11 8:49 ` Huang Ying
2022-07-11 11:57 ` Oscar Salvador
2022-07-11 8:49 ` [PATCH -V2 3/7] migrate_pages(): fix THP failure counting for -ENOMEM Huang Ying
` (4 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Huang Ying @ 2022-07-11 8:49 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Huang Ying, Baolin Wang, Zi Yan, Yang Shi
Before commit b5bade978e9b ("mm: migrate: fix the return value of
migrate_pages()"), the tail pages of THP will be put in the "from"
list directly. So one of the loop cursors (page2) needs to be reset,
as is done in try_split_thp() via list_safe_reset_next(). But after
the commit, the tail pages of THP will be put in a dedicated
list (thp_split_pages). That is, the "from" list will not be changed
during splitting. So, it's unnecessary to call list_safe_reset_next()
anymore.
This is a code cleanup, no functionality changes are expected.
Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Zi Yan <ziy@nvidia.com>
Cc: Yang Shi <shy828301@gmail.com>
---
mm/migrate.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 472335f0aaa3..794312072eb3 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1301,16 +1301,13 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
return rc;
}
-static inline int try_split_thp(struct page *page, struct page **page2,
- struct list_head *from)
+static inline int try_split_thp(struct page *page, struct list_head *split_pages)
{
- int rc = 0;
+ int rc;
lock_page(page);
- rc = split_huge_page_to_list(page, from);
+ rc = split_huge_page_to_list(page, split_pages);
unlock_page(page);
- if (!rc)
- list_safe_reset_next(page, *page2, lru);
return rc;
}
@@ -1414,7 +1411,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
/* THP migration is unsupported */
if (is_thp) {
nr_thp_failed++;
- if (!try_split_thp(page, &page2, &thp_split_pages)) {
+ if (!try_split_thp(page, &thp_split_pages)) {
nr_thp_split++;
goto retry;
}
@@ -1433,7 +1430,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
*/
if (is_thp && !nosplit) {
nr_thp_failed++;
- if (!try_split_thp(page, &page2, &thp_split_pages)) {
+ if (!try_split_thp(page, &thp_split_pages)) {
nr_thp_split++;
goto retry;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH -V2 2/7] migrate_pages(): remove unnecessary list_safe_reset_next()
2022-07-11 8:49 ` [PATCH -V2 2/7] migrate_pages(): remove unnecessary list_safe_reset_next() Huang Ying
@ 2022-07-11 11:57 ` Oscar Salvador
0 siblings, 0 replies; 17+ messages in thread
From: Oscar Salvador @ 2022-07-11 11:57 UTC (permalink / raw)
To: Huang Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Baolin Wang, Zi Yan, Yang Shi
On Mon, Jul 11, 2022 at 04:49:43PM +0800, Huang Ying wrote:
> Before commit b5bade978e9b ("mm: migrate: fix the return value of
> migrate_pages()"), the tail pages of THP will be put in the "from"
> list directly. So one of the loop cursors (page2) needs to be reset,
> as is done in try_split_thp() via list_safe_reset_next(). But after
> the commit, the tail pages of THP will be put in a dedicated
> list (thp_split_pages). That is, the "from" list will not be changed
> during splitting. So, it's unnecessary to call list_safe_reset_next()
> anymore.
>
> This is a code cleanup, no functionality changes are expected.
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Yang Shi <shy828301@gmail.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
--
Oscar Salvador
SUSE Labs
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH -V2 3/7] migrate_pages(): fix THP failure counting for -ENOMEM
2022-07-11 8:49 [PATCH -V2 0/7] migrate_pages(): fix several bugs in error path Huang Ying
2022-07-11 8:49 ` [PATCH -V2 1/7] migrate: fix syscall move_pages() return value for failure Huang Ying
2022-07-11 8:49 ` [PATCH -V2 2/7] migrate_pages(): remove unnecessary list_safe_reset_next() Huang Ying
@ 2022-07-11 8:49 ` Huang Ying
2022-07-11 12:01 ` Oscar Salvador
2022-07-11 8:49 ` [PATCH -V2 4/7] migrate_pages(): fix failure counting for THP subpages retrying Huang Ying
` (3 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Huang Ying @ 2022-07-11 8:49 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Huang Ying, Baolin Wang, Zi Yan, Yang Shi
In unmap_and_move(), if the new THP cannot be allocated, -ENOMEM will
be returned, and migrate_pages() will try to split the THP unless
"reason" is MR_NUMA_MISPLACED (that is, nosplit == true). But when
nosplit == true, the THP migration failure will not be counted. This
is incorrect. So in this patch, the THP migration failure will be
counted for -ENOMEM regardless of nosplit is true or false. The
nr_failed counting isn't fixed because it's not used. Added some
comments for it per Baolin's suggestion.
Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Fixes: 5984fabb6e82 ("mm: move_pages: report the number of non-attempted pages")
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Zi Yan <ziy@nvidia.com>
Cc: Yang Shi <shy828301@gmail.com>
---
mm/migrate.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 794312072eb3..38e2c789a9c3 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1426,11 +1426,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
/*
* When memory is low, don't bother to try to migrate
* other pages, just exit.
- * THP NUMA faulting doesn't split THP to retry.
*/
- if (is_thp && !nosplit) {
+ if (is_thp) {
nr_thp_failed++;
- if (!try_split_thp(page, &thp_split_pages)) {
+ /* THP NUMA faulting doesn't split THP to retry. */
+ if (!nosplit && !try_split_thp(page, &thp_split_pages)) {
nr_thp_split++;
goto retry;
}
@@ -1446,6 +1446,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
* the caller otherwise the page refcnt will be leaked.
*/
list_splice_init(&thp_split_pages, from);
+ /* nr_failed isn't updated for not used */
nr_thp_failed += thp_retry;
goto out;
case -EAGAIN:
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH -V2 3/7] migrate_pages(): fix THP failure counting for -ENOMEM
2022-07-11 8:49 ` [PATCH -V2 3/7] migrate_pages(): fix THP failure counting for -ENOMEM Huang Ying
@ 2022-07-11 12:01 ` Oscar Salvador
0 siblings, 0 replies; 17+ messages in thread
From: Oscar Salvador @ 2022-07-11 12:01 UTC (permalink / raw)
To: Huang Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Baolin Wang, Zi Yan, Yang Shi
On Mon, Jul 11, 2022 at 04:49:44PM +0800, Huang Ying wrote:
> In unmap_and_move(), if the new THP cannot be allocated, -ENOMEM will
> be returned, and migrate_pages() will try to split the THP unless
> "reason" is MR_NUMA_MISPLACED (that is, nosplit == true). But when
> nosplit == true, the THP migration failure will not be counted. This
> is incorrect. So in this patch, the THP migration failure will be
> counted for -ENOMEM regardless of nosplit is true or false. The
> nr_failed counting isn't fixed because it's not used. Added some
> comments for it per Baolin's suggestion.
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Fixes: 5984fabb6e82 ("mm: move_pages: report the number of non-attempted pages")
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Yang Shi <shy828301@gmail.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
> ---
> mm/migrate.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 794312072eb3..38e2c789a9c3 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1426,11 +1426,11 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> /*
> * When memory is low, don't bother to try to migrate
> * other pages, just exit.
> - * THP NUMA faulting doesn't split THP to retry.
> */
> - if (is_thp && !nosplit) {
> + if (is_thp) {
> nr_thp_failed++;
> - if (!try_split_thp(page, &thp_split_pages)) {
> + /* THP NUMA faulting doesn't split THP to retry. */
> + if (!nosplit && !try_split_thp(page, &thp_split_pages)) {
> nr_thp_split++;
> goto retry;
> }
> @@ -1446,6 +1446,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> * the caller otherwise the page refcnt will be leaked.
> */
> list_splice_init(&thp_split_pages, from);
> + /* nr_failed isn't updated for not used */
> nr_thp_failed += thp_retry;
> goto out;
> case -EAGAIN:
> --
> 2.30.2
>
>
--
Oscar Salvador
SUSE Labs
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH -V2 4/7] migrate_pages(): fix failure counting for THP subpages retrying
2022-07-11 8:49 [PATCH -V2 0/7] migrate_pages(): fix several bugs in error path Huang Ying
` (2 preceding siblings ...)
2022-07-11 8:49 ` [PATCH -V2 3/7] migrate_pages(): fix THP failure counting for -ENOMEM Huang Ying
@ 2022-07-11 8:49 ` Huang Ying
2022-07-11 12:13 ` Oscar Salvador
2022-07-11 8:49 ` [PATCH -V2 5/7] migrate_pages(): fix failure counting for THP on -ENOSYS Huang Ying
` (2 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Huang Ying @ 2022-07-11 8:49 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Huang Ying, Baolin Wang, Zi Yan, Yang Shi
If THP is failed to be migrated for -ENOSYS and -ENOMEM, the THP will
be split into thp_split_pages, and after other pages are migrated,
pages in thp_split_pages will be migrated with no_subpage_counting ==
true, because its failure have been counted already. If some pages in
thp_split_pages are retried during migration, we should not count
their failure if no_subpage_counting == true too. This is done this
patch to fix the failure counting for THP subpages retrying.
Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Fixes: 5984fabb6e82 ("mm: move_pages: report the number of non-attempted pages")
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Zi Yan <ziy@nvidia.com>
Cc: Yang Shi <shy828301@gmail.com>
---
mm/migrate.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 38e2c789a9c3..4bceba143db0 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1477,7 +1477,8 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
}
}
}
- nr_failed += retry;
+ if (!no_subpage_counting)
+ nr_failed += retry;
nr_thp_failed += thp_retry;
/*
* Try to migrate subpages of fail-to-migrate THPs, no nr_failed
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH -V2 4/7] migrate_pages(): fix failure counting for THP subpages retrying
2022-07-11 8:49 ` [PATCH -V2 4/7] migrate_pages(): fix failure counting for THP subpages retrying Huang Ying
@ 2022-07-11 12:13 ` Oscar Salvador
0 siblings, 0 replies; 17+ messages in thread
From: Oscar Salvador @ 2022-07-11 12:13 UTC (permalink / raw)
To: Huang Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Baolin Wang, Zi Yan, Yang Shi
On Mon, Jul 11, 2022 at 04:49:45PM +0800, Huang Ying wrote:
> If THP is failed to be migrated for -ENOSYS and -ENOMEM, the THP will
> be split into thp_split_pages, and after other pages are migrated,
> pages in thp_split_pages will be migrated with no_subpage_counting ==
> true, because its failure have been counted already. If some pages in
> thp_split_pages are retried during migration, we should not count
> their failure if no_subpage_counting == true too. This is done this
> patch to fix the failure counting for THP subpages retrying.
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Fixes: 5984fabb6e82 ("mm: move_pages: report the number of non-attempted pages")
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Yang Shi <shy828301@gmail.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
--
Oscar Salvador
SUSE Labs
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH -V2 5/7] migrate_pages(): fix failure counting for THP on -ENOSYS
2022-07-11 8:49 [PATCH -V2 0/7] migrate_pages(): fix several bugs in error path Huang Ying
` (3 preceding siblings ...)
2022-07-11 8:49 ` [PATCH -V2 4/7] migrate_pages(): fix failure counting for THP subpages retrying Huang Ying
@ 2022-07-11 8:49 ` Huang Ying
2022-07-11 12:26 ` Oscar Salvador
2022-07-11 8:49 ` [PATCH -V2 6/7] migrate_pages(): fix failure counting for THP splitting Huang Ying
2022-07-11 8:49 ` [PATCH -V2 7/7] migrate_pages(): fix failure counting for retry Huang Ying
6 siblings, 1 reply; 17+ messages in thread
From: Huang Ying @ 2022-07-11 8:49 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Huang Ying, Baolin Wang, Zi Yan, Yang Shi
If THP or hugetlbfs page migration isn't supported, unmap_and_move()
or unmap_and_move_huge_page() will return -ENOSYS. For THP, splitting
will be tried, but if splitting doesn't succeed, the THP will be left
in "from" list wrongly. If some other pages are retried, the THP
migration failure will counted again. This is fixed via moving the
failure THP from "from" to "ret_pages".
Another issue of the original code is that the unsupported failure
processing isn't consistent between THP and hugetlbfs page. Make them
consistent in this patch to make the code easier to be understood too.
Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Fixes: 5984fabb6e82 ("mm: move_pages: report the number of non-attempted pages")
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Zi Yan <ziy@nvidia.com>
Cc: Yang Shi <shy828301@gmail.com>
---
mm/migrate.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 4bceba143db0..8cce73b7c046 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1192,10 +1192,8 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
* tables or check whether the hugepage is pmd-based or not before
* kicking migration.
*/
- if (!hugepage_migration_supported(page_hstate(hpage))) {
- list_move_tail(&hpage->lru, ret);
+ if (!hugepage_migration_supported(page_hstate(hpage)))
return -ENOSYS;
- }
if (page_count(hpage) == 1) {
/* page was freed from under us. So we are done. */
@@ -1392,6 +1390,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
* page will be put back
* -EAGAIN: stay on the from list
* -ENOMEM: stay on the from list
+ * -ENOSYS: stay on the from list
* Other errno: put on ret_pages list then splice to
* from list
*/
@@ -1421,6 +1420,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
}
nr_failed_pages += nr_subpages;
+ list_move_tail(&page->lru, &ret_pages);
break;
case -ENOMEM:
/*
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH -V2 5/7] migrate_pages(): fix failure counting for THP on -ENOSYS
2022-07-11 8:49 ` [PATCH -V2 5/7] migrate_pages(): fix failure counting for THP on -ENOSYS Huang Ying
@ 2022-07-11 12:26 ` Oscar Salvador
2022-07-13 8:30 ` Huang, Ying
0 siblings, 1 reply; 17+ messages in thread
From: Oscar Salvador @ 2022-07-11 12:26 UTC (permalink / raw)
To: Huang Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Baolin Wang, Zi Yan, Yang Shi
On Mon, Jul 11, 2022 at 04:49:46PM +0800, Huang Ying wrote:
> If THP or hugetlbfs page migration isn't supported, unmap_and_move()
> or unmap_and_move_huge_page() will return -ENOSYS. For THP, splitting
> will be tried, but if splitting doesn't succeed, the THP will be left
> in "from" list wrongly. If some other pages are retried, the THP
> migration failure will counted again. This is fixed via moving the
> failure THP from "from" to "ret_pages".
>
> Another issue of the original code is that the unsupported failure
> processing isn't consistent between THP and hugetlbfs page. Make them
> consistent in this patch to make the code easier to be understood too.
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Fixes: 5984fabb6e82 ("mm: move_pages: report the number of non-attempted pages")
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Yang Shi <shy828301@gmail.com>
> ---
> mm/migrate.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 4bceba143db0..8cce73b7c046 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1192,10 +1192,8 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
> * tables or check whether the hugepage is pmd-based or not before
> * kicking migration.
> */
> - if (!hugepage_migration_supported(page_hstate(hpage))) {
> - list_move_tail(&hpage->lru, ret);
> + if (!hugepage_migration_supported(page_hstate(hpage)))
> return -ENOSYS;
> - }
>
> if (page_count(hpage) == 1) {
> /* page was freed from under us. So we are done. */
> @@ -1392,6 +1390,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> * page will be put back
> * -EAGAIN: stay on the from list
> * -ENOMEM: stay on the from list
> + * -ENOSYS: stay on the from list
> * Other errno: put on ret_pages list then splice to
> * from list
> */
> @@ -1421,6 +1420,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> }
>
> nr_failed_pages += nr_subpages;
> + list_move_tail(&page->lru, &ret_pages);
I must be missing something, but migrate_pages() calls unmap_and_move_huge_page()
with ret being ret_pages, so
list_move_tail(&hpage->lru, ret) == list_move_tail(&page->lru, &ret_pages)
Yet, you say "This is fixed via moving the failure THP from "from" to "ret_pages"".
/me confused.
--
Oscar Salvador
SUSE Labs
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH -V2 5/7] migrate_pages(): fix failure counting for THP on -ENOSYS
2022-07-11 12:26 ` Oscar Salvador
@ 2022-07-13 8:30 ` Huang, Ying
0 siblings, 0 replies; 17+ messages in thread
From: Huang, Ying @ 2022-07-13 8:30 UTC (permalink / raw)
To: Oscar Salvador
Cc: Andrew Morton, linux-mm, linux-kernel, Baolin Wang, Zi Yan, Yang Shi
Oscar Salvador <osalvador@suse.de> writes:
> On Mon, Jul 11, 2022 at 04:49:46PM +0800, Huang Ying wrote:
>> If THP or hugetlbfs page migration isn't supported, unmap_and_move()
>> or unmap_and_move_huge_page() will return -ENOSYS. For THP, splitting
>> will be tried, but if splitting doesn't succeed, the THP will be left
>> in "from" list wrongly. If some other pages are retried, the THP
>> migration failure will counted again. This is fixed via moving the
>> failure THP from "from" to "ret_pages".
>>
>> Another issue of the original code is that the unsupported failure
>> processing isn't consistent between THP and hugetlbfs page. Make them
>> consistent in this patch to make the code easier to be understood too.
>>
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> Fixes: 5984fabb6e82 ("mm: move_pages: report the number of non-attempted pages")
>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> Cc: Zi Yan <ziy@nvidia.com>
>> Cc: Yang Shi <shy828301@gmail.com>
>> ---
>> mm/migrate.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index 4bceba143db0..8cce73b7c046 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1192,10 +1192,8 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
>> * tables or check whether the hugepage is pmd-based or not before
>> * kicking migration.
>> */
>> - if (!hugepage_migration_supported(page_hstate(hpage))) {
>> - list_move_tail(&hpage->lru, ret);
>> + if (!hugepage_migration_supported(page_hstate(hpage)))
>> return -ENOSYS;
>> - }
>>
>> if (page_count(hpage) == 1) {
>> /* page was freed from under us. So we are done. */
>> @@ -1392,6 +1390,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>> * page will be put back
>> * -EAGAIN: stay on the from list
>> * -ENOMEM: stay on the from list
>> + * -ENOSYS: stay on the from list
>> * Other errno: put on ret_pages list then splice to
>> * from list
>> */
>> @@ -1421,6 +1420,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>> }
>>
>> nr_failed_pages += nr_subpages;
>> + list_move_tail(&page->lru, &ret_pages);
>
> I must be missing something, but migrate_pages() calls unmap_and_move_huge_page()
> with ret being ret_pages, so
>
> list_move_tail(&hpage->lru, ret) == list_move_tail(&page->lru, &ret_pages)
>
> Yet, you say "This is fixed via moving the failure THP from "from" to "ret_pages"".
> /me confused.
To make it consistent between hugetlb page and THP/normal page, I have
revised the unmap_and_move_huge_page() via deleting the list_move_tail()
there for ENOSYS. After that, we move the hugetlb page and THP/normal
page in the same way in migrate_pages().
Best Regards,
Huang, Ying
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH -V2 6/7] migrate_pages(): fix failure counting for THP splitting
2022-07-11 8:49 [PATCH -V2 0/7] migrate_pages(): fix several bugs in error path Huang Ying
` (4 preceding siblings ...)
2022-07-11 8:49 ` [PATCH -V2 5/7] migrate_pages(): fix failure counting for THP on -ENOSYS Huang Ying
@ 2022-07-11 8:49 ` Huang Ying
2022-07-11 12:39 ` Oscar Salvador
2022-07-11 8:49 ` [PATCH -V2 7/7] migrate_pages(): fix failure counting for retry Huang Ying
6 siblings, 1 reply; 17+ messages in thread
From: Huang Ying @ 2022-07-11 8:49 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Huang Ying, Baolin Wang, Zi Yan, Yang Shi
If THP is failed to be migrated, it may be split and retry. But after
splitting, the head page will be left in "from" list, although THP
migration failure has been counted already. If the head page is
failed to be migrated too, the failure will be counted twice
incorrectly. So this is fixed in this patch via moving the head page
of THP after splitting to "thp_split_pages" too.
Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Fixes: 5984fabb6e82 ("mm: move_pages: report the number of non-attempted pages")
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Zi Yan <ziy@nvidia.com>
Cc: Yang Shi <shy828301@gmail.com>
---
mm/migrate.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 8cce73b7c046..557708ce13a1 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1306,6 +1306,8 @@ static inline int try_split_thp(struct page *page, struct list_head *split_pages
lock_page(page);
rc = split_huge_page_to_list(page, split_pages);
unlock_page(page);
+ if (!rc)
+ list_move_tail(&page->lru, split_pages);
return rc;
}
@@ -1365,7 +1367,6 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
thp_retry = 0;
list_for_each_entry_safe(page, page2, from, lru) {
-retry:
/*
* THP statistics is based on the source huge page.
* Capture required information that might get lost
@@ -1412,7 +1413,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
nr_thp_failed++;
if (!try_split_thp(page, &thp_split_pages)) {
nr_thp_split++;
- goto retry;
+ break;
}
/* Hugetlb migration is unsupported */
} else if (!no_subpage_counting) {
@@ -1432,7 +1433,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
/* THP NUMA faulting doesn't split THP to retry. */
if (!nosplit && !try_split_thp(page, &thp_split_pages)) {
nr_thp_split++;
- goto retry;
+ break;
}
} else if (!no_subpage_counting) {
nr_failed++;
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH -V2 6/7] migrate_pages(): fix failure counting for THP splitting
2022-07-11 8:49 ` [PATCH -V2 6/7] migrate_pages(): fix failure counting for THP splitting Huang Ying
@ 2022-07-11 12:39 ` Oscar Salvador
0 siblings, 0 replies; 17+ messages in thread
From: Oscar Salvador @ 2022-07-11 12:39 UTC (permalink / raw)
To: Huang Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Baolin Wang, Zi Yan, Yang Shi
On Mon, Jul 11, 2022 at 04:49:47PM +0800, Huang Ying wrote:
> If THP is failed to be migrated, it may be split and retry. But after
> splitting, the head page will be left in "from" list, although THP
> migration failure has been counted already. If the head page is
> failed to be migrated too, the failure will be counted twice
> incorrectly. So this is fixed in this patch via moving the head page
> of THP after splitting to "thp_split_pages" too.
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Fixes: 5984fabb6e82 ("mm: move_pages: report the number of non-attempted pages")
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Yang Shi <shy828301@gmail.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
> ---
> mm/migrate.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 8cce73b7c046..557708ce13a1 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1306,6 +1306,8 @@ static inline int try_split_thp(struct page *page, struct list_head *split_pages
> lock_page(page);
> rc = split_huge_page_to_list(page, split_pages);
> unlock_page(page);
> + if (!rc)
> + list_move_tail(&page->lru, split_pages);
>
> return rc;
> }
> @@ -1365,7 +1367,6 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> thp_retry = 0;
>
> list_for_each_entry_safe(page, page2, from, lru) {
> -retry:
> /*
> * THP statistics is based on the source huge page.
> * Capture required information that might get lost
> @@ -1412,7 +1413,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> nr_thp_failed++;
> if (!try_split_thp(page, &thp_split_pages)) {
> nr_thp_split++;
> - goto retry;
> + break;
> }
> /* Hugetlb migration is unsupported */
> } else if (!no_subpage_counting) {
> @@ -1432,7 +1433,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> /* THP NUMA faulting doesn't split THP to retry. */
> if (!nosplit && !try_split_thp(page, &thp_split_pages)) {
> nr_thp_split++;
> - goto retry;
> + break;
> }
> } else if (!no_subpage_counting) {
> nr_failed++;
> --
> 2.30.2
>
>
--
Oscar Salvador
SUSE Labs
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH -V2 7/7] migrate_pages(): fix failure counting for retry
2022-07-11 8:49 [PATCH -V2 0/7] migrate_pages(): fix several bugs in error path Huang Ying
` (5 preceding siblings ...)
2022-07-11 8:49 ` [PATCH -V2 6/7] migrate_pages(): fix failure counting for THP splitting Huang Ying
@ 2022-07-11 8:49 ` Huang Ying
2022-07-11 12:52 ` Oscar Salvador
6 siblings, 1 reply; 17+ messages in thread
From: Huang Ying @ 2022-07-11 8:49 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, linux-kernel, Huang Ying, Baolin Wang, Zi Yan, Yang Shi
After 10 retries, we will give up and the remaining pages will be
counted as failure in nr_failed and nr_thp_failed. We should count
the failure in nr_failed_pages too. This is done in this patch.
Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Fixes: 5984fabb6e82 ("mm: move_pages: report the number of non-attempted pages")
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Zi Yan <ziy@nvidia.com>
Cc: Yang Shi <shy828301@gmail.com>
---
mm/migrate.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 557708ce13a1..cee6fc5a2d31 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1345,6 +1345,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
int thp_retry = 1;
int nr_failed = 0;
int nr_failed_pages = 0;
+ int nr_retry_pages = 0;
int nr_succeeded = 0;
int nr_thp_succeeded = 0;
int nr_thp_failed = 0;
@@ -1365,6 +1366,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
retry = 0;
thp_retry = 0;
+ nr_retry_pages = 0;
list_for_each_entry_safe(page, page2, from, lru) {
/*
@@ -1439,7 +1441,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
nr_failed++;
}
- nr_failed_pages += nr_subpages;
+ nr_failed_pages += nr_subpages + nr_retry_pages;
/*
* There might be some subpages of fail-to-migrate THPs
* left in thp_split_pages list. Move them back to migration
@@ -1455,6 +1457,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
thp_retry++;
else
retry++;
+ nr_retry_pages += nr_subpages;
break;
case MIGRATEPAGE_SUCCESS:
nr_succeeded += nr_subpages;
@@ -1481,6 +1484,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
if (!no_subpage_counting)
nr_failed += retry;
nr_thp_failed += thp_retry;
+ nr_failed_pages += nr_retry_pages;
/*
* Try to migrate subpages of fail-to-migrate THPs, no nr_failed
* counting in this round, since all subpages of a THP is counted
--
2.30.2
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH -V2 7/7] migrate_pages(): fix failure counting for retry
2022-07-11 8:49 ` [PATCH -V2 7/7] migrate_pages(): fix failure counting for retry Huang Ying
@ 2022-07-11 12:52 ` Oscar Salvador
0 siblings, 0 replies; 17+ messages in thread
From: Oscar Salvador @ 2022-07-11 12:52 UTC (permalink / raw)
To: Huang Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Baolin Wang, Zi Yan, Yang Shi
On Mon, Jul 11, 2022 at 04:49:48PM +0800, Huang Ying wrote:
> After 10 retries, we will give up and the remaining pages will be
> counted as failure in nr_failed and nr_thp_failed. We should count
> the failure in nr_failed_pages too. This is done in this patch.
>
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Fixes: 5984fabb6e82 ("mm: move_pages: report the number of non-attempted pages")
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Yang Shi <shy828301@gmail.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
> ---
> mm/migrate.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 557708ce13a1..cee6fc5a2d31 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1345,6 +1345,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> int thp_retry = 1;
> int nr_failed = 0;
> int nr_failed_pages = 0;
> + int nr_retry_pages = 0;
> int nr_succeeded = 0;
> int nr_thp_succeeded = 0;
> int nr_thp_failed = 0;
> @@ -1365,6 +1366,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> for (pass = 0; pass < 10 && (retry || thp_retry); pass++) {
> retry = 0;
> thp_retry = 0;
> + nr_retry_pages = 0;
>
> list_for_each_entry_safe(page, page2, from, lru) {
> /*
> @@ -1439,7 +1441,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> nr_failed++;
> }
>
> - nr_failed_pages += nr_subpages;
> + nr_failed_pages += nr_subpages + nr_retry_pages;
> /*
> * There might be some subpages of fail-to-migrate THPs
> * left in thp_split_pages list. Move them back to migration
> @@ -1455,6 +1457,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> thp_retry++;
> else
> retry++;
> + nr_retry_pages += nr_subpages;
> break;
> case MIGRATEPAGE_SUCCESS:
> nr_succeeded += nr_subpages;
> @@ -1481,6 +1484,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> if (!no_subpage_counting)
> nr_failed += retry;
> nr_thp_failed += thp_retry;
> + nr_failed_pages += nr_retry_pages;
> /*
> * Try to migrate subpages of fail-to-migrate THPs, no nr_failed
> * counting in this round, since all subpages of a THP is counted
> --
> 2.30.2
>
>
--
Oscar Salvador
SUSE Labs
^ permalink raw reply [flat|nested] 17+ messages in thread