| From e9610671e9d80b78e88c5a1f88975f2baff79d5a Mon Sep 17 00:00:00 2001 |
| From: Yang Shi <yang.shi@linux.alibaba.com> |
| Date: Sat, 4 Jan 2020 12:59:46 -0800 |
| Subject: [PATCH] mm: move_pages: return valid node id in status if the page is |
| already on the target node |
| |
| commit e0153fc2c7606f101392b682e720a7a456d6c766 upstream. |
| |
| Felix Abecassis reports move_pages() would return random status if the |
| pages are already on the target node by the below test program: |
| |
| 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]); |
| } |
| |
| 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. |
| |
| Link: http://lkml.kernel.org/r/1575584353-125392-1-git-send-email-yang.shi@linux.alibaba.com |
| Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") |
| Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> |
| Reported-by: Felix Abecassis <fabecassis@nvidia.com> |
| Tested-by: Felix Abecassis <fabecassis@nvidia.com> |
| Suggested-by: Michal Hocko <mhocko@suse.com> |
| Reviewed-by: John Hubbard <jhubbard@nvidia.com> |
| Acked-by: Christoph Lameter <cl@linux.com> |
| Acked-by: Michal Hocko <mhocko@suse.com> |
| Reviewed-by: Vlastimil Babka <vbabka@suse.cz> |
| Cc: Mel Gorman <mgorman@techsingularity.net> |
| Cc: <stable@vger.kernel.org> [4.17+] |
| Signed-off-by: Andrew Morton <akpm@linux-foundation.org> |
| Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/mm/migrate.c b/mm/migrate.c |
| index dbb3b5bee4ee..6a414bb23cc9 100644 |
| --- a/mm/migrate.c |
| +++ b/mm/migrate.c |
| @@ -1522,9 +1522,11 @@ 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 doesn't have to be migrated because it is already on the |
| + * target node |
| + * 1 - when it has been queued |
| */ |
| static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, |
| int node, struct list_head *pagelist, bool migrate_all) |
| @@ -1563,7 +1565,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, |
| if (PageHuge(page)) { |
| if (PageHead(page)) { |
| isolate_huge_page(page, pagelist); |
| - err = 0; |
| + err = 1; |
| } |
| } else { |
| struct page *head; |
| @@ -1573,7 +1575,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, |
| if (err) |
| goto out_putpage; |
| |
| - err = 0; |
| + err = 1; |
| list_add_tail(&head->lru, pagelist); |
| mod_node_page_state(page_pgdat(head), |
| NR_ISOLATED_ANON + page_is_file_cache(head), |
| @@ -1650,8 +1652,17 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, |
| */ |
| err = add_page_for_migration(mm, addr, current_node, |
| &pagelist, flags & MPOL_MF_MOVE_ALL); |
| - if (!err) |
| + |
| + if (!err) { |
| + /* The page is already on the target node */ |
| + err = store_status(status, i, current_node, 1); |
| + if (err) |
| + goto out_flush; |
| continue; |
| + } else if (err > 0) { |
| + /* The page is successfully queued for migration */ |
| + continue; |
| + } |
| |
| err = store_status(status, i, err, 1); |
| if (err) |
| -- |
| 2.7.4 |
| |