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

Commit Message

Yang Shi Dec. 5, 2019, 6:54 p.m. UTC

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.

Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move")
Reported-by: Felix Abecassis <[e-mail]>
Tested-by: Felix Abecassis <[e-mail]>
Suggested-by: Michal Hocko <[e-mail]>
Cc: John Hubbard <[e-mail]>
Cc: Christoph Lameter <[e-mail]>
Cc: Vlastimil Babka <[e-mail]>
Cc: Mel Gorman <[e-mail]>
Cc: <[e-mail]> 4.17+
Signed-off-by: Yang Shi <[e-mail]>
---
v3: * Adopted the suggestion from Michal.
v2: * Correted the return value when add_page_for_migration() returns 1.

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

Comments

Qian Cai Dec. 5, 2019, 7:19 p.m. UTC | #1

> On Dec 5, 2019, at 1:54 PM, Yang Shi <[e-mail]> wrote:
> 
> 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.

This does not look correct either.

“ENOENT
No pages were found that require moving. All pages are either already on the target node, not present, had an invalid address or could not be moved because they were mapped by multiple processes.”

move_pages() should return -ENOENT instead.

Yang Shi Dec. 5, 2019, 7:27 p.m. UTC | #2

On 12/5/19 11:19 AM, Qian Cai wrote:
>
>> On Dec 5, 2019, at 1:54 PM, Yang Shi <[e-mail]> wrote:
>>
>> 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.
> This does not look correct either.
>
> “ENOENT
> No pages were found that require moving. All pages are either already on the target node, not present, had an invalid address or could not be moved because they were mapped by multiple processes.”
>
> move_pages() should return -ENOENT instead.

Yes, we noticed this too. I had a note in v1 and v2 patch, but I forgot 
paste in v3, says:

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.

And, Michal also commented to the note:

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.

Qian Cai Dec. 5, 2019, 7:34 p.m. UTC | #3

> On Dec 5, 2019, at 2:27 PM, Yang Shi <[e-mail]> wrote:
> 
> 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.

No, I think it is important to figure out this in the first place. Otherwise, it is pointless to touch this piece of code over and over again, i.e., this is not another issue but the core of this problem on hand.

Christoph Lameter (Ampere) Dec. 5, 2019, 7:45 p.m. UTC | #4

On Fri, 6 Dec 2019, 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:

Looks ok.

Acked-by: Christoph Lameter <[e-mail]>

Nitpicks:

> @@ -1553,7 +1555,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;

Add a meaningful constant instead of 1?

>  out:
>  	up_read(&mm->mmap_sem);
> +
>  	return err;

Dont do that.

John Hubbard Dec. 5, 2019, 9:27 p.m. UTC | #5

On 12/5/19 10:54 AM, 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.
> 
> Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move")
> Reported-by: Felix Abecassis <[e-mail]>
> Tested-by: Felix Abecassis <[e-mail]>
> Suggested-by: Michal Hocko <[e-mail]>
> Cc: John Hubbard <[e-mail]>
> Cc: Christoph Lameter <[e-mail]>
> Cc: Vlastimil Babka <[e-mail]>
> Cc: Mel Gorman <[e-mail]>
> Cc: <[e-mail]> 4.17+
> Signed-off-by: Yang Shi <[e-mail]>
> ---
> v3: * Adopted the suggestion from Michal.
> v2: * Correted the return value when add_page_for_migration() returns 1.
> 
>  mm/migrate.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index a8f87cb..9c172f4 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1512,9 +1512,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)
> @@ -1553,7 +1555,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;
> @@ -1563,7 +1565,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),
> @@ -1578,6 +1580,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
>  	put_page(page);
>  out:
>  	up_read(&mm->mmap_sem);
> +
>  	return err;
>  }
>  
> @@ -1640,8 +1643,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)
> +
> +		/* The page is already on the target node */
> +		if (!err) {
> +			err = store_status(status, i, current_node, 1);
> +			if (err)
> +				goto out_flush;
>  			continue;
> +		/* The page is successfully queued */

Nit: could you move this comment down one line, so that it's clear that it
applies to the "else" branch? Like this:

		} else if (err > 0) {
			/* The page is successfully queued for migration */
			continue;
		}


