Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570740 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 1/4] mm/migration: make isolate_movable_page() return int type ysxie@foxmail.com - 2017-01-31 14:30 +0100
    Re: [PATCH v5 1/4] mm/migration: make isolate_movable_page() return  int type Minchan Kim <minchan@kernel.org> - 2017-02-01 07:50 +0100
      Re: [PATCH v5 1/4] mm/migration: make isolate_movable_page() return  int type Michal Hocko <mhocko@kernel.org> - 2017-02-01 09:00 +0100
        Re: [PATCH v5 1/4] mm/migration: make isolate_movable_page() return  int type Minchan Kim <minchan@kernel.org> - 2017-02-01 10:50 +0100
          Re: [PATCH v5 1/4] mm/migration: make isolate_movable_page() return  int type Michal Hocko <mhocko@kernel.org> - 2017-02-01 11:10 +0100
            Re: [PATCH v5 1/4] mm/migration: make isolate_movable_page() return  int type Minchan Kim <minchan@kernel.org> - 2017-02-02 08:40 +0100
              Re: [PATCH v5 1/4] mm/migration: make isolate_movable_page() return  int type Yisheng Xie <xieyisheng1@huawei.com> - 2017-02-03 02:50 +0100
          Re: [PATCH v5 1/4] mm/migration: make isolate_movable_page() return  int type Yisheng Xie <xieyisheng1@huawei.com> - 2017-02-03 02:30 +0100

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

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

[toc] | [next] | [standalone]

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

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>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web