| Message ID | 1575596090-115377-1-git-send-email-yang.shi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | move_pages.2: not return ENOENT if the page are already on the target nodes | expand |
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,
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."
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,
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.
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,
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.
> 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.
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).
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
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,
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.
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 <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.
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
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
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
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(-)