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

[Minchan Kim <minchan@kernel.org>]

On Wed, Feb 01, 2017 at 11:00:23AM +0100, Michal Hocko wrote:
> 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@foxmail.com wrote:
> > > > > From: Yisheng Xie <xieyisheng1@huawei.com>
> > > > > 
> > > > > 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@huawei.com>
> > > > > Suggested-by: Michal Hocko <mhocko@kernel.org>
> > > > 
> > > > 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.

It means it's binary return value(success: 0 fail : -EBUSY) so IMO,
bool is better and caller should return -EBUSY if that functions
returns *false*. No need to make deeper propagation level.
Anyway, it's trivial so I'm not against it if you want to make
isolate_movable_page returns int. Insetad, please remove -EINVAL
in this patch and just return -EBUSY for isolate_movable_page to
be consistent with isolate_lru_page.
Then, we don't need to fix any driver side, either. Even, no need to
update any document because you don't add any new error value.

That's enough.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>