Re: [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node

From: Yang Shi
Date: Thu Dec 05 2019 - 12:20:26 EST



On 12/4/19 9:44 PM, John Hubbard wrote:
On 12/4/19 8:21 PM, Yang Shi wrote:
Felix Abecassis reports move_pages() would return random status if the
pages are already on the target node by the below test program:

---8<---

This is correct correct code, so:

Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>

...with a few nitpicky notes about comments, below, that might help:

Thanks, John. Will take in new version.



int main(void)
{
ÂÂÂÂconst long node_id = 1;
ÂÂÂÂconst long page_size = sysconf(_SC_PAGESIZE);
ÂÂÂÂconst int64_t num_pages = 8;

ÂÂÂÂunsigned long nodemask =Â 1 << node_id;
ÂÂÂÂlong ret = set_mempolicy(MPOL_BIND, &nodemask, sizeof(nodemask));
ÂÂÂÂif (ret < 0)
ÂÂÂÂÂÂÂ return (EXIT_FAILURE);

ÂÂÂÂvoid **pages = malloc(sizeof(void*) * num_pages);
ÂÂÂÂfor (int i = 0; i < num_pages; ++i) {
ÂÂÂÂÂÂÂ pages[i] = mmap(NULL, page_size, PROT_WRITE | PROT_READ,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ MAP_PRIVATE | MAP_POPULATE | MAP_ANONYMOUS,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ -1, 0);
ÂÂÂÂÂÂÂ if (pages[i] == MAP_FAILED)
ÂÂÂÂÂÂÂÂÂÂÂ return (EXIT_FAILURE);
ÂÂÂÂ}

ÂÂÂÂret = set_mempolicy(MPOL_DEFAULT, NULL, 0);
ÂÂÂÂif (ret < 0)
ÂÂÂÂÂÂÂ return (EXIT_FAILURE);

ÂÂÂÂint *nodes = malloc(sizeof(int) * num_pages);
ÂÂÂÂint *status = malloc(sizeof(int) * num_pages);
ÂÂÂÂfor (int i = 0; i < num_pages; ++i) {
ÂÂÂÂÂÂÂ nodes[i] = node_id;
ÂÂÂÂÂÂÂ status[i] = 0xd0; /* simulate garbage values */
ÂÂÂÂ}

ÂÂÂÂret = move_pages(0, num_pages, pages, nodes, status, MPOL_MF_MOVE);
ÂÂÂÂprintf("move_pages: %ld\n", ret);
ÂÂÂÂfor (int i = 0; i < num_pages; ++i)
ÂÂÂÂÂÂÂ printf("status[%d] = %d\n", i, status[i]);
}
---8<---

Then running the program would return nonsense status values:
$ ./move_pages_bug
move_pages: 0
status[0] = 208
status[1] = 208
status[2] = 208
status[3] = 208
status[4] = 208
status[5] = 208
status[6] = 208
status[7] = 208

This is because the status is not set if the page is already on the
target node, but move_pages() should return valid status as long as it
succeeds. The valid status may be errno or node id.

We can't simply initialize status array to zero since the pages may be
not on node 0. Fix it by updating status with node id which the page is
already on. And, it looks we have to update the status inside
add_page_for_migration() since the page struct is not available outside
it.

Make add_page_for_migration() return 1 if store_status() is failed in
order to not mix up the status value since -EFAULT is also a valid
status.

Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move")
Reported-by: Felix Abecassis <fabecassis@xxxxxxxxxx>
Tested-by: Felix Abecassis <fabecassis@xxxxxxxxxx>
Cc: John Hubbard <jhubbard@xxxxxxxxxx>
Cc: Michal Hocko <mhocko@xxxxxxxx>
Cc: Christoph Lameter <cl@xxxxxxxxx>
Cc: Vlastimil Babka <vbabka@xxxxxxx>
Cc: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx> 4.17+
Signed-off-by: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
---
v2: *Correted the return value when add_page_for_migration() returns 1.

John noticed another return value inconsistency between the implementation and
the manpage. The manpage says it should return -ENOENT if the page is already
on the target node, but it doesn't. It looks the original code didn't return
-ENOENT either, I'm not sure if this is a document issue or not. Anyway this
is another issue, once we confirm it we can fix it later.

 mm/migrate.c | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index a8f87cb..f1090a0 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1512,17 +1512,21 @@ static int do_move_pages_to_node(struct mm_struct *mm,
 /*
ÂÂ * Resolves the given address to a struct page, isolates it from the LRU and
ÂÂ * puts it to the given pagelist.
- * Returns -errno if the page cannot be found/isolated or 0 when it has been
- * queued or the page doesn't need to be migrated because it is already on
- * the target node
+ * Returns:
+ *ÂÂÂÂ errno - if the page cannot be found/isolated
+ *ÂÂÂÂ 0 - when it has been queued or the page doesn't need to be migrated
+ *ÂÂÂÂÂÂÂÂ because it is already on the target node
+ *ÂÂÂÂ 1 - if store_status() is failed


I recommend this wording instead:

Â* Returns:
Â*ÂÂÂÂ errno - if the page cannot be found/isolated
Â*ÂÂÂÂ 0 - when it has been queued or the page doesn't need to be migrated
Â*ÂÂÂÂÂÂÂÂ because it is already on the target node
Â*ÂÂÂÂ 1 - The page doesn't need to be migrated because it is already on the
Â*ÂÂÂÂÂÂÂÂ target node. However, attempting to store the node ID in the status
Â*ÂÂÂÂÂÂÂÂ array failed. Unlike other failures in this function, this case
Â*ÂÂÂÂÂÂÂÂ needs to turn into a fatal failure in the calling function.


ÂÂ */
 static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
-ÂÂÂÂÂÂÂ int node, struct list_head *pagelist, bool migrate_all)
+ÂÂÂÂÂÂÂ int node, struct list_head *pagelist, bool migrate_all,
+ÂÂÂÂÂÂÂ int __user *status, int start)
 {
ÂÂÂÂÂ struct vm_area_struct *vma;
ÂÂÂÂÂ struct page *page;
ÂÂÂÂÂ unsigned int follflags;
ÂÂÂÂÂ int err;
+ÂÂÂ bool same_node = false;
 Â down_read(&mm->mmap_sem);
ÂÂÂÂÂ err = -EFAULT;
@@ -1543,8 +1547,10 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
ÂÂÂÂÂÂÂÂÂ goto out;
 Â err = 0;
-ÂÂÂ if (page_to_nid(page) == node)
+ÂÂÂ if (page_to_nid(page) == node) {
+ÂÂÂÂÂÂÂ same_node = true;
ÂÂÂÂÂÂÂÂÂ goto out_putpage;
+ÂÂÂ }
 Â err = -EACCES;
ÂÂÂÂÂ if (page_mapcount(page) > 1 && !migrate_all)
@@ -1578,6 +1584,16 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
ÂÂÂÂÂ put_page(page);
 out:
ÂÂÂÂÂ up_read(&mm->mmap_sem);
+
+ÂÂÂ /*
+ÂÂÂÂ * Must call store_status() after releasing mmap_sem since put_user
+ÂÂÂÂ * need acquire mmap_sem too, otherwise potential deadlock may exist.
+ÂÂÂÂ */
+ÂÂÂ if (same_node) {
+ÂÂÂÂÂÂÂ if (store_status(status, start, node, 1))
+ÂÂÂÂÂÂÂÂÂÂÂ err = 1;
+ÂÂÂ }
+
ÂÂÂÂÂ return err;
 }
 @@ -1639,10 +1655,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
