Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 19 Jan 2020 10:41:24 +0800
From:   Wei Yang <richardw.yang@linux.intel.com>
To:     Yang Shi <shy828301@gmail.com>
Cc:     Wei Yang <richardw.yang@linux.intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] mm/migrate.c: also overwrite error when it is bigger
 than zero
Message-ID: <20200119024124.GF9745@richard>
Reply-To: Wei Yang <richardw.yang@linux.intel.com>
References: <20200117074534.25324-1-richardw.yang@linux.intel.com>
 <20200117222740.GB29229@richard>
 <CAHbLzkoYH1_JHH99pnopj_v=Wb=UEGMS9dJs1J6GZn0=6F4SJw@mail.gmail.com>
 <20200117234829.GA2844@richard>
 <CAHbLzko_UC47Y0gBsRRK0oJS5fvhJ80EpvrjTsFi8+PuTCHGEQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHbLzko_UC47Y0gBsRRK0oJS5fvhJ80EpvrjTsFi8+PuTCHGEQ@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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