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;
MIME-Version: 1.0
References: <20200117074534.25324-1-richardw.yang@linux.intel.com> <20200117222740.GB29229@richard>
In-Reply-To: <20200117222740.GB29229@richard>
From:   Yang Shi <shy828301@gmail.com>
Date:   Fri, 17 Jan 2020 15:30:18 -0800
Message-ID: <CAHbLzkoYH1_JHH99pnopj_v=Wb=UEGMS9dJs1J6GZn0=6F4SJw@mail.gmail.com>
Subject: Re: [PATCH] mm/migrate.c: also overwrite error when it is bigger than zero
To:     Wei Yang <richardw.yang@linux.intel.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Linux MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

I'm not sure if it breaks the user interface since the behavior has
been existed for years, and it looks nobody complains about it. Maybe
glibc helps hide it or people just care if it is 0 and the status.

>
> >Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> >---
> > mm/migrate.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/mm/migrate.c b/mm/migrate.c
> >index 557da996b936..c3ef70de5876 100644
> >--- a/mm/migrate.c
> >+++ b/mm/migrate.c
> >@@ -1677,7 +1677,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> >       err1 = do_move_pages_to_node(mm, &pagelist, current_node);
> >       if (!err1)
> >               err1 = store_status(status, start, current_node, i - start);
> >-      if (!err)
> >+      if (err >= 0)
> >               err = err1;
> > out:
> >       return err;
> >--
> >2.17.1
>
> --
> Wei Yang
> Help you, Help me
>