ÂÂÂÂÂÂÂÂÂÂ * report them via status
ÂÂÂÂÂÂÂÂÂÂ */

Let's change the comment above add_page_for_migration(), to read:

ÂÂÂÂÂÂÂ /*
ÂÂÂÂÂÂÂÂ * Most errors in the page lookup or isolation are not fatal
ÂÂÂÂÂÂÂÂ * and we simply report them via the status array. However,
ÂÂÂÂÂÂÂÂ * positive error values are fatal.
ÂÂÂÂÂÂÂÂ */


ÂÂÂÂÂÂÂÂÂ err = add_page_for_migration(mm, addr, current_node,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &pagelist, flags & MPOL_MF_MOVE_ALL);
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &pagelist, flags & MPOL_MF_MOVE_ALL, status,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ i);
+
ÂÂÂÂÂÂÂÂÂ if (!err)
ÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
 + /* store_status() failed in add_page_for_migration() */

...and let's replace the above line, with the following:

ÂÂÂÂÂÂÂ /*
ÂÂÂÂÂÂÂÂ * Most errors in the page lookup or isolation are not fatal
ÂÂÂÂÂÂÂÂ * and we simply report them via the status array. However,
ÂÂÂÂÂÂÂÂ * positive error values are fatal.
ÂÂÂÂÂÂÂÂ */


+ÂÂÂÂÂÂÂ if (err > 0) {
+ÂÂÂÂÂÂÂÂÂÂÂ err = -EFAULT;
+ÂÂÂÂÂÂÂÂÂÂÂ goto out_flush;
+ÂÂÂÂÂÂÂ }
+
ÂÂÂÂÂÂÂÂÂ err = store_status(status, i, err, 1);
ÂÂÂÂÂÂÂÂÂ if (err)
ÂÂÂÂÂÂÂÂÂÂÂÂÂ goto out_flush;


And with that, I think the comments help a little bit more, in reading
through the code.


thanks,