Message-ID: <20200124231926.GA16638@richard>
Date:   Sat, 25 Jan 2020 07:19:26 +0800
From:   Wei Yang <richardw.yang@...ux.intel.com>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     Wei Yang <richardw.yang@...ux.intel.com>, g@...hard.suse.de,
        Yang Shi <yang.shi@...ux.alibaba.com>,
        akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [v2 PATCH] mm: move_pages: report the number of non-attempted
 pages

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

Powered by blists - more mailing lists