Re: [PATCH] mm/migrate.c: also overwrite error when it is bigger than zero

[Yang Shi]

On Sat, Jan 18, 2020 at 6:41 PM Wei Yang <richardw.yang@xxxxxxxxxxxxxxx> wrote:
>
> On Fri, Jan 17, 2020 at 08:56:27PM -0800, Yang Shi wrote:
> >On Fri, Jan 17, 2020 at 3:48 PM Wei Yang <richardw.yang@xxxxxxxxxxxxxxx> wrote:
> >>
> >> On Fri, Jan 17, 2020 at 03:30:18PM -0800, Yang Shi wrote:
> >> >On Fri, Jan 17, 2020 at 2:27 PM Wei Yang <richardw.yang@xxxxxxxxxxxxxxx> wrote:
> >> >>
> >> >> On Fri, Jan 17, 2020 at 03:45:34PM +0800, Wei Yang wrote:
> >> >> >If we get here after successfully adding page to list, err would be
> >> >> >the number of pages in the list.
> >> >> >
> >> >> >Current code has two problems:
> >> >> >
> >> >> >  * on success, 0 is not returned
> >> >> >  * on error, the real error code is not returned
> >> >> >
> >> >>
> >> >> Well, this breaks the user interface. User would receive 1 even the migration
> >> >> succeed.
> >> >>
> >> >> The change is introduced by e0153fc2c760 ("mm: move_pages: return valid node
> >> >> id in status if the page is already on the target node").
> >> >
> >> >Yes, it may return a value which is > 0. But, it seems do_pages_move()
> >> >could return > 0 value even before this commit.
> >> >
> >> >For example, if I read the code correctly, it would do:
> >> >
> >> >If we already have some pages on the queue then
> >> >add_page_for_migration() return error, then do_move_pages_to_node() is
> >> >called, but it may return > 0 value (the number of pages that were
> >> >*not* migrated by migrate_pages()), then the code flow would just jump
> >> >to "out" and return the value. And, it may happen to be 1.
> >> >
> >>
> >> This is another point I think current code is not working well. And actually,
> >> the behavior is not well defined or our kernel is broken for a while.
> >
> >Yes, we already spotted a few mismatches, inconsistencies and edge
> >cases in these NUMA APIs.
> >
> >>
> >> When you look at the man page, it says:
> >>
> >>     RETURN VALUE
> >>            On success move_pages() returns zero.  On error, it returns -1, and sets errno to indicate the error
> >>
> >> So per my understanding, the design is to return -1 on error instead of the
> >> pages not managed to move.
> >
> >So do I.
> >
> >>
> >> For the user interface, if original code check 0 for success, your change
> >> breaks it. Because your code would return 1 instead of 0. Suppose most user
> >> just read the man page for programming instead of reading the kernel source
> >> code. I believe we need to fix it.
> >
> >Yes, I definitely agree we need fix it. But the commit log looks
> >confusing, particularly "on error, the real error code is not
> >returned". If the error is returned by add_page_for_migration() then
> >it will not be returned to userspace instead of reporting via status.
> >Do you mean this?
> >
>
> Sorry for the confusion.
>
> Here I mean, if add_page_for_migratioin() return 1, and the following err1
> from do_move_pages_to_node() is set, the err1 is not returned.
>
> The reason is err is not 0 at this point.

Yes, I see your point. Please elaborate this in the commit log.

>
> --
> Wei Yang
> Help you, Help me