[PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Yang Shi]

Since commit e78bbfa82624 ("mm: stop returning -ENOENT
from sys_move_pages() if nothing got migrated"), move_pages doesn't
return -ENOENT anymore if the pages are already on the target nodes, but
this change is never reflected in manpage.

Cc: Michael Kerrisk <mtk.manpages@gmail.com>
Cc: Christoph Lameter <cl@linux.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Qian Cai <cai@lca.pw>
Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
---
 man2/move_pages.2 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/man2/move_pages.2 b/man2/move_pages.2
index 2d96468..2a2f3cd 100644
--- a/man2/move_pages.2
+++ b/man2/move_pages.2
@@ -192,9 +192,8 @@ was specified or an attempt was made to migrate pages of a kernel thread.
 One of the target nodes is not online.
 .TP
 .B 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
+No pages were found.
+All pages are either not present, had an invalid address or could not be
 moved because they were mapped by multiple processes.
 .TP
 .B EPERM
-- 
1.8.3.1

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[John Hubbard]

On 12/5/19 5:34 PM, Yang Shi wrote:
...
> 
> diff --git a/man2/move_pages.2 b/man2/move_pages.2
> index 2d96468..2a2f3cd 100644
> --- a/man2/move_pages.2
> +++ b/man2/move_pages.2
> @@ -192,9 +192,8 @@ was specified or an attempt was made to migrate pages of a kernel thread.
>  One of the target nodes is not online.
>  .TP
>  .B 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
> +No pages were found.
> +All pages are either not present, had an invalid address or could not be
>  moved because they were mapped by multiple processes.

How about this wording (ignoring man formatting for the moment):

No pages were moved, because all requested pages fell into one or more of
the following cases:

* Page not present.
* Page has an invalid address.
* Page is mapped by multiple processes.

Reasoning: I don't like the "no pages were found" all by itself, because it
blindly rewords the meaning of ENOENT. ENOENT is merely the closest
symbol we have. So we use ENOENT and that's fine, but the descriptive text 
should describe what really happened, which is "no pages were moved". If we had 
an ENOPAGESMOVED then we'd use that. :)

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Michal Hocko]

