Re: [PATCH v5 1/4] mm/migration: make isolate_movable_page() return int type

[Michal Hocko]

On Wed 01-02-17 18:46:36, Minchan Kim wrote:
> On Wed, Feb 01, 2017 at 08:59:24AM +0100, Michal Hocko wrote:
> > On Wed 01-02-17 15:48:21, Minchan Kim wrote:
> > > Hi Yisheng,
> > > 
> > > On Tue, Jan 31, 2017 at 09:06:18PM +0800, ysxie@xxxxxxxxxxx wrote:
> > > > From: Yisheng Xie <xieyisheng1@xxxxxxxxxx>
> > > > 
> > > > This patch changes the return type of isolate_movable_page()
> > > > from bool to int. It will return 0 when isolate movable page
> > > > successfully, return -EINVAL when the page is not a non-lru movable
> > > > page, and for other cases it will return -EBUSY.
> > > > 
> > > > There is no functional change within this patch but prepare
> > > > for later patch.
> > > > 
> > > > Signed-off-by: Yisheng Xie <xieyisheng1@xxxxxxxxxx>
> > > > Suggested-by: Michal Hocko <mhocko@xxxxxxxxxx>
> > > 
> > > Sorry for missing this one you guys were discussing.
> > > I don't understand the patch's goal although I read later patches.
> > 
> > The point is that the failed isolation has to propagate error up the
> > call chain to the userspace which has initiated the migration.
> > 
> > > isolate_movable_pages returns success/fail so that's why I selected
> > > bool rather than int but it seems you guys want to propagate more
> > > detailed error to the user so added -EBUSY and -EINVAL.
> > > 
> > > But the question is why isolate_lru_pages doesn't have -EINVAL?
> > 
> > It doesn't have to same as isolate_movable_pages. We should just return
> > EBUSY when the page is no longer movable.
> 
> Why isolate_lru_page is okay to return -EBUSY in case of race while
> isolate_movable_page should return -EINVAL?
> What's the logic in your mind? I totally cannot understand.

Let me rephrase. Both should return EBUSY.

> > > Secondly, madvise man page should update?
> > 
> > Why?
> 
> man page of madvise doesn't say anything about the error propagation
> for soft_offline.

OK, EBUSY should be documented.

> > > Thirdly, if a driver fail isolation due to -ENOMEM, it should be
> > > propagated, too?
> > 
> > Yes
> > 
> > > if we want to propagte detailed error to user, driver's isolate_page
> > > function should return right error.
> > 
> > Yes
> 
> It seems we are okay to return just -EBUSY until now but now you try to
> return more various error. I don't understand what problem you are
> seeing with just -EBUSY. Anyway, if you want to do it, it should be able
> to propagate error from driver side. That means it should make rule
> what kinds of error driver can return. Please write down it to
> Documentation/vm/page_migration and fix zsmalloc/virtio-balloon, too.

agreed!

-- 
Michal Hocko
SUSE Labs