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

[Wei Yang <richardw.yang@linux.intel.com>]

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@linux.intel.com> 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@linux.intel.com> 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.

-- 
Wei Yang
Help you, Help me