On Fri 06-12-19 09:34:50, Yang Shi wrote:
> Since commit e78bbfa82624 ("mm: stop returning -ENOENT
> from sys_move_pages() if nothing got migrated"), move_pages doesn't
> return -ENOENT anymore if the pages are already on the target nodes, but
> this change is never reflected in manpage.
> 
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Qian Cai <cai@lca.pw>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> ---
>  man2/move_pages.2 | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/man2/move_pages.2 b/man2/move_pages.2
> index 2d96468..2a2f3cd 100644
> --- a/man2/move_pages.2
> +++ b/man2/move_pages.2
> @@ -192,9 +192,8 @@ was specified or an attempt was made to migrate pages of a kernel thread.
>  One of the target nodes is not online.
>  .TP
>  .B 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
> +No pages were found.
> +All pages are either not present, had an invalid address or could not be
>  moved because they were mapped by multiple processes.

I would rather not put any specifics here because those reasons might
differ in future. I would simply go with "No pages were found that
require or could be moved."

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[John Hubbard]

On 12/5/19 5:34 PM, Yang Shi wrote:
> Since commit e78bbfa82624 ("mm: stop returning -ENOENT
> from sys_move_pages() if nothing got migrated"), move_pages doesn't
> return -ENOENT anymore if the pages are already on the target nodes, but
> this change is never reflected in manpage.
> 
> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Qian Cai <cai@lca.pw>
> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> ---
>   man2/move_pages.2 | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/man2/move_pages.2 b/man2/move_pages.2
> index 2d96468..2a2f3cd 100644
> --- a/man2/move_pages.2
> +++ b/man2/move_pages.2
> @@ -192,9 +192,8 @@ was specified or an attempt was made to migrate pages of a kernel thread.
>   One of the target nodes is not online.
>   .TP
>   .B 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
> +No pages were found.
> +All pages are either not present, had an invalid address or could not be
>   moved because they were mapped by multiple processes.
>   .TP
>   .B EPERM
> 

whoa, hold on. If I'm reading through the various error paths correctly, then this
code is *never* going to return ENOENT for the whole function. It can fill in that
value per-page, in the status array, but that's all. Did I get that right?

If so, we need to redo this part of the man page.


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Michal Hocko]

On Fri 06-12-19 00:25:53, John Hubbard wrote:
> On 12/5/19 5:34 PM, Yang Shi wrote:
> > Since commit e78bbfa82624 ("mm: stop returning -ENOENT
> > from sys_move_pages() if nothing got migrated"), move_pages doesn't
> > return -ENOENT anymore if the pages are already on the target nodes, but
> > this change is never reflected in manpage.
> > 
> > Cc: Michael Kerrisk <mtk.manpages@gmail.com>
> > Cc: Christoph Lameter <cl@linux.com>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Qian Cai <cai@lca.pw>
> > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
> > ---
> >   man2/move_pages.2 | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/man2/move_pages.2 b/man2/move_pages.2
> > index 2d96468..2a2f3cd 100644
> > --- a/man2/move_pages.2
> > +++ b/man2/move_pages.2
> > @@ -192,9 +192,8 @@ was specified or an attempt was made to migrate pages of a kernel thread.
> >   One of the target nodes is not online.
> >   .TP
> >   .B 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
> > +No pages were found.
> > +All pages are either not present, had an invalid address or could not be
> >   moved because they were mapped by multiple processes.
> >   .TP
> >   .B EPERM
> > 
> 
> whoa, hold on. If I'm reading through the various error paths correctly, then this
> code is *never* going to return ENOENT for the whole function. It can fill in that
> value per-page, in the status array, but that's all. Did I get that right?

You are right. Both store_status and do_move_pages_to_node do overwrite
the error code. So you are right that ENOENT return value is not
possible. I haven't checked since when this is the case. This whole
syscall is a disaster from the API and documentation POV.

Btw. Page states error codes could see some refinements as well.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Yang Shi]

On 12/6/19 12:25 AM, John Hubbard wrote:
> On 12/5/19 5:34 PM, Yang Shi wrote:
>> Since commit e78bbfa82624 ("mm: stop returning -ENOENT
>> from sys_move_pages() if nothing got migrated"), move_pages doesn't
>> return -ENOENT anymore if the pages are already on the target nodes, but
>> this change is never reflected in manpage.
>>
>> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
>> Cc: Christoph Lameter <cl@linux.com>
>> Cc: John Hubbard <jhubbard@nvidia.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Qian Cai <cai@lca.pw>
>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>> ---
>>   man2/move_pages.2 | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/man2/move_pages.2 b/man2/move_pages.2
>> index 2d96468..2a2f3cd 100644
>> --- a/man2/move_pages.2
>> +++ b/man2/move_pages.2
>> @@ -192,9 +192,8 @@ was specified or an attempt was made to migrate 
>> pages of a kernel thread.
>>   One of the target nodes is not online.
>>   .TP
>>   .B 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
>> +No pages were found.
>> +All pages are either not present, had an invalid address or could 
>> not be
>>   moved because they were mapped by multiple processes.
>>   .TP
>>   .B EPERM
>>
>
> whoa, hold on. If I'm reading through the various error paths 
> correctly, then this
> code is *never* going to return ENOENT for the whole function. It can 
> fill in that
> value per-page, in the status array, but that's all. Did I get that 
> right?

Nice catch. Yes, you are right.

>
> If so, we need to redo this part of the man page.

Yes.

>
>
> thanks,

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Yang Shi]

On 12/6/19 1:45 AM, Michal Hocko wrote:
> On Fri 06-12-19 00:25:53, John Hubbard wrote:
>> On 12/5/19 5:34 PM, Yang Shi wrote:
>>> Since commit e78bbfa82624 ("mm: stop returning -ENOENT
>>> from sys_move_pages() if nothing got migrated"), move_pages doesn't
>>> return -ENOENT anymore if the pages are already on the target nodes, but
>>> this change is never reflected in manpage.
>>>
>>> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
>>> Cc: Christoph Lameter <cl@linux.com>
>>> Cc: John Hubbard <jhubbard@nvidia.com>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: Qian Cai <cai@lca.pw>
>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>>> ---
>>>    man2/move_pages.2 | 5 ++---
>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/man2/move_pages.2 b/man2/move_pages.2
>>> index 2d96468..2a2f3cd 100644
>>> --- a/man2/move_pages.2
>>> +++ b/man2/move_pages.2
>>> @@ -192,9 +192,8 @@ was specified or an attempt was made to migrate pages of a kernel thread.
>>>    One of the target nodes is not online.
>>>    .TP
>>>    .B 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
>>> +No pages were found.
>>> +All pages are either not present, had an invalid address or could not be
>>>    moved because they were mapped by multiple processes.
>>>    .TP
>>>    .B EPERM
>>>
>> whoa, hold on. If I'm reading through the various error paths correctly, then this
>> code is *never* going to return ENOENT for the whole function. It can fill in that
>> value per-page, in the status array, but that's all. Did I get that right?
> You are right. Both store_status and do_move_pages_to_node do overwrite
> the error code. So you are right that ENOENT return value is not
> possible. I haven't checked since when this is the case. This whole
> syscall is a disaster from the API and documentation POV.

It looks since commit e78bbfa82624 ("mm: stop returning -ENOENT from 
sys_move_pages() if nothing got migrated") too, which reset err to 0 
unconditionally. It seems it is on purpose by that commit the syscall 
caller should check status for the details according to the commit log.

>
> Btw. Page states error codes could see some refinements as well.

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Qian Cai]

> On Dec 6, 2019, at 12:31 PM, Yang Shi <yang.shi@linux.alibaba.com> wrote:
> 
> It looks since commit e78bbfa82624 ("mm: stop returning -ENOENT from sys_move_pages() if nothing got migrated") too, which reset err to 0 unconditionally. It seems it is on purpose by that commit the syscall caller should check status for the details according to the commit log.

I don’t read it on purpose. “There is no point in returning -ENOENT from sys_move_pages() if all pages
were already on the right node”, so this is only taking about the pages in the desired node. Anyway, but now it is probably the best time to think outside the box redesigning this syscalls and nuke this whole mess.

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Christopher Lameter]

[-- Attachment #1: Type: text/plain, Size: 1410 bytes --]

On Fri, 6 Dec 2019, Qian Cai wrote:

> > On Dec 6, 2019, at 12:31 PM, Yang Shi <yang.shi@linux.alibaba.com> wrote:
> >
> > It looks since commit e78bbfa82624 ("mm: stop returning -ENOENT from sys_move_pages() if nothing got migrated") too, which reset err to 0 unconditionally. It seems it is on purpose by that commit the syscall caller should check status for the details according to the commit log.
>
> I don’t read it on purpose. “There is no point in returning -ENOENT from
> sys_move_pages() if all pages were already on the right node”, so this
> is only taking about the pages in the desired node. Anyway, but now it
> is probably the best time to think outside the box redesigning this
> syscalls and nuke this whole mess.

The nature of the beast is that moving pages is not a deterministic
process. The ability to move depends on pages being pinned and locked
by other kernel subsystem. Other system components may also move the page
independently.

If the user calls this system call and wants to move some pages then he
has presumably figured out somehow that pages are misplaced. If no pages
can be moved then the system call did nothing which could indicate that
some other process is interfering with the desire to move pages to certain
nodes.

This could be important to know (maybe the other system components already
moved the page indepently or another user is also migrating pages).

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Michael Kerrisk (man-pages)]

On 12/6/19 6:26 PM, Yang Shi wrote:
> 
> 
> On 12/6/19 12:25 AM, John Hubbard wrote:
>> On 12/5/19 5:34 PM, Yang Shi wrote:
>>> Since commit e78bbfa82624 ("mm: stop returning -ENOENT
>>> from sys_move_pages() if nothing got migrated"), move_pages doesn't
>>> return -ENOENT anymore if the pages are already on the target nodes, but
>>> this change is never reflected in manpage.
>>>
>>> Cc: Michael Kerrisk <mtk.manpages@gmail.com>
>>> Cc: Christoph Lameter <cl@linux.com>
>>> Cc: John Hubbard <jhubbard@nvidia.com>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: Qian Cai <cai@lca.pw>
>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
>>> ---
>>>   man2/move_pages.2 | 5 ++---
>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/man2/move_pages.2 b/man2/move_pages.2
>>> index 2d96468..2a2f3cd 100644
>>> --- a/man2/move_pages.2
>>> +++ b/man2/move_pages.2
>>> @@ -192,9 +192,8 @@ was specified or an attempt was made to migrate 
>>> pages of a kernel thread.
>>>   One of the target nodes is not online.
>>>   .TP
>>>   .B 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
>>> +No pages were found.
>>> +All pages are either not present, had an invalid address or could 
>>> not be
>>>   moved because they were mapped by multiple processes.
>>>   .TP
>>>   .B EPERM
>>>
>>
>> whoa, hold on. If I'm reading through the various error paths 
>> correctly, then this
>> code is *never* going to return ENOENT for the whole function. It can 
>> fill in that
>> value per-page, in the status array, but that's all. Did I get that 
>> right?
> 
> Nice catch. Yes, you are right.
> 
>>
>> If so, we need to redo this part of the man page.
> 
> Yes.

So where are things at with this? Is an improved man-pages 
patch on the way, or is some other action (on the API) planned?

Thanks,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[John Hubbard]

On 12/13/19 5:55 PM, Michael Kerrisk (man-pages) wrote:
...
>>> whoa, hold on. If I'm reading through the various error paths
>>> correctly, then this
>>> code is *never* going to return ENOENT for the whole function. It can
>>> fill in that
>>> value per-page, in the status array, but that's all. Did I get that
>>> right?
>>
>> Nice catch. Yes, you are right.
>>
>>>
>>> If so, we need to redo this part of the man page.
>>
>> Yes.
> 
> So where are things at with this? Is an improved man-pages
> patch on the way, or is some other action (on the API) planned?
> 

I was waiting to see if Yang was going to respond...anyway, I think
we're looking at approximately this sort of change:

diff --git a/man2/move_pages.2 b/man2/move_pages.2
index 2d96468fa..1bf1053f2 100644
--- a/man2/move_pages.2
+++ b/man2/move_pages.2
@@ -191,12 +191,6 @@ was specified or an attempt was made to migrate pages of a kernel thread.
  .B ENODEV
  One of the target nodes is not online.
  .TP
-.B 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.
-.TP
  .B EPERM
  The caller specified
  .B MPOL_MF_MOVE_ALL

...But I'm not sure if we should change the implementation, instead, so
that it *can* return ENOENT. That's the main question to resolve before
creating any more patches, I think.

In addition, Michal mentioned that the page states in the status array also
need updated documentation.


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Michal Hocko]

On Tue 17-12-19 23:36:09, John Hubbard wrote:
[...]
> diff --git a/man2/move_pages.2 b/man2/move_pages.2
> index 2d96468fa..1bf1053f2 100644
> --- a/man2/move_pages.2
> +++ b/man2/move_pages.2
> @@ -191,12 +191,6 @@ was specified or an attempt was made to migrate pages of a kernel thread.
>  .B ENODEV
>  One of the target nodes is not online.
>  .TP
> -.B 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.
> -.TP
>  .B EPERM
>  The caller specified
>  .B MPOL_MF_MOVE_ALL
> 
> ...But I'm not sure if we should change the implementation, instead, so
> that it *can* return ENOENT. That's the main question to resolve before
> creating any more patches, I think.

I would start by dropping any note about ENOENT first. I am not really
sure there is a reasonable usecase for it but maybe somebody comes up
with something and only then we should consider it.

Feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>

ideally with a kernel commit which removed the ENOENT.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Yang Shi]