> +		} else if (err > 0) {
> +			continue;
> +		}
>  
>  		err = store_status(status, i, err, 1);
>  		if (err)
> 

Also agree with Christopher's requests, but all of these don't affect the
correct operation of the patch, so you can add:

Reviewed-by: John Hubbard <[e-mail]>

thanks,

Yang Shi Dec. 5, 2019, 9:59 p.m. UTC | #6

On 12/5/19 11:45 AM, Christopher Lameter wrote:
> On Fri, 6 Dec 2019, 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:
> Looks ok.
>
> Acked-by: Christoph Lameter <[e-mail]>
>
> Nitpicks:
>
>> @@ -1553,7 +1555,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;
> Add a meaningful constant instead of 1?

Since it just returns errno, 0 and 1 it sounds not worth a constant or enum.

>
>>   out:
>>   	up_read(&mm->mmap_sem);
>> +
>>   	return err;
> Dont do that.

Yes, my fat finger.

Yang Shi Dec. 5, 2019, 10 p.m. UTC | #7

On 12/5/19 1:27 PM, John Hubbard wrote:
> On 12/5/19 10:54 AM, 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.
>>
>> Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move")
>> Reported-by: Felix Abecassis <[e-mail]>
>> Tested-by: Felix Abecassis <[e-mail]>
>> Suggested-by: Michal Hocko <[e-mail]>
>> Cc: John Hubbard <[e-mail]>
>> Cc: Christoph Lameter <[e-mail]>
>> Cc: Vlastimil Babka <[e-mail]>
>> Cc: Mel Gorman <[e-mail]>
>> Cc: <[e-mail]> 4.17+
>> Signed-off-by: Yang Shi <[e-mail]>
>> ---
>> v3: * Adopted the suggestion from Michal.
>> v2: * Correted the return value when add_page_for_migration() returns 1.
>>
>>   mm/migrate.c | 24 ++++++++++++++++++------
>>   1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index a8f87cb..9c172f4 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -1512,9 +1512,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)
>> @@ -1553,7 +1555,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;
>> @@ -1563,7 +1565,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),
>> @@ -1578,6 +1580,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
>>   	put_page(page);
>>   out:
>>   	up_read(&mm->mmap_sem);
>> +
>>   	return err;
>>   }
>>   
>> @@ -1640,8 +1643,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)
>> +
>> +		/* The page is already on the target node */
>> +		if (!err) {
>> +			err = store_status(status, i, current_node, 1);
>> +			if (err)
>> +				goto out_flush;
>>   			continue;
>> +		/* The page is successfully queued */
> Nit: could you move this comment down one line, so that it's clear that it
> applies to the "else" branch? Like this:
>
> 		} else if (err > 0) {
> 			/* The page is successfully queued for migration */
> 			continue;
> 		}

Sure.

>
>
>> +		} else if (err > 0) {
>> +			continue;
>> +		}
>>   
>>   		err = store_status(status, i, err, 1);
>>   		if (err)
>>
> Also agree with Christopher's requests, but all of these don't affect the
> correct operation of the patch, so you can add:
>
> Reviewed-by: John Hubbard <[e-mail]>
>
> thanks,

Yang Shi Dec. 5, 2019, 10:09 p.m. UTC | #8

On 12/5/19 11:34 AM, Qian Cai wrote:
>
>> On Dec 5, 2019, at 2:27 PM, Yang Shi <[e-mail]> wrote:
>>
>> 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.
> No, I think it is important to figure out this in the first place. Otherwise, it is pointless to touch this piece of code over and over again, i.e., this is not another issue but the core of this problem on hand.

As I said the status return value issue is a regression, but the -ENOENT 
issue has been there since the syscall was introduced (The visual 
inspection shows so I didn't actually run test against 2.6.x kernel, but 
it returns 0 for >= 3.10 at least). It does need further clarification 
(doc problem or code problem).

Michal also noticed several inconsistencies when he was reworking 
move_pages(), and I agree with him that we'd better not touch them 
without a clear usecase.

Qian Cai Dec. 5, 2019, 10:23 p.m. UTC | #9

