Date:   Wed, 4 Dec 2019 21:44:22 -0800
From:   John Hubbard <jhubbard@...dia.com>
To:     Yang Shi <yang.shi@...ux.alibaba.com>, <fabecassis@...dia.com>,
        <mhocko@...e.com>, <cl@...ux.com>, <vbabka@...e.cz>,
        <mgorman@...hsingularity.net>, <akpm@...ux-foundation.org>
CC:     <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        <stable@...r.kernel.org>
Subject: Re: [v2 PATCH] mm: move_pages: return valid node id in status if the
 page is already on the target node

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@...dia.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@...dia.com>
> Tested-by: Felix Abecassis <fabecassis@...dia.com>
> Cc: John Hubbard <jhubbard@...dia.com>
> Cc: Michal Hocko <mhocko@...e.com>
> Cc: Christoph Lameter <cl@...ux.com>
> Cc: Vlastimil Babka <vbabka@...e.cz>
> Cc: Mel Gorman <mgorman@...hsingularity.net>
> Cc: <stable@...r.kernel.org> 4.17+
> Signed-off-by: Yang Shi <yang.shi@...ux.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

Powered by blists - more mailing lists