On 12/17/19 11:36 PM, John Hubbard wrote:
> On 12/13/19 5:55 PM, Michael Kerrisk (man-pages) wrote:
> ...
>>>> whoa, hold on. If I'm reading through the various error paths
>>>> correctly, then this
>>>> code is *never* going to return ENOENT for the whole function. It can
>>>> fill in that
>>>> value per-page, in the status array, but that's all. Did I get that
>>>> right?
>>>
>>> Nice catch. Yes, you are right.
>>>
>>>>
>>>> If so, we need to redo this part of the man page.
>>>
>>> Yes.
>>
>> So where are things at with this? Is an improved man-pages
>> patch on the way, or is some other action (on the API) planned?
>>
>
> I was waiting to see if Yang was going to respond...anyway, I think
> we're looking at approximately this sort of change:
>

Hi John,

I apologize for the delay, just came back from vacation. Thanks for 
taking care of the patch.

> diff --git a/man2/move_pages.2 b/man2/move_pages.2
> index 2d96468fa..1bf1053f2 100644
> --- a/man2/move_pages.2
> +++ b/man2/move_pages.2
> @@ -191,12 +191,6 @@ was specified or an attempt was made to migrate 
> pages of a kernel thread.
>  .B ENODEV
>  One of the target nodes is not online.
>  .TP
> -.B 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.
> -.TP
>  .B EPERM
>  The caller specified
>  .B MPOL_MF_MOVE_ALL
>
> ...But I'm not sure if we should change the implementation, instead, so
> that it *can* return ENOENT. That's the main question to resolve before
> creating any more patches, I think.
>
> In addition, Michal mentioned that the page states in the status array 
> also
> need updated documentation.
>
>
> thanks,

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Yang Shi]