> On Dec 5, 2019, at 5:09 PM, Yang Shi <[e-mail]> wrote:
> 
> As I said the status return value issue is a regression, but the -ENOENT issue has been there since the syscall was introduced (The visual inspection shows so I didn't actually run test against 2.6.x kernel, but it returns 0 for >= 3.10 at least). It does need further clarification (doc problem or code problem).

The question is why we should care about this change of behavior. It is arguably you are even trying to fix an ambiguous part of the manpage, but instead leave a more obviously one still broken. It is really difficult to understand the logical here.

> 
> Michal also noticed several inconsistencies when he was reworking move_pages(), and I agree with him that we'd better not touch them without a clear usecase.

It could argue that there is no use case to restore the behavior either.

John Hubbard Dec. 5, 2019, 10:41 p.m. UTC | #10

On 12/5/19 2:23 PM, Qian Cai wrote:
>> On Dec 5, 2019, at 5:09 PM, Yang Shi <[e-mail]> wrote:
>>
>> As I said the status return value issue is a regression, but the -ENOENT issue has been there since the syscall was introduced (The visual inspection shows so I didn't actually run test against 2.6.x kernel, but it returns 0 for >= 3.10 at least). It does need further clarification (doc problem or code problem).
> 
> The question is why we should care about this change of behavior. It is arguably you are even trying to fix an ambiguous part of the manpage, but instead leave a more obviously one still broken. It is really difficult to understand the logical here.
> 

Please recall how this started: it was due to a report from a real end user, who was 
seeing a real problem. After a few emails, it was clear that there's not a good
work around available for cases like this:

* User space calls move_pages(), gets 0 (success) returned, and based on that,
proceeds to iterate through the status array.

* The status array remains untouched by the move_pages() call, so confusion and
wrong behavior ensues.

After some further discussion, we decided that the current behavior really is 
incorrect, and that it needs fixing in the kernel. Which this patch does.

>>
>> Michal also noticed several inconsistencies when he was reworking move_pages(), and I agree with him that we'd better not touch them without a clear usecase.
> 
> It could argue that there is no use case to restore the behavior either.
> 

So far, there are no reports from the field, and that's probably the key
difference between these two situations.

Hope that clears up the reasoning for you. I might add that, were you to study
all the emails in these threads, and the code and the man page, you would
probably agree with the conclusions above. You might disagree with the underlying
philosophies (such as "user space is really important and we fix it if it
breaks", etc), but that's a different conversation.


thanks,

Qian Cai Dec. 5, 2019, 11:16 p.m. UTC | #11

> On Dec 5, 2019, at 5:41 PM, John Hubbard <[e-mail]> wrote:
> 
> Please recall how this started: it was due to a report from a real end user, who was 
> seeing a real problem. After a few emails, it was clear that there's not a good
> work around available for cases like this:
> 
> * User space calls move_pages(), gets 0 (success) returned, and based on that,
> proceeds to iterate through the status array.
> 
> * The status array remains untouched by the move_pages() call, so confusion and
> wrong behavior ensues.
> 
> After some further discussion, we decided that the current behavior really is 
> incorrect, and that it needs fixing in the kernel. Which this patch does.

Well, that test code itself  does not really tell any real world use case.  Also, thanks to the discussion, it brought to me it is more obvious and critical  that the return code is wrong according to the spec. Then, if that part is taking care of, it would kill two-bird with one stone because there is no need to return status array anymore. Make sense?

John Hubbard Dec. 5, 2019, 11:24 p.m. UTC | #12

On 12/5/19 3:16 PM, Qian Cai wrote:
> 
> 
>> On Dec 5, 2019, at 5:41 PM, John Hubbard <[e-mail]> wrote:
>>
>> Please recall how this started: it was due to a report from a real end user, who was 
>> seeing a real problem. After a few emails, it was clear that there's not a good
>> work around available for cases like this:
>>
>> * User space calls move_pages(), gets 0 (success) returned, and based on that,
>> proceeds to iterate through the status array.
>>
>> * The status array remains untouched by the move_pages() call, so confusion and
>> wrong behavior ensues.
>>
>> After some further discussion, we decided that the current behavior really is 
>> incorrect, and that it needs fixing in the kernel. Which this patch does.
> 
> Well, that test code itself  does not really tell any real world use case.  Also, thanks to the discussion, it brought to me it is more obvious and critical  that the return code is wrong according to the spec. Then, if that part is taking care of, it would kill two-bird with one stone because there is no need to return status array anymore. Make sense?
> 

Let's check in the fix that is clearly correct and non-controversial, in one
patch. Then another patch can be created for the other case. This allows forward
progress and quick resolution of the user's bug report, while still dealing
with all the problems.

If you try to fix too many problems in one patch (and remember, sometimes ">1"
is too many), then things bog down. It's always a judgment call, but what's 
unfolding here is quite consistent with the usual judgment calls in this area.

I don't think anyone is saying, "don't work on the second problem", it's just
that it's less urgent, due to no reports from the field. If you are passionate
about fixing the second problem (and are ready and willing to handle the fallout
from user space, if it occurs), then I'd encourage you to look into it.

It could turn out to be one of those "cannot change this because user space expectations
have baked and hardened, and changes would break user space" situations, just to
warn you in advance, though.

thanks,

Qian Cai Dec. 5, 2019, 11:58 p.m. UTC | #13

> On Dec 5, 2019, at 6:24 PM, John Hubbard <[e-mail]> wrote:
> 
> Let's check in the fix that is clearly correct and non-controversial, in one
> patch. Then another patch can be created for the other case. This allows forward
> progress and quick resolution of the user's bug report, while still dealing
> with all the problems.
> 
> If you try to fix too many problems in one patch (and remember, sometimes ">1"
> is too many), then things bog down. It's always a judgment call, but what's 
> unfolding here is quite consistent with the usual judgment calls in this area.
> 
> I don't think anyone is saying, "don't work on the second problem", it's just
> that it's less urgent, due to no reports from the field. If you are passionate
> about fixing the second problem (and are ready and willing to handle the fallout
> from user space, if it occurs), then I'd encourage you to look into it.
> 
> It could turn out to be one of those "cannot change this because user space expectations
> have baked and hardened, and changes would break user space" situations, just to
> warn you in advance, though.

There is no need to paper over the underlying issue. One can think there is only one problem. The way move_pages() deal with pages are already in the desired node. Then, I don’t see there is any controversy that it was broken for so long and just restore it to according to the manpage. If you worried about people has already depended on the broken behavior, it could stay in linux-next for many releases to gather feedback. In any case, I don’t see it need to hurry to fix this until someone can show the real world use case for it apart from some random test code.

John Hubbard Dec. 6, 2019, 12:04 a.m. UTC | #14

On 12/5/19 3:58 PM, Qian Cai wrote:
> 
> 
>> On Dec 5, 2019, at 6:24 PM, John Hubbard <[e-mail]> wrote:
>>
>> Let's check in the fix that is clearly correct and non-controversial, in one
>> patch. Then another patch can be created for the other case. This allows forward
>> progress and quick resolution of the user's bug report, while still dealing
>> with all the problems.
>>
>> If you try to fix too many problems in one patch (and remember, sometimes ">1"
>> is too many), then things bog down. It's always a judgment call, but what's 
>> unfolding here is quite consistent with the usual judgment calls in this area.
>>
>> I don't think anyone is saying, "don't work on the second problem", it's just
>> that it's less urgent, due to no reports from the field. If you are passionate
>> about fixing the second problem (and are ready and willing to handle the fallout
>> from user space, if it occurs), then I'd encourage you to look into it.
>>
>> It could turn out to be one of those "cannot change this because user space expectations
>> have baked and hardened, and changes would break user space" situations, just to
>> warn you in advance, though.
> 
> There is no need to paper over the underlying issue. One can think there is only one problem. The way move_pages() deal with pages are already in the desired node. Then, I don’t see there is any controversy that it was broken for so long and just restore it to according to the manpage. If you worried about people has already depended on the broken behavior, it could stay in linux-next for many releases to gather feedback. In any case, I don’t see it need to hurry to fix this until someone can show the real world use case for it apart from some random test code.
> 

Felix's code is not random test code. It's code he wrote and he expected it to work.

Anyway, I've explained what I want here, and done my best to explain it. So I'm 
dropping off now. :)

thanks,

Qian Cai Dec. 6, 2019, 12:19 a.m. UTC | #15

> On Dec 5, 2019, at 7:04 PM, John Hubbard <[e-mail]> wrote:
> 
> Felix's code is not random test code. It's code he wrote and he expected it to work.

Sure, but could he show a bit detail if the kernel will start to behavior as expected like what was written in the manpage to return ENOENT in this case, why is it not acceptable? i.e., why is it important to depend on the broken behavior?

Yang Shi Dec. 6, 2019, 1:11 a.m. UTC | #16

On 12/5/19 4:19 PM, Qian Cai wrote:
>
>> On Dec 5, 2019, at 7:04 PM, John Hubbard <[e-mail]> wrote:
>>
>> Felix's code is not random test code. It's code he wrote and he expected it to work.
> Sure, but could he show a bit detail if the kernel will start to behavior as expected like what was written in the manpage to return ENOENT in this case, why is it not acceptable? i.e., why is it important to depend on the broken behavior?


I think I found the root cause. It did return -ENOENT when the syscall 
was introduced (my first impression was wrong), but the behavior was 
changed since 2.6.28 by commit e78bbfa82624 ("mm: stop returning -ENOENT 
from sys_move_pages() if nothing got migrated"). The full commit log is 
as below:

Author: Brice Goglin <[e-mail]>
Date:   Sat Oct 18 20:27:15 2008 -0700

     mm: stop returning -ENOENT from sys_move_pages() if nothing got 
migrated

     A patchset reworking sys_move_pages().  It removes the possibly large
     vmalloc by using multiple chunks when migrating large buffers. It also
     dramatically increases the throughput for large buffers since the 
lookup
     in new_page_node() is now limited to a single chunk, causing the 
quadratic
     complexity to have a much slower impact.  There is no need to use any
     radix-tree-like structure to improve this lookup.

     sys_move_pages() duration on a 4-quadcore-opteron 2347HE (1.9Gz),
     migrating between nodes #2 and #3:

             length          move_pages (us)         move_pages+patch (us)
             4kB             126                     98
             40kB            198                     168
             400kB           963                     937
             4MB             12503                   11930
             40MB            246867                  11848

     Patches #1 and #4 are the important ones:
     1) stop returning -ENOENT from sys_move_pages() if nothing got migrated
     2) don't vmalloc a huge page_to_node array for do_pages_stat()
     3) extract do_pages_move() out of sys_move_pages()
     4) rework do_pages_move() to work on page_sized chunks
     5) move_pages: no need to set pp->page to ZERO_PAGE(0) by default

     This patch:

     There is no point in returning -ENOENT from sys_move_pages() if all 
