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 <[email protected]>
Cc: Christoph Lameter <[email protected]>
Cc: John Hubbard <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Qian Cai <[email protected]>
Signed-off-by: Yang Shi <[email protected]>
---
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
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
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 <[email protected]>
> Cc: Christoph Lameter <[email protected]>
> Cc: John Hubbard <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Qian Cai <[email protected]>
> Signed-off-by: Yang Shi <[email protected]>
> ---
> 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
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 <[email protected]>
> Cc: Christoph Lameter <[email protected]>
> Cc: John Hubbard <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Qian Cai <[email protected]>
> Signed-off-by: Yang Shi <[email protected]>
> ---
> 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
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 <[email protected]>
> > Cc: Christoph Lameter <[email protected]>
> > Cc: John Hubbard <[email protected]>
> > Cc: Michal Hocko <[email protected]>
> > Cc: Qian Cai <[email protected]>
> > Signed-off-by: Yang Shi <[email protected]>
> > ---
> > 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
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 <[email protected]>
>> Cc: Christoph Lameter <[email protected]>
>> Cc: John Hubbard <[email protected]>
>> Cc: Michal Hocko <[email protected]>
>> Cc: Qian Cai <[email protected]>
>> Signed-off-by: Yang Shi <[email protected]>
>> ---
>> 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,
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 <[email protected]>
>>> Cc: Christoph Lameter <[email protected]>
>>> Cc: John Hubbard <[email protected]>
>>> Cc: Michal Hocko <[email protected]>
>>> Cc: Qian Cai <[email protected]>
>>> Signed-off-by: Yang Shi <[email protected]>
>>> ---
>>> 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.
> On Dec 6, 2019, at 12:31 PM, Yang Shi <[email protected]> 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.
On Fri, 6 Dec 2019, Qian Cai wrote:
> > On Dec 6, 2019, at 12:31 PM, Yang Shi <[email protected]> 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).
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 <[email protected]>
>>> Cc: Christoph Lameter <[email protected]>
>>> Cc: John Hubbard <[email protected]>
>>> Cc: Michal Hocko <[email protected]>
>>> Cc: Qian Cai <[email protected]>
>>> Signed-off-by: Yang Shi <[email protected]>
>>> ---
>>> 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/
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,
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 <[email protected]>
>
> 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.
Yang Shi <[email protected]> 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 <[email protected]>
>>
>> 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
On 12/30/19 7:49 PM, Eric W. Biederman wrote:
> Yang Shi <[email protected]> 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 <[email protected]>
>>>
>>> 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