[PATCH v5 0/4] HWPOISON: soft offlining for non-lru movable page

[PATCH v5 0/4] HWPOISON: soft offlining for non-lru movable page

[ysxie]

From: Yisheng Xie <xieyisheng1@huawei.com>

Hi Andrew,
Could you please help to abandon the v3 of this patch for it will compile
error with CONFIG_MIGRATION=n, and it also has error path handling problem.
I am so sorry about troubling you.

Hi Michal, Minchan and all,
Could you please help to review it?

Any suggestion is more than welcome. And Thanks for all of you.

After Minchan's commit bda807d44454 ("mm: migrate: support non-lru movable
page migration"), some type of non-lru page like zsmalloc and virtio-balloon
page also support migration.

Therefore, we can:
1) soft offlining no-lru movable pages, which means when memory corrected
errors occur on a non-lru movable page, we can stop to use it by migrating
data onto another page and disable the original (maybe half-broken) one.

2) enable memory hotplug for non-lru movable pages, i.e. we may offline
blocks, which include such pages, by using non-lru page migration.

This patchset is heavily depend on non-lru movable page migration.
--------
v5:
 * change the return type of isolate_movable_page() from bool to int as
   Michal's suggestion.
 * add "enable memory hotplug for non-lru movable pages" to this patchset,
   which also make some change as Michal's suggestion here.

v4:
 * make isolate_movable_page always defined to avoid compile error with
   CONFIG_MIGRATION = n
 * return -EBUSY when isolate_movable_page return false which means failed
   to isolate movable page.

v3:
  * delete some unneed limitation and use !__PageMovable instead of PageLRU
    after isolate page to avoid isolated count mismatch, as Minchan Kim's suggestion.

v2:
 * delete function soft_offline_movable_page() and hanle non-lru movable
   page in __soft_offline_page() as Michal Hocko suggested.

Yisheng Xie (4):
  mm/migration: make isolate_movable_page() return int type
  mm/migration: make isolate_movable_page always defined
  HWPOISON: soft offlining for non-lru movable page
  mm/hotplug: enable memory hotplug for non-lru movable pages

 include/linux/migrate.h |  4 +++-
 mm/compaction.c         |  2 +-
 mm/memory-failure.c     | 26 ++++++++++++++++----------
 mm/memory_hotplug.c     | 28 +++++++++++++++++-----------
 mm/migrate.c            | 11 +++++++----
 mm/page_alloc.c         |  8 ++++++--
 6 files changed, 50 insertions(+), 29 deletions(-)

-- 
1.9.1

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

[ysxie]

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>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
CC: Vlastimil Babka <vbabka@suse.cz>
---
 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

[PATCH v5 2/4] mm/migration: make isolate_movable_page always defined

[ysxie]

From: Yisheng Xie <xieyisheng1@huawei.com>

Define isolate_movable_page as a static inline function when
CONFIG_MIGRATION is not enable. It should return -EBUSY
here which means failed to isolate movable pages.

This patch do not have any functional change but prepare for
later patch.

Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
Acked-by: Minchan Kim <minchan@kernel.org>
Suggested-by: Michal Hocko <mhocko@kernel.org>
Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
CC: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/migrate.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 43d5deb..fa76b51 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -56,6 +56,8 @@ static inline int migrate_pages(struct list_head *l, new_page_t new,
 		free_page_t free, unsigned long private, enum migrate_mode mode,
 		int reason)
 	{ return -ENOSYS; }
+static inline int isolate_movable_page(struct page *page, isolate_mode_t mode)
+	{ return -EBUSY; }
 
 static inline int migrate_prep(void) { return -ENOSYS; }
 static inline int migrate_prep_local(void) { return -ENOSYS; }
-- 
1.9.1
\x11

[PATCH v5 3/4] HWPOISON: soft offlining for non-lru movable page

[ysxie]

From: Yisheng Xie <xieyisheng1@huawei.com>