pages
     were already on the right node, while we return 0 if only 1 page 
was not.
     Most application don't know where their pages are allocated, so 
it's not
     an error to try to migrate them anyway.

     Just return 0 and let the status array in user-space be checked if the
     application needs details.

     It will make the upcoming chunked-move_pages() support much easier.

     Signed-off-by: Brice Goglin <[e-mail]>
     Acked-by: Christoph Lameter <[e-mail]>
     Cc: Nick Piggin <[e-mail]>
     Signed-off-by: Andrew Morton <[e-mail]>
     Signed-off-by: Linus Torvalds <[e-mail]>


So the behavior was changed in kernel intentionally but never reflected 
in the manpage. I will propose a patch to fix the document.

Michal Hocko Dec. 6, 2019, 7:35 a.m. UTC | #17

On Thu 05-12-19 19:45:49, Cristopher Lameter wrote:
> On Fri, 6 Dec 2019, 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:
> 
> Looks ok.
> 
> Acked-by: Christoph Lameter <[e-mail]>
> 
> Nitpicks:
> 
> > @@ -1553,7 +1555,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;
> 
> Add a meaningful constant instead of 1?

Well 1 has a good meaning here actually. We have -errno or the number of
queued pages.

Patch

diff --git a/mm/migrate.c b/mm/migrate.c
index a8f87cb..9c172f4 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1512,9 +1512,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)
@@ -1553,7 +1555,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;
@@ -1563,7 +1565,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),
@@ -1578,6 +1580,7 @@  static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
 	put_page(page);
 out:
 	up_read(&mm->mmap_sem);
+
 	return err;
 }
 
@@ -1640,8 +1643,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)
+
+		/* The page is already on the target node */
+		if (!err) {
+			err = store_status(status, i, current_node, 1);
+			if (err)
+				goto out_flush;
 			continue;
+		/* The page is successfully queued */
+		} else if (err > 0) {
+			continue;
+		}
 
 		err = store_status(status, i, err, 1);
 		if (err)