* Re: [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node
2019-12-05 4:21 [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node Yang Shi
@ 2019-12-05 5:44 ` John Hubbard
2019-12-05 5:50 ` John Hubbard
2019-12-05 17:20 ` Yang Shi
2019-12-05 9:42 ` Qian Cai
2019-12-05 11:31 ` Michal Hocko
2 siblings, 2 replies; 10+ messages in thread
From: John Hubbard @ 2019-12-05 5:44 UTC (permalink / raw)
To: Yang Shi, fabecassis, mhocko, cl, vbabka, mgorman, akpm
Cc: linux-mm, linux-kernel, stable
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@nvidia.com>
...with a few nitpicky notes about comments, below, that might help:
>
> 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@nvidia.com>
> Tested-by: Felix Abecassis <fabecassis@nvidia.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: <stable@vger.kernel.org> 4.17+
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> ---
> 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,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node
2019-12-05 5:44 ` John Hubbard
@ 2019-12-05 5:50 ` John Hubbard
2019-12-05 17:20 ` Yang Shi
1 sibling, 0 replies; 10+ messages in thread
From: John Hubbard @ 2019-12-05 5:50 UTC (permalink / raw)
To: Yang Shi, fabecassis, mhocko, cl, vbabka, mgorman, akpm
Cc: linux-mm, linux-kernel, stable
On 12/4/19 9:44 PM, John Hubbard wrote:
... */
>
> 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:
>
Correction, I experienced a fatal editor copy-paste mistake here. :) I meant to
suggest this:
/*
* add_page_for_migration() experienced a fatal failure (see the
* comments in that routine for details).
*/
>
>
>> + if (err > 0) {
>> + err = -EFAULT;
>> + goto out_flush;
>> + }
>> +
>> err = store_status(status, i, err, 1);
>> if (err)
>> goto out_flush;
>>
thanks,
--
John Hubbard
NVIDIA
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node
2019-12-05 5:44 ` John Hubbard
2019-12-05 5:50 ` John Hubbard
@ 2019-12-05 17:20 ` Yang Shi
1 sibling, 0 replies; 10+ messages in thread
From: Yang Shi @ 2019-12-05 17:20 UTC (permalink / raw)
To: John Hubbard, fabecassis, mhocko, cl, vbabka, mgorman, akpm
Cc: linux-mm, linux-kernel, stable
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@nvidia.com>
>
> ...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@nvidia.com>
>> Tested-by: Felix Abecassis <fabecassis@nvidia.com>
>> Cc: John Hubbard <jhubbard@nvidia.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Christoph Lameter <cl@linux.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Mel Gorman <mgorman@techsingularity.net>
>> Cc: <stable@vger.kernel.org> 4.17+
>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>> ---
>> 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,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node
2019-12-05 4:21 [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node Yang Shi
2019-12-05 5:44 ` John Hubbard
@ 2019-12-05 9:42 ` Qian Cai
2019-12-05 17:39 ` Yang Shi
2019-12-05 11:31 ` Michal Hocko
2 siblings, 1 reply; 10+ messages in thread
From: Qian Cai @ 2019-12-05 9:42 UTC (permalink / raw)
To: Yang Shi
Cc: fabecassis, jhubbard, mhocko, cl, vbabka, mgorman, akpm,
linux-mm, linux-kernel, stable
> On Dec 4, 2019, at 11:21 PM, Yang Shi <yang.shi@linux.alibaba.com> 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<---
>
> 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.
Don’t really feel it is a bug after all. As you mentioned, the manpage was rather poorly written. Why it is not a good idea just update the manpage or/and code comments instead to document the current behavior?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node
2019-12-05 9:42 ` Qian Cai
@ 2019-12-05 17:39 ` Yang Shi
2019-12-05 18:11 ` Qian Cai
0 siblings, 1 reply; 10+ messages in thread
From: Yang Shi @ 2019-12-05 17:39 UTC (permalink / raw)
To: Qian Cai
Cc: fabecassis, jhubbard, mhocko, cl, vbabka, mgorman, akpm,
linux-mm, linux-kernel, stable
On 12/5/19 1:42 AM, Qian Cai wrote:
>
>> On Dec 4, 2019, at 11:21 PM, Yang Shi <yang.shi@linux.alibaba.com> 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<---
>>
>> 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.
> Don’t really feel it is a bug after all. As you mentioned, the manpage was rather poorly written. Why it is not a good idea just update the manpage or/and code comments instead to document the current behavior?
There are definitely a few inconsistencies, but I think the manpage is
quite clear for this specific case, which says "status is an array of
integers that return the status of each page. The array contains valid
values only if move_pages() did not return an error." And, it looks
kernel just misbehaved since 4.17 due to the fixes commit, so it sounds
like a regression too.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node
2019-12-05 17:39 ` Yang Shi
@ 2019-12-05 18:11 ` Qian Cai
2019-12-05 18:17 ` Christopher Lameter
0 siblings, 1 reply; 10+ messages in thread
From: Qian Cai @ 2019-12-05 18:11 UTC (permalink / raw)
To: Yang Shi
Cc: fabecassis, jhubbard, mhocko, cl, vbabka, mgorman, akpm,
linux-mm, linux-kernel, stable
> On Dec 5, 2019, at 12:39 PM, Yang Shi <yang.shi@linux.alibaba.com> wrote:
>
> There are definitely a few inconsistencies, but I think the manpage is quite clear for this specific case, which says "status is an array of integers that return the status of each page. The array contains valid values only if move_pages() did not return an error." And, it looks kernel just misbehaved since 4.17 due to the fixes commit, so it sounds like a regression too.
“Only if” in strictly logical term does not necessarily mean it must contain valid values if move_pages() succeed.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node
2019-12-05 18:11 ` Qian Cai
@ 2019-12-05 18:17 ` Christopher Lameter
0 siblings, 0 replies; 10+ messages in thread
From: Christopher Lameter @ 2019-12-05 18:17 UTC (permalink / raw)
To: Qian Cai
Cc: Yang Shi, fabecassis, jhubbard, mhocko, vbabka, mgorman, akpm,
linux-mm, linux-kernel, stable
[-- Attachment #1: Type: text/plain, Size: 870 bytes --]
On Thu, 5 Dec 2019, Qian Cai wrote:
> > On Dec 5, 2019, at 12:39 PM, Yang Shi <yang.shi@linux.alibaba.com> wrote:
> >
> > There are definitely a few inconsistencies, but I think the manpage is quite clear for this specific case, which says "status is an array of integers that return the status of each page. The array contains valid values only if move_pages() did not return an error." And, it looks kernel just misbehaved since 4.17 due to the fixes commit, so it sounds like a regression too.
>
> “Only if” in strictly logical term does not necessarily mean it must contain valid values if move_pages() succeed.
The intended meaning is that valid values in the status array are provided
when the syscall succeeds and not otherwise. In the error case there may
be some valid values but since the operation may have aborted not all
values may have a value.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node
2019-12-05 4:21 [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node Yang Shi
2019-12-05 5:44 ` John Hubbard
2019-12-05 9:42 ` Qian Cai
@ 2019-12-05 11:31 ` Michal Hocko
2019-12-05 17:18 ` Yang Shi
2 siblings, 1 reply; 10+ messages in thread
From: Michal Hocko @ 2019-12-05 11:31 UTC (permalink / raw)
To: Yang Shi
Cc: fabecassis, jhubbard, cl, vbabka, mgorman, akpm, linux-mm,
linux-kernel, stable
On Thu 05-12-19 12:21:18, 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<---
>
> 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.
The code is indeed more complex than I wanted but I couldn't figure an
easier way back then. I wanted to keep store_status at a single place
because the failure handling is quite complex already.
> 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.
Can we simply return 1 when there is something to migrate instead?
Something like
diff --git a/mm/migrate.c b/mm/migrate.c
index 4fe45d1428c8..f3730804b8d4 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1516,9 +1516,9 @@ 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 or 0 when it doesn't have
+ * to be migrate or 1 dwhen it has been queued or the page doesn't need to be
+ * migrated because it is already on the target node
*/
static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
int node, struct list_head *pagelist, bool migrate_all)
@@ -1557,7 +1557,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;
@@ -1567,7 +1567,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),
@@ -1644,8 +1644,14 @@ 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) {
+ err = store_status(status, i, current_node, 1);
+ if (err)
+ goto out_flush;
+ continue;
+ } else if (err > 0) {
continue;
+ }
err = store_status(status, i, err, 1);
if (err)
this would still keep store_status ugliness at a single place.
>
> Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move")
> Reported-by: Felix Abecassis <fabecassis@nvidia.com>
> Tested-by: Felix Abecassis <fabecassis@nvidia.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: <stable@vger.kernel.org> 4.17+
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> ---
> 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.
I do not remember all the details but my recollection is that there were
several inconsistencies present before I touched the code and I've
decided to not touch them without a clear usecase.
--
Michal Hocko
SUSE Labs
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [v2 PATCH] mm: move_pages: return valid node id in status if the page is already on the target node
2019-12-05 11:31 ` Michal Hocko
@ 2019-12-05 17:18 ` Yang Shi
0 siblings, 0 replies; 10+ messages in thread
From: Yang Shi @ 2019-12-05 17:18 UTC (permalink / raw)
To: Michal Hocko
Cc: fabecassis, jhubbard, cl, vbabka, mgorman, akpm, linux-mm,
linux-kernel, stable
On 12/5/19 3:31 AM, Michal Hocko wrote:
> On Thu 05-12-19 12:21:18, 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<---
>>
>> 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.
> The code is indeed more complex than I wanted but I couldn't figure an
> easier way back then. I wanted to keep store_status at a single place
> because the failure handling is quite complex already.
>
>> 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.
> Can we simply return 1 when there is something to migrate instead?
> Something like
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 4fe45d1428c8..f3730804b8d4 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1516,9 +1516,9 @@ 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 or 0 when it doesn't have
> + * to be migrate or 1 dwhen it has been queued or the page doesn't need to be
> + * migrated because it is already on the target node
> */
> static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
> int node, struct list_head *pagelist, bool migrate_all)
> @@ -1557,7 +1557,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;
> @@ -1567,7 +1567,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),
> @@ -1644,8 +1644,14 @@ 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) {
> + err = store_status(status, i, current_node, 1);
> + if (err)
> + goto out_flush;
> + continue;
> + } else if (err > 0) {
> continue;
> + }
>
> err = store_status(status, i, err, 1);
> if (err)
>
> this would still keep store_status ugliness at a single place.
Thanks for the suggestion, it looks neater, will do in v2.
>
>> Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move")
>> Reported-by: Felix Abecassis <fabecassis@nvidia.com>
>> Tested-by: Felix Abecassis <fabecassis@nvidia.com>
>> Cc: John Hubbard <jhubbard@nvidia.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Christoph Lameter <cl@linux.com>
>> Cc: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Mel Gorman <mgorman@techsingularity.net>
>> Cc: <stable@vger.kernel.org> 4.17+
>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>> ---
>> 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.
> I do not remember all the details but my recollection is that there were
> several inconsistencies present before I touched the code and I've
> decided to not touch them without a clear usecase.
I agree. It looks nobody complained those inconsistency, and I didn't
see LTP covers those.
^ permalink raw reply [flat|nested] 10+ messages in thread