On 12/18/19 2:17 AM, Michal Hocko wrote:
> On Tue 17-12-19 23:36:09, John Hubbard wrote:
> [...]
>> diff --git a/man2/move_pages.2 b/man2/move_pages.2
>> index 2d96468fa..1bf1053f2 100644
>> --- a/man2/move_pages.2
>> +++ b/man2/move_pages.2
>> @@ -191,12 +191,6 @@ was specified or an attempt was made to migrate pages of a kernel thread.
>>   .B ENODEV
>>   One of the target nodes is not online.
>>   .TP
>> -.B 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.
>> -.TP
>>   .B EPERM
>>   The caller specified
>>   .B MPOL_MF_MOVE_ALL
>>
>> ...But I'm not sure if we should change the implementation, instead, so
>> that it *can* return ENOENT. That's the main question to resolve before
>> creating any more patches, I think.
> I would start by dropping any note about ENOENT first. I am not really
> sure there is a reasonable usecase for it but maybe somebody comes up
> with something and only then we should consider it.
>
> Feel free to add
> Acked-by: Michal Hocko <mhocko@suse.com>
>
> ideally with a kernel commit which removed the ENOENT.

A quick audit doesn't show kernel code or comment notes about ENOENT 
wrongly. The status could be set as ENOENT if the page is not present 
(follow_page() returns NULL), and man page does match what kernel does.

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Eric W. Biederman]

Yang Shi <yang.shi@linux.alibaba.com> writes:

> On 12/18/19 2:17 AM, Michal Hocko wrote:
>> On Tue 17-12-19 23:36:09, John Hubbard wrote:
>> [...]
>>> diff --git a/man2/move_pages.2 b/man2/move_pages.2
>>> index 2d96468fa..1bf1053f2 100644
>>> --- a/man2/move_pages.2
>>> +++ b/man2/move_pages.2
>>> @@ -191,12 +191,6 @@ was specified or an attempt was made to migrate pages of a kernel thread.
>>>   .B ENODEV
>>>   One of the target nodes is not online.
>>>   .TP
>>> -.B 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.
>>> -.TP
>>>   .B EPERM
>>>   The caller specified
>>>   .B MPOL_MF_MOVE_ALL
>>>
>>> ...But I'm not sure if we should change the implementation, instead, so
>>> that it *can* return ENOENT. That's the main question to resolve before
>>> creating any more patches, I think.
>> I would start by dropping any note about ENOENT first. I am not really
>> sure there is a reasonable usecase for it but maybe somebody comes up
>> with something and only then we should consider it.
>>
>> Feel free to add
>> Acked-by: Michal Hocko <mhocko@suse.com>
>>
>> ideally with a kernel commit which removed the ENOENT.
>
> A quick audit doesn't show kernel code or comment notes about ENOENT
> wrongly. The status could be set as ENOENT if the page is not present
> (follow_page() returns NULL), and man page does match what kernel
> does.

Doesn't the function one layer up then consume the ENOENT?

Eric

Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes

[Yang Shi]

On 12/30/19 7:49 PM, Eric W. Biederman wrote:
> Yang Shi <yang.shi@linux.alibaba.com> writes:
>
>> On 12/18/19 2:17 AM, Michal Hocko wrote:
>>> On Tue 17-12-19 23:36:09, John Hubbard wrote:
>>> [...]
>>>> diff --git a/man2/move_pages.2 b/man2/move_pages.2
>>>> index 2d96468fa..1bf1053f2 100644
>>>> --- a/man2/move_pages.2
>>>> +++ b/man2/move_pages.2
>>>> @@ -191,12 +191,6 @@ was specified or an attempt was made to migrate pages of a kernel thread.
>>>>    .B ENODEV
>>>>    One of the target nodes is not online.
>>>>    .TP
>>>> -.B 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.
>>>> -.TP
>>>>    .B EPERM
>>>>    The caller specified
>>>>    .B MPOL_MF_MOVE_ALL
>>>>
>>>> ...But I'm not sure if we should change the implementation, instead, so
>>>> that it *can* return ENOENT. That's the main question to resolve before
>>>> creating any more patches, I think.
>>> I would start by dropping any note about ENOENT first. I am not really
>>> sure there is a reasonable usecase for it but maybe somebody comes up
>>> with something and only then we should consider it.
>>>
>>> Feel free to add
>>> Acked-by: Michal Hocko <mhocko@suse.com>
>>>
>>> ideally with a kernel commit which removed the ENOENT.
>> A quick audit doesn't show kernel code or comment notes about ENOENT
>> wrongly. The status could be set as ENOENT if the page is not present
>> (follow_page() returns NULL), and man page does match what kernel
>> does.
> Doesn't the function one layer up then consume the ENOENT?

No, it doesn't. The return value would be reset unconditionally by 
store_status(). This is what the man page patch tries to correct.

>
> Eric