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

[Minchan Kim]

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.

> 
> > Secondly, madvise man page should update?
> 
> Why?

man page of madvise doesn't say anything about the error propagation
for soft_offline.

> 
> > 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.

> 
> > I don't feel this all changes should be done now. What's the problem
> > if we change isolate_lru_page from int to bool? it returns just binary
> > value so it should be right place to use bool. If it fails, error val
> > is just -EBUSY.
> 
> We really want to propagate the reason why the offline operation has
> failed. Why would we want to postpone that?

I'm not against but if you want to do it, just do it rightly.

> 
> > > Cc: Minchan Kim <minchan@xxxxxxxxxx>
> > > Cc: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>
> > > CC: Vlastimil Babka <vbabka@xxxxxxx>
> > > ---
> > >  include/linux/migrate.h |  2 +-
> > >  mm/compaction.c         |  2 +-
> > >  mm/migrate.c            | 11 +++++++----
> > >  3 files changed, 9 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> > > index ae8d475..43d5deb 100644
> > > --- a/include/linux/migrate.h
> > > +++ b/include/linux/migrate.h
> > > @@ -37,7 +37,7 @@ extern int migrate_page(struct address_space *,
> > >  			struct page *, struct page *, enum migrate_mode);
> > >  extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free,
> > >  		unsigned long private, enum migrate_mode mode, int reason);
> > > -extern bool isolate_movable_page(struct page *page, isolate_mode_t mode);
> > > +extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
> > >  extern void putback_movable_page(struct page *page);
> > >  
> > >  extern int migrate_prep(void);
> > > diff --git a/mm/compaction.c b/mm/compaction.c
> > > index 949198d..1d89147 100644
> > > --- a/mm/compaction.c
> > > +++ b/mm/compaction.c
> > > @@ -802,7 +802,7 @@ static bool too_many_isolated(struct zone *zone)
> > >  					locked = false;
> > >  				}
> > >  
> > > -				if (isolate_movable_page(page, isolate_mode))
> > > +				if (!isolate_movable_page(page, isolate_mode))
> > >  					goto isolate_success;
> > >  			}
> > >  
> > > diff --git a/mm/migrate.c b/mm/migrate.c
> > > index 87f4d0f..bbbd170 100644
> > > --- a/mm/migrate.c
> > > +++ b/mm/migrate.c
> > > @@ -74,8 +74,9 @@ int migrate_prep_local(void)
> > >  	return 0;
> > >  }
> > >  
> > > -bool isolate_movable_page(struct page *page, isolate_mode_t mode)
> > > +int isolate_movable_page(struct page *page, isolate_mode_t mode)
> > >  {
> > > +	int ret = -EBUSY;
> > >  	struct address_space *mapping;
> > >  
> > >  	/*
> > > @@ -95,8 +96,10 @@ bool isolate_movable_page(struct page *page, isolate_mode_t mode)
> > >  	 * assumes anybody doesn't touch PG_lock of newly allocated page
> > >  	 * so unconditionally grapping the lock ruins page's owner side.
> > >  	 */
> > > -	if (unlikely(!__PageMovable(page)))
> > > +	if (unlikely(!__PageMovable(page))) {
> > > +		ret = -EINVAL;
> > >  		goto out_putpage;
> > > +	}
> > >  	/*
> > >  	 * As movable pages are not isolated from LRU lists, concurrent
> > >  	 * compaction threads can race against page migration functions
> > > @@ -125,14 +128,14 @@ bool isolate_movable_page(struct page *page, isolate_mode_t mode)
> > >  	__SetPageIsolated(page);
> > >  	unlock_page(page);
> > >  
> > > -	return true;
> > > +	return 0;
> > >  
> > >  out_no_isolated:
> > >  	unlock_page(page);
> > >  out_putpage:
> > >  	put_page(page);
> > >  out:
> > > -	return false;
> > > +	return ret;
> > >  }
> > >  
> > >  /* It should be called on page which is PG_movable */
> > > -- 
> > > 1.9.1
> > > 
> > > --
> > > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > > the body to majordomo@xxxxxxxxxx  For more info on Linux MM,
> > > see: http://www.linux-mm.org/ .
> > > Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
> 
> -- 
> Michal Hocko
> SUSE Labs
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxxx  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>