This patch is to extends soft offlining framework to support
non-lru page, which already support migration after
commit bda807d44454 ("mm: migrate: support non-lru movable page
migration")

When memory corrected errors occur on a non-lru movable page,
we can choose to stop using it by migrating data onto another
page and disable the original (maybe half-broken) one.

Signed-off-by: Yisheng Xie <xieyisheng1@huawei.com>
Suggested-by: Michal Hocko <mhocko@kernel.org>
Suggested-by: Minchan Kim <minchan@kernel.org>
Reviewed-by: Minchan Kim <minchan@kernel.org>
Acked-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
CC: Vlastimil Babka <vbabka@suse.cz>
---
 mm/memory-failure.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index f283c7e..3d0f2fd 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1527,7 +1527,8 @@ static int get_any_page(struct page *page, unsigned long pfn, int flags)
 {
 	int ret = __get_any_page(page, pfn, flags);
 
-	if (ret == 1 && !PageHuge(page) && !PageLRU(page)) {
+	if (ret == 1 && !PageHuge(page) &&
+	    !PageLRU(page) && !__PageMovable(page)) {
 		/*
 		 * Try to free it.
 		 */
@@ -1649,7 +1650,10 @@ static int __soft_offline_page(struct page *page, int flags)
 	 * Try to migrate to a new page instead. migrate.c
 	 * handles a large number of cases for us.
 	 */
-	ret = isolate_lru_page(page);
+	if (PageLRU(page))
+		ret = isolate_lru_page(page);
+	else
+		ret = isolate_movable_page(page, ISOLATE_UNEVICTABLE);
 	/*
 	 * Drop page reference which is came from get_any_page()
 	 * successful isolate_lru_page() already took another one.
@@ -1657,18 +1661,20 @@ static int __soft_offline_page(struct page *page, int flags)
 	put_hwpoison_page(page);
 	if (!ret) {
 		LIST_HEAD(pagelist);
-		inc_node_page_state(page, NR_ISOLATED_ANON +
-					page_is_file_cache(page));
+		/*
+		 * After isolated lru page, the PageLRU will be cleared,
+		 * so use !__PageMovable instead for LRU page's mapping
+		 * cannot have PAGE_MAPPING_MOVABLE.
+		 */
+		if (!__PageMovable(page))
+			inc_node_page_state(page, NR_ISOLATED_ANON +
+						page_is_file_cache(page));
 		list_add(&page->lru, &pagelist);
 		ret = migrate_pages(&pagelist, new_page, NULL, MPOL_MF_MOVE_ALL,
 					MIGRATE_SYNC, MR_MEMORY_FAILURE);
 		if (ret) {
-			if (!list_empty(&pagelist)) {
-				list_del(&page->lru);
-				dec_node_page_state(page, NR_ISOLATED_ANON +
-						page_is_file_cache(page));
-				putback_lru_page(page);
-			}
+			if (!list_empty(&pagelist))
+				putback_movable_pages(&pagelist);
 
 			pr_info("soft offline: %#lx: migration failed %d, type %lx\n",
 				pfn, ret, page->flags);
-- 
1.9.1

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

[Minchan Kim]

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.

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?
Secondly, madvise man page should update?
Thirdly, if a driver fail isolation due to -ENOMEM, it should be
propagated, too?

if we want to propagte detailed error to user, driver's isolate_page
function should return right error.

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.


> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> CC: Vlastimil Babka <vbabka@suse.cz>
> ---
>  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@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>

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

[Michal Hocko]

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.

> Secondly, madvise man page should update?

Why?

> 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

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

> > Cc: Minchan Kim <minchan@kernel.org>
> > Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> > CC: Vlastimil Babka <vbabka@suse.cz>
> > ---
> >  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@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>

-- 
Michal Hocko
SUSE Labs

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

> 
> > 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@kernel.org>
> > > Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> > > CC: Vlastimil Babka <vbabka@suse.cz>
> > > ---
> > >  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@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>
> 
> -- 
> Michal Hocko
> SUSE Labs
> 
> --
> 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>

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

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

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

[Minchan Kim]

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.

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

[Yisheng Xie]

hi Minchan,
Thanks for your reviewing.
On 2017/2/1 17:46, 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.
> 
Sorry, that's my mistake for error understanding code.
You're right. It should be EBUSY if it is in race, I will change it.

Thanks
Yisheng Xie.
>>
>>> 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@kernel.org>
>>>> Cc: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>>>> CC: Vlastimil Babka <vbabka@suse.cz>
>>>> ---
>>>>  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@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>
>>
>> -- 
>> Michal Hocko
>> SUSE Labs
>>
>> --
>> 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>
> 
> --
> 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>
> 
> .
> 

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

[Yisheng Xie]

Hi Minchan
Thanks for reviewing.
On 2017/2/2 15:28, Minchan Kim wrote:
> 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.
> 
Ok, I will remove the -EINVAL.

Thanks.
Yisheng Xie.

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