Linux-man Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
@ 2019-12-06  1:34 Yang Shi
  2019-12-06  1:47 ` John Hubbard
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Yang Shi @ 2019-12-06  1:34 UTC (permalink / raw)
  To: mtk.manpages, cl, jhubbard, mhocko, cai, akpm
  Cc: yang.shi, linux-man, linux-api, linux-mm, linux-kernel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-06  1:34 [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes Yang Shi
@ 2019-12-06  1:47 ` John Hubbard
  2019-12-06  7:47 ` Michal Hocko
  2019-12-06  8:25 ` John Hubbard
  2 siblings, 0 replies; 16+ messages in thread
From: John Hubbard @ 2019-12-06  1:47 UTC (permalink / raw)
  To: Yang Shi, mtk.manpages, cl, mhocko, cai, akpm
  Cc: linux-man, linux-api, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-06  1:34 [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes Yang Shi
  2019-12-06  1:47 ` John Hubbard
@ 2019-12-06  7:47 ` Michal Hocko
  2019-12-06  8:25 ` John Hubbard
  2 siblings, 0 replies; 16+ messages in thread
From: Michal Hocko @ 2019-12-06  7:47 UTC (permalink / raw)
  To: Yang Shi
  Cc: mtk.manpages, cl, jhubbard, cai, akpm, linux-man, linux-api,
	linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-06  1:34 [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes Yang Shi
  2019-12-06  1:47 ` John Hubbard
  2019-12-06  7:47 ` Michal Hocko
@ 2019-12-06  8:25 ` John Hubbard
  2019-12-06  9:45   ` Michal Hocko
  2019-12-06 17:26   ` Yang Shi
  2 siblings, 2 replies; 16+ messages in thread
From: John Hubbard @ 2019-12-06  8:25 UTC (permalink / raw)
  To: Yang Shi, mtk.manpages, cl, mhocko, cai, akpm
  Cc: linux-man, linux-api, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-06  8:25 ` John Hubbard
@ 2019-12-06  9:45   ` Michal Hocko
  2019-12-06 17:31     ` Yang Shi
  2019-12-06 17:26   ` Yang Shi
  1 sibling, 1 reply; 16+ messages in thread
From: Michal Hocko @ 2019-12-06  9:45 UTC (permalink / raw)
  To: John Hubbard
  Cc: Yang Shi, mtk.manpages, cl, cai, akpm, linux-man, linux-api,
	linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-06  8:25 ` John Hubbard
  2019-12-06  9:45   ` Michal Hocko
@ 2019-12-06 17:26   ` Yang Shi
  2019-12-14  1:55     ` Michael Kerrisk (man-pages)
  1 sibling, 1 reply; 16+ messages in thread
From: Yang Shi @ 2019-12-06 17:26 UTC (permalink / raw)
  To: John Hubbard, mtk.manpages, cl, mhocko, cai, akpm
  Cc: linux-man, linux-api, linux-mm, linux-kernel



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,


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-06  9:45   ` Michal Hocko
@ 2019-12-06 17:31     ` Yang Shi
  2019-12-06 18:00       ` Qian Cai
  0 siblings, 1 reply; 16+ messages in thread
From: Yang Shi @ 2019-12-06 17:31 UTC (permalink / raw)
  To: Michal Hocko, John Hubbard
  Cc: mtk.manpages, cl, cai, akpm, linux-man, linux-api, linux-mm,
	linux-kernel



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.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-06 17:31     ` Yang Shi
@ 2019-12-06 18:00       ` Qian Cai
  2019-12-06 18:19         ` Christopher Lameter
  0 siblings, 1 reply; 16+ messages in thread
From: Qian Cai @ 2019-12-06 18:00 UTC (permalink / raw)
  To: Yang Shi
  Cc: Michal Hocko, John Hubbard, mtk.manpages, cl, akpm, linux-man,
	linux-api, linux-mm, linux-kernel



> 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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-06 18:00       ` Qian Cai
@ 2019-12-06 18:19         ` Christopher Lameter
  0 siblings, 0 replies; 16+ messages in thread
From: Christopher Lameter @ 2019-12-06 18:19 UTC (permalink / raw)
  To: Qian Cai
  Cc: Yang Shi, Michal Hocko, John Hubbard, mtk.manpages, akpm,
	linux-man, linux-api, linux-mm, linux-kernel

[-- 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).

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-06 17:26   ` Yang Shi
@ 2019-12-14  1:55     ` Michael Kerrisk (man-pages)
  2019-12-18  7:36       ` John Hubbard
  0 siblings, 1 reply; 16+ messages in thread
From: Michael Kerrisk (man-pages) @ 2019-12-14  1:55 UTC (permalink / raw)
  To: Yang Shi, John Hubbard, cl, mhocko, cai, akpm
  Cc: mtk.manpages, linux-man, linux-api, linux-mm, linux-kernel

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/

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-14  1:55     ` Michael Kerrisk (man-pages)
@ 2019-12-18  7:36       ` John Hubbard
  2019-12-18 10:17         ` Michal Hocko
  2019-12-31  2:49         ` Yang Shi
  0 siblings, 2 replies; 16+ messages in thread
From: John Hubbard @ 2019-12-18  7:36 UTC (permalink / raw)
  To: Michael Kerrisk (man-pages), Yang Shi, cl, mhocko, cai, akpm
  Cc: linux-man, linux-api, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-18  7:36       ` John Hubbard
@ 2019-12-18 10:17         ` Michal Hocko
  2019-12-31  3:00           ` Yang Shi
  2019-12-31  2:49         ` Yang Shi
  1 sibling, 1 reply; 16+ messages in thread
From: Michal Hocko @ 2019-12-18 10:17 UTC (permalink / raw)
  To: John Hubbard
  Cc: Michael Kerrisk (man-pages),
	Yang Shi, cl, cai, akpm, linux-man, linux-api, linux-mm,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-18  7:36       ` John Hubbard
  2019-12-18 10:17         ` Michal Hocko
@ 2019-12-31  2:49         ` Yang Shi
  1 sibling, 0 replies; 16+ messages in thread
From: Yang Shi @ 2019-12-31  2:49 UTC (permalink / raw)
  To: John Hubbard, Michael Kerrisk (man-pages), cl, mhocko, cai, akpm
  Cc: linux-man, linux-api, linux-mm, linux-kernel



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,


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-18 10:17         ` Michal Hocko
@ 2019-12-31  3:00           ` Yang Shi
  2019-12-31  3:49             ` ebiederm
  0 siblings, 1 reply; 16+ messages in thread
From: Yang Shi @ 2019-12-31  3:00 UTC (permalink / raw)
  To: Michal Hocko, John Hubbard
  Cc: Michael Kerrisk (man-pages),
	cl, cai, akpm, linux-man, linux-api, linux-mm, linux-kernel



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.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-31  3:00           ` Yang Shi
@ 2019-12-31  3:49             ` ebiederm
  2020-01-02 22:15               ` Yang Shi
  0 siblings, 1 reply; 16+ messages in thread
From: ebiederm @ 2019-12-31  3:49 UTC (permalink / raw)
  To: Yang Shi
  Cc: Michal Hocko, John Hubbard, Michael Kerrisk \(man-pages\),
	cl, cai, akpm, linux-man, linux-api, linux-mm, linux-kernel

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
  2019-12-31  3:49             ` ebiederm
@ 2020-01-02 22:15               ` Yang Shi
  0 siblings, 0 replies; 16+ messages in thread
From: Yang Shi @ 2020-01-02 22:15 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Michal Hocko, John Hubbard, Michael Kerrisk (man-pages),
	cl, cai, akpm, linux-man, linux-api, linux-mm, linux-kernel



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


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-06  1:34 [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes Yang Shi
2019-12-06  1:47 ` John Hubbard
2019-12-06  7:47 ` Michal Hocko
2019-12-06  8:25 ` John Hubbard
2019-12-06  9:45   ` Michal Hocko
2019-12-06 17:31     ` Yang Shi
2019-12-06 18:00       ` Qian Cai
2019-12-06 18:19         ` Christopher Lameter
2019-12-06 17:26   ` Yang Shi
2019-12-14  1:55     ` Michael Kerrisk (man-pages)
2019-12-18  7:36       ` John Hubbard
2019-12-18 10:17         ` Michal Hocko
2019-12-31  3:00           ` Yang Shi
2019-12-31  3:49             ` ebiederm
2020-01-02 22:15               ` Yang Shi
2019-12-31  2:49         ` Yang Shi

Linux-man Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-man/0 linux-man/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-man linux-man/ https://lore.kernel.org/linux-man \
		linux-man@vger.kernel.org
	public-inbox-index linux-man

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-man


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git