On Wed 29-08-18 07:59:37, Jan Stancek wrote:
>
>
> ----- Original Message -----
> > On Sun 26-08-18 15:47:52, Michael Kerrisk wrote:
> > > Hello Jan,
> > >
> > > On 08/24/2018 02:27 PM, Jan Stancek wrote:
> > > > Since the rework done in a49bd4d71637 ("mm, numa: rework do_pages_move"),
> > > > move_pages() can return also positive value.
> > > >
> > > > Signed-off-by: Jan Stancek <jstancek@xxxxxxxxxx>
> > > > ---
> > > > man2/move_pages.2 | 6 ++++--
> > > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > https://github.com/linux-test-project/ltp/issues/388
> > > >
> > > > diff --git a/man2/move_pages.2 b/man2/move_pages.2
> > > > index f2c4f7f7a269..35cf6de1ba78 100644
> > > > --- a/man2/move_pages.2
> > > > +++ b/man2/move_pages.2
> > > > @@ -157,9 +157,11 @@ The page is not present.
> > > > .B -ENOMEM
> > > > Unable to allocate memory on target node.
> > > > .SH RETURN VALUE
> > > > -On success
> > > > +On success (since 4.17)
> > > > .BR move_pages ()
> > > > -returns zero.
> > > > +returns the number of pages that could not be moved
> > > > +(i.e., a return of zero means that all pages were successfully moved,
> > > > +older kernels return 0).
> > > > .\" FIXME . Is the following quite true: does the wrapper in numactl
> > > > .\" do the right thing?
> > > > On error, it returns \-1, and sets
> > >
> > > The wording here seems a bit confusing, because the detail about
> > > older kernel behavior is hidden in a parenthetical aside. Can you
> > > confirm what I understand:
> > >
> > > [[
> > > Before Linux 4.17, move_pages() always returned 0 on success.
> > > [But, what is the return value if not all of the pages could
> > > be moved? Is it 0 or -1?]
> >
> > Please refer to the changelog of the commit. The failure semantic was
> > quite hazy. Some errors were simply not reported. But in general there
> > was -1 on failure and 0 on success (whatever the later means). One would
> > have to check for status of each page to tell.
>
> Could we define 'success' as "status array has been populated"?
>
> >
> > > Since Linux, a successful call to move_pages() returns the number
> > > of pages that could not be moved (i.e., a return of zero means that
> > > all pages were successfully moved, older kernels return 0).
> > > ]]
> > >
> > > If that's correct, then I think the new text should read more
> > > like that, withe open question answered.
>
> Would this work?
>
> [[
> Before Linux 4.17, return value of 0 (success) meant that result
> of the move (per page) is stored in status array.
>
> Since Linux 4.17, a successful call to move_pages() returns the number
> of pages that could not be moved (i.e., a return of zero means that
> all pages were successfully moved), and populates status array.
> ]]
I am not sure the former is true and now after double checking I am
pretty sure the later is not true either. The failure case was not
really well defined in this function previously and right now the
number of failed pages to migrate is also incomplete. It applies only
to the batch we attempted to migrated. Which is not the full range.
The following would make it the full range aware
diff --git a/mm/migrate.c b/mm/migrate.c
index d6a2e89b086a..40037f6d2d4b 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1622,8 +1622,16 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
goto out_flush;
err = do_move_pages_to_node(mm, &pagelist, current_node);
- if (err)
+ if (err) {
+ /*
+ * Possitive err means the number of failed pages to
+ * migrate. Make sure to report the rest of the
+ * nr_pages is not migrated as well.
+ */
+ if (err > 0)
+ err += nr_pages - i - 1;
goto out;
+ }
if (i > start) {
err = store_status(status, start, current_node, i - start);
if (err)
--
Michal Hocko
SUSE Labs
