On Fri, Jan 24, 2020 at 04:40:15PM +0100, Michal Hocko wrote:
>On Fri 24-01-20 23:26:42, Wei Yang wrote:
>> On Fri, Jan 24, 2020 at 07:46:49AM +0100, Michal Hocko wrote:
>> >On Fri 24-01-20 06:56:47, Wei Yang wrote:
>> >> On Thu, Jan 23, 2020 at 09:55:26AM +0100, Michal Hocko wrote:
>> >> >On Thu 23-01-20 11:27:36, Wei Yang wrote:
>> >> >> On Thu, Jan 23, 2020 at 07:38:51AM +0800, Yang Shi wrote:
>> >> >> >Since commit a49bd4d71637 ("mm, numa: rework do_pages_move"),
>> >> >> >the semantic of move_pages() was changed to return the number of
>> >> >> >non-migrated pages (failed to migration) and the call would be aborted
>> >> >> >immediately if migrate_pages() returns positive value. But it didn't
>> >> >> >report the number of pages that we even haven't attempted to migrate.
>> >> >> >So, fix it by including non-attempted pages in the return value.
>> >> >> >
>> >> >>
>> >> >> First, we want to change the semantic of move_pages(2). The return value
>> >> >> indicates the number of pages we didn't managed to migrate?
>> >> >>
>> >> >> Second, the return value from migrate_pages() doesn't mean the number of pages
>> >> >> we failed to migrate. For example, one -ENOMEM is returned on the first page,
>> >> >> migrate_pages() would return 1. But actually, no page successfully migrated.
>> >> >
>> >> >ENOMEM is considered a permanent failure and as such it is returned by
>> >> >migrate pages (see goto out).
>> >> >
>> >> >> Third, even the migrate_pages() return the exact non-migrate page, we are not
>> >> >> sure those non-migrated pages are at the tail of the list. Because in the last
>> >> >> case in migrate_pages(), it just remove the page from list. It could be a page
>> >> >> in the middle of the list. Then, in userspace, how the return value be
>> >> >> leveraged to determine the valid status? Any page in the list could be the
>> >> >> victim.
>> >> >
>> >> >Yes, I was wrong when stating that the caller would know better which
>> >> >status to check. I misremembered the original patch as it was quite some
>> >> >time ago. While storing the error code would be possible after some
>> >> >massaging of migrate_pages is this really something we deeply care
>> >> >about. The caller can achieve the same by initializing the status array
>> >> >to a non-node number - e.g. -1 - and check based on that.
>> >> >
>> >>
>> >> So for a user, the best practice is to initialize the status array to -1 and
>> >> check each status to see whether the page is migrated successfully?
>> >
>> >Yes IMO. Just consider -errno return value. You have no way to find out
>> >which pages have been migrated until we reached that error. The
>> >possitive return value would fall into the same case.
>> >
>> >> Then do we need to return the number of non-migrated page? What benefit could
>> >> user get from the number. How about just return an error code to indicate the
>> >> failure? I may miss some point, would you mind giving me a hint?
>> >
>> >This is certainly possible. We can return -EAGAIN if some pages couldn't
>> >be migrated because they are pinned. But please read my previous email
>> >to the very end for arguments why this might cause more problems than it
>> >actually solves.
>> >
>>
>> Let me put your comment here:
>>
>> Because new users could have started depending on it. It
>> is not all that unlikely that the current implementation would just
>> work for them because they are migrating a set of pages on to the same
>> node so the batch would be a single list throughout the whole given
>> page set.
>>
>> Your idea is to preserve current semantic, return non-migrated pages number to
>> userspace.
>>
>> And the reason is:
>>
>> 1. Users have started depending on it.
>> 2. No real bug reported yet.
>> 3. User always migrate page to the same node. (If my understanding is
>> correct)
>>
>> I think this gets some reason, since we want to minimize the impact to
>> userland.
>>
>> While let's see what user probably use this syscall. Since from the man page,
>> we never told the return value could be positive, the number of non-migrated
>> pages, user would think only 0 means a successful migration and all other
>> cases are failure. Then user probably handle negative and positive return
>> value the same way, like (!err).
>>
>> If my guess is true, return a negative error value for this case could
>> minimize the impact to userland here.
>> 1. Preserve the semantic of move_pages(2): 0 means success, negative means
>> some error and needs extra handling.
>> 2. Trivial change to the man page.
>> 3. Suppose no change to users.
>
>Do you have any actual proposal we can discuss? I suspect we are going
>in circles here. Sure both ways are possible. The disucssion we are
>having here is which behavior makes more sense. The interface is and has
>been in the past very awkward. Some corner cases have been fixed some
>new created. While I am not happy about the later we should finally land
>with some decision.
Ok, I found myself may miss some mechanism about the err reporting from kernel
to userland.
If do_pages_move() returns a negative err, the value would be set into errno
and actually user just see a return value of -1?
So userland just see two types of return value if kernel comply with man page:
0 : success
-1: failure, with reason set into errno
Is my understanding correct? I tried to read the syscall path, but not find
how the negative value is set into errno.
Since our kernel already return a positive value one migration failure, so the
exact return value from move_pages() syscall is:
> 0: number of non-migrate pages
0 : success
-1 : failure, with reason set into errno
Since everything looks good to userland now, we just extend the semantic of
move_pages() to make positive return value an explicit error case.
Is my understanding correct here?
If this is the case, I agree with this fix. It looks the minimal change to
current real world.
>--
>Michal Hocko
>SUSE Labs
--
Wei Yang
Help you, Help me
