[PATCH 1/4] mm/migrate: correct return value of migrate_pages()

[PATCH 1/4] mm/migrate: correct return value of migrate_pages()

[Joonsoo Kim]

migrate_pages() should return number of pages not migrated or error code.
When unmap_and_move return -EAGAIN, outer loop is re-execution without
initialising nr_failed. This makes nr_failed over-counted.

So this patch correct it by initialising nr_failed in outer loop.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

diff --git a/mm/migrate.c b/mm/migrate.c
index 3747fcd..1f59ccc 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1102,6 +1102,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 
 	for(pass = 0; pass < 10 && retry; pass++) {
 		retry = 0;
+		nr_failed = 0;
 
 		list_for_each_entry_safe(page, page2, from, lru) {
 			cond_resched();
-- 
1.7.9.5

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

[PATCH 2/4] mm/mempolicy: correct putback method for isolate pages if failed

[Joonsoo Kim]

queue_pages_range() isolates hugetlbfs pages and putback_lru_pages() can't
handle these. We should change it to putback_movable_pages().

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index eca4a31..6d04d37 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1318,7 +1318,7 @@ static long do_mbind(unsigned long start, unsigned long len,
 		if (nr_failed && (flags & MPOL_MF_STRICT))
 			err = -EIO;
 	} else
-		putback_lru_pages(&pagelist);
+		putback_movable_pages(&pagelist);
 
 	up_write(&mm->mmap_sem);
  mpol_out:
-- 
1.7.9.5

--
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 2/4] mm/mempolicy: correct putback method for isolate pages if failed

[Naoya Horiguchi]

Hi Joonsoo,

On Fri, Dec 06, 2013 at 05:41:48PM +0900, Joonsoo Kim wrote:
> queue_pages_range() isolates hugetlbfs pages and putback_lru_pages() can't
> handle these. We should change it to putback_movable_pages().
> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Nice fix, thanks.
I think that this patch is worth going into -stable 3.12,
because it can break in-use hugepage list.

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index eca4a31..6d04d37 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1318,7 +1318,7 @@ static long do_mbind(unsigned long start, unsigned long len,
>  		if (nr_failed && (flags & MPOL_MF_STRICT))
>  			err = -EIO;
>  	} else
> -		putback_lru_pages(&pagelist);
> +		putback_movable_pages(&pagelist);
>  
>  	up_write(&mm->mmap_sem);
>   mpol_out:
> -- 
> 1.7.9.5
> 
> --
> 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>

[PATCH 3/4] mm/migrate: remove putback_lru_pages, fix comment on putback_movable_pages

[Joonsoo Kim]

Some part of putback_lru_pages() and putback_movable_pages() is
duplicated, so it could confuse us what we should use.
We can remove putback_lru_pages() since it is not really needed now.
This makes us undestand and maintain the code more easily.

And comment on putback_movable_pages() is stale now, so fix it.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index f5096b5..7782b74 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -35,7 +35,6 @@ enum migrate_reason {
 
 #ifdef CONFIG_MIGRATION
 
-extern void putback_lru_pages(struct list_head *l);
 extern void putback_movable_pages(struct list_head *l);
 extern int migrate_page(struct address_space *,
 			struct page *, struct page *, enum migrate_mode);
@@ -59,7 +58,6 @@ extern int migrate_page_move_mapping(struct address_space *mapping,
 #else
 
 static inline void putback_lru_pages(struct list_head *l) {}
-static inline void putback_movable_pages(struct list_head *l) {}
 static inline int migrate_pages(struct list_head *l, new_page_t x,
 		unsigned long private, enum migrate_mode mode, int reason)
 	{ return -ENOSYS; }
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index b7c1716..1debdea 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1569,7 +1569,13 @@ static int __soft_offline_page(struct page *page, int flags)
 		ret = migrate_pages(&pagelist, new_page, MPOL_MF_MOVE_ALL,
 					MIGRATE_SYNC, MR_MEMORY_FAILURE);
 		if (ret) {
-			putback_lru_pages(&pagelist);
+			if (!list_empty(&pagelist)) {
+				list_del(&page->lru);
+				dec_zone_page_state(page, NR_ISOLATED_ANON +
+						page_is_file_cache(page));
+				putback_lru_page(page);
+			}
+
 			pr_info("soft offline: %#lx: migration failed %d, type %lx\n",
 				pfn, ret, page->flags);
 			if (ret > 0)
diff --git a/mm/migrate.c b/mm/migrate.c
index 1f59ccc..8392de4 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -71,28 +71,12 @@ int migrate_prep_local(void)
 }
 
 /*
- * Add isolated pages on the list back to the LRU under page lock
- * to avoid leaking evictable pages back onto unevictable list.
- */
-void putback_lru_pages(struct list_head *l)
-{
-	struct page *page;
-	struct page *page2;
-
-	list_for_each_entry_safe(page, page2, l, lru) {
-		list_del(&page->lru);
-		dec_zone_page_state(page, NR_ISOLATED_ANON +
-				page_is_file_cache(page));
-			putback_lru_page(page);
-	}
-}
-
-/*
  * Put previously isolated pages back onto the appropriate lists
  * from where they were once taken off for compaction/migration.
  *
- * This function shall be used instead of putback_lru_pages(),
- * whenever the isolated pageset has been built by isolate_migratepages_range()
+ * This function shall be used whenever the isolated pageset has been
+ * built from lru, balloon, hugetlbfs page. See isolate_migratepages_range()
+ * and isolate_huge_page().
  */
 void putback_movable_pages(struct list_head *l)
 {
@@ -1697,6 +1681,12 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
 	nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page,
 				     node, MIGRATE_ASYNC, MR_NUMA_MISPLACED);
 	if (nr_remaining) {
+		if (!list_empty(&migratepages)) {
+			list_del(&page->lru);
+			dec_zone_page_state(page, NR_ISOLATED_ANON +
+					page_is_file_cache(page));
+			putback_lru_page(page);
+		}
 		putback_lru_pages(&migratepages);
 		isolated = 0;
 	} else
-- 
1.7.9.5

--
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 3/4] mm/migrate: remove putback_lru_pages, fix comment on putback_movable_pages

[Zhang Yanfei]

Hello

On 12/06/2013 04:41 PM, Joonsoo Kim wrote:
> Some part of putback_lru_pages() and putback_movable_pages() is
> duplicated, so it could confuse us what we should use.
> We can remove putback_lru_pages() since it is not really needed now.
> This makes us undestand and maintain the code more easily.
> 
> And comment on putback_movable_pages() is stale now, so fix it.
> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index f5096b5..7782b74 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -35,7 +35,6 @@ enum migrate_reason {
>  
>  #ifdef CONFIG_MIGRATION
>  
> -extern void putback_lru_pages(struct list_head *l);
>  extern void putback_movable_pages(struct list_head *l);
>  extern int migrate_page(struct address_space *,
>  			struct page *, struct page *, enum migrate_mode);
> @@ -59,7 +58,6 @@ extern int migrate_page_move_mapping(struct address_space *mapping,
>  #else
>  
>  static inline void putback_lru_pages(struct list_head *l) {}

If you want to remove the function, this should be removed, right?

> -static inline void putback_movable_pages(struct list_head *l) {}
>  static inline int migrate_pages(struct list_head *l, new_page_t x,
>  		unsigned long private, enum migrate_mode mode, int reason)
>  	{ return -ENOSYS; }
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index b7c1716..1debdea 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1569,7 +1569,13 @@ static int __soft_offline_page(struct page *page, int flags)
>  		ret = migrate_pages(&pagelist, new_page, MPOL_MF_MOVE_ALL,
>  					MIGRATE_SYNC, MR_MEMORY_FAILURE);
>  		if (ret) {
> -			putback_lru_pages(&pagelist);
> +			if (!list_empty(&pagelist)) {
> +				list_del(&page->lru);
> +				dec_zone_page_state(page, NR_ISOLATED_ANON +
> +						page_is_file_cache(page));
> +				putback_lru_page(page);
> +			}
> +
>  			pr_info("soft offline: %#lx: migration failed %d, type %lx\n",
>  				pfn, ret, page->flags);
>  			if (ret > 0)
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 1f59ccc..8392de4 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -71,28 +71,12 @@ int migrate_prep_local(void)
>  }
>  
>  /*
> - * Add isolated pages on the list back to the LRU under page lock
> - * to avoid leaking evictable pages back onto unevictable list.
> - */
> -void putback_lru_pages(struct list_head *l)
> -{
> -	struct page *page;
> -	struct page *page2;
> -
> -	list_for_each_entry_safe(page, page2, l, lru) {
> -		list_del(&page->lru);
> -		dec_zone_page_state(page, NR_ISOLATED_ANON +
> -				page_is_file_cache(page));
> -			putback_lru_page(page);
> -	}
> -}
> -
> -/*
>   * Put previously isolated pages back onto the appropriate lists
>   * from where they were once taken off for compaction/migration.
>   *
> - * This function shall be used instead of putback_lru_pages(),
> - * whenever the isolated pageset has been built by isolate_migratepages_range()
> + * This function shall be used whenever the isolated pageset has been
> + * built from lru, balloon, hugetlbfs page. See isolate_migratepages_range()
> + * and isolate_huge_page().
>   */
>  void putback_movable_pages(struct list_head *l)
>  {
> @@ -1697,6 +1681,12 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma,
>  	nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page,
>  				     node, MIGRATE_ASYNC, MR_NUMA_MISPLACED);
>  	if (nr_remaining) {
> +		if (!list_empty(&migratepages)) {
> +			list_del(&page->lru);
> +			dec_zone_page_state(page, NR_ISOLATED_ANON +
> +					page_is_file_cache(page));
> +			putback_lru_page(page);
> +		}
>  		putback_lru_pages(&migratepages);
>  		isolated = 0;
>  	} else
> 


-- 
Thanks.
Zhang Yanfei

--
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 3/4] mm/migrate: remove putback_lru_pages, fix comment on putback_movable_pages

[Joonsoo Kim]

2013/12/6 Zhang Yanfei <zhangyanfei@cn.fujitsu.com>:
> Hello
>
> On 12/06/2013 04:41 PM, Joonsoo Kim wrote:
>> Some part of putback_lru_pages() and putback_movable_pages() is
>> duplicated, so it could confuse us what we should use.
>> We can remove putback_lru_pages() since it is not really needed now.
>> This makes us undestand and maintain the code more easily.
>>
>> And comment on putback_movable_pages() is stale now, so fix it.
>>
>> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>>
>> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
>> index f5096b5..7782b74 100644
>> --- a/include/linux/migrate.h
>> +++ b/include/linux/migrate.h
>> @@ -35,7 +35,6 @@ enum migrate_reason {
>>
>>  #ifdef CONFIG_MIGRATION
>>
>> -extern void putback_lru_pages(struct list_head *l);
>>  extern void putback_movable_pages(struct list_head *l);
>>  extern int migrate_page(struct address_space *,
>>                       struct page *, struct page *, enum migrate_mode);
>> @@ -59,7 +58,6 @@ extern int migrate_page_move_mapping(struct address_space *mapping,
>>  #else
>>
>>  static inline void putback_lru_pages(struct list_head *l) {}
>
> If you want to remove the function, this should be removed, right?

Hello, Zhang.

Oop... It's my mistake. I will send v2.
Thanks for finding this.

Thanks.

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

[PATCH 4/4] mm/compaction: respect ignore_skip_hint in update_pageblock_skip

[Joonsoo Kim]

update_pageblock_skip() only fits to compaction which tries to isolate by
pageblock unit. If isolate_migratepages_range() is called by CMA, it try to
isolate regardless of pageblock unit and it don't reference
get_pageblock_skip() by ignore_skip_hint. We should also respect it on
update_pageblock_skip() to prevent from setting the wrong information.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

diff --git a/mm/compaction.c b/mm/compaction.c
index 805165b..f58bcd0 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -134,6 +134,10 @@ static void update_pageblock_skip(struct compact_control *cc,
 			bool migrate_scanner)
 {
 	struct zone *zone = cc->zone;
+
+	if (cc->ignore_skip_hint)
+		return;
+
 	if (!page)
 		return;
 
-- 
1.7.9.5

--
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 4/4] mm/compaction: respect ignore_skip_hint in update_pageblock_skip

[Vlastimil Babka]

On 12/06/2013 09:41 AM, Joonsoo Kim wrote:
> update_pageblock_skip() only fits to compaction which tries to isolate by
> pageblock unit. If isolate_migratepages_range() is called by CMA, it try to
> isolate regardless of pageblock unit and it don't reference
> get_pageblock_skip() by ignore_skip_hint. We should also respect it on
> update_pageblock_skip() to prevent from setting the wrong information.

Yeah, this will also prevent updating cached migrate scanner pfn, which 
makes perfect sense, as cma doesn't read them.

> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> diff --git a/mm/compaction.c b/mm/compaction.c
> index 805165b..f58bcd0 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -134,6 +134,10 @@ static void update_pageblock_skip(struct compact_control *cc,
>   			bool migrate_scanner)
>   {
>   	struct zone *zone = cc->zone;
> +
> +	if (cc->ignore_skip_hint)
> +		return;
> +
>   	if (!page)
>   		return;
>
>

--
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 4/4] mm/compaction: respect ignore_skip_hint in update_pageblock_skip

[Naoya Horiguchi]

On Fri, Dec 06, 2013 at 05:41:50PM +0900, Joonsoo Kim wrote:
> update_pageblock_skip() only fits to compaction which tries to isolate by
> pageblock unit. If isolate_migratepages_range() is called by CMA, it try to
> isolate regardless of pageblock unit and it don't reference
> get_pageblock_skip() by ignore_skip_hint. We should also respect it on
> update_pageblock_skip() to prevent from setting the wrong information.
> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Yes, get/set_pageblock_skip() is irrelevant if ignore_skip_hint is true.
This bug was introduced in v3.7-rc, so this patch would go to stable-3.7
and later.

Reviewed-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>

Thanks,
Naoya

> diff --git a/mm/compaction.c b/mm/compaction.c
> index 805165b..f58bcd0 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -134,6 +134,10 @@ static void update_pageblock_skip(struct compact_control *cc,
>  			bool migrate_scanner)
>  {
>  	struct zone *zone = cc->zone;
> +
> +	if (cc->ignore_skip_hint)
> +		return;
> +
>  	if (!page)
>  		return;
>  
> -- 
> 1.7.9.5
> 
> --
> 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 1/4] mm/migrate: correct return value of migrate_pages()

[Vlastimil Babka]

On 12/06/2013 09:41 AM, Joonsoo Kim wrote:
> migrate_pages() should return number of pages not migrated or error code.
> When unmap_and_move return -EAGAIN, outer loop is re-execution without
> initialising nr_failed. This makes nr_failed over-counted.
>
> So this patch correct it by initialising nr_failed in outer loop.
>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 3747fcd..1f59ccc 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1102,6 +1102,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>
>   	for(pass = 0; pass < 10 && retry; pass++) {
>   		retry = 0;
> +		nr_failed = 0;
>
>   		list_for_each_entry_safe(page, page2, from, lru) {
>   			cond_resched();
>

If I'm reading the code correctly, unmap_and_move() (and 
unmap_and_move_huge_page() as well) deletes all pages from the 'from' 
list, unless it fails with -EAGAIN. So the only pages you see in 
subsequent passes are those that failed with -EAGAIN and those are not 
counted as nr_failed. So there shouldn't be over-count, but your patch 
could result in under-count.

Perhaps a comment somewhere would clarify this.

--
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 1/4] mm/migrate: correct return value of migrate_pages()

[Naoya Horiguchi]

On Fri, Dec 06, 2013 at 03:42:16PM +0100, Vlastimil Babka wrote:
> On 12/06/2013 09:41 AM, Joonsoo Kim wrote:
> >migrate_pages() should return number of pages not migrated or error code.
> >When unmap_and_move return -EAGAIN, outer loop is re-execution without
> >initialising nr_failed. This makes nr_failed over-counted.
> >
> >So this patch correct it by initialising nr_failed in outer loop.
> >
> >Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> >
> >diff --git a/mm/migrate.c b/mm/migrate.c
> >index 3747fcd..1f59ccc 100644
> >--- a/mm/migrate.c
> >+++ b/mm/migrate.c
> >@@ -1102,6 +1102,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> >
> >  	for(pass = 0; pass < 10 && retry; pass++) {
> >  		retry = 0;
> >+		nr_failed = 0;
> >
> >  		list_for_each_entry_safe(page, page2, from, lru) {
> >  			cond_resched();
> >
> 
> If I'm reading the code correctly, unmap_and_move() (and
> unmap_and_move_huge_page() as well) deletes all pages from the
> 'from' list, unless it fails with -EAGAIN. So the only pages you see
> in subsequent passes are those that failed with -EAGAIN and those
> are not counted as nr_failed. So there shouldn't be over-count, but
> your patch could result in under-count.
> 
> Perhaps a comment somewhere would clarify this.

I agree and suggest the one below.
Joonsoo, feel free to append it to your series:)

Thanks,
Naoya Horiguchi
---
From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Date: Fri, 6 Dec 2013 13:08:15 -0500
Subject: [PATCH] migrate: add comment about permanent failure path

Let's add a comment about where the failed page goes to, which makes
code more readable.

Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
 mm/migrate.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index 661ff5f66591..c01caafa0a6f 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1118,7 +1118,12 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
 				nr_succeeded++;
 				break;
 			default:
-				/* Permanent failure */
+				/*
+				 * Permanent failure (-EBUSY, -ENOSYS, etc.):
+				 * unlike -EAGAIN case, the failed page is
+				 * removed from migration page list and not
+				 * retried in the next outer loop.
+				 */
 				nr_failed++;
 				break;
 			}
-- 
1.8.3.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 1/4] mm/migrate: correct return value of migrate_pages()

[Christoph Lameter]

On Fri, 6 Dec 2013, Naoya Horiguchi wrote:

> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Date: Fri, 6 Dec 2013 13:08:15 -0500
> Subject: [PATCH] migrate: add comment about permanent failure path
>
> Let's add a comment about where the failed page goes to, which makes
> code more readable.

Acked-by: Christoph Lameter <cl@linux.com>

--
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 1/4] mm/migrate: correct return value of migrate_pages()

[Joonsoo Kim]

On Fri, Dec 06, 2013 at 01:37:26PM -0500, Naoya Horiguchi wrote:
> On Fri, Dec 06, 2013 at 03:42:16PM +0100, Vlastimil Babka wrote:
> > On 12/06/2013 09:41 AM, Joonsoo Kim wrote:
> > >migrate_pages() should return number of pages not migrated or error code.
> > >When unmap_and_move return -EAGAIN, outer loop is re-execution without
> > >initialising nr_failed. This makes nr_failed over-counted.
> > >
> > >So this patch correct it by initialising nr_failed in outer loop.
> > >
> > >Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > >
> > >diff --git a/mm/migrate.c b/mm/migrate.c
> > >index 3747fcd..1f59ccc 100644
> > >--- a/mm/migrate.c
> > >+++ b/mm/migrate.c
> > >@@ -1102,6 +1102,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
> > >
> > >  	for(pass = 0; pass < 10 && retry; pass++) {
> > >  		retry = 0;
> > >+		nr_failed = 0;
> > >
> > >  		list_for_each_entry_safe(page, page2, from, lru) {
> > >  			cond_resched();
> > >
> > 
> > If I'm reading the code correctly, unmap_and_move() (and
> > unmap_and_move_huge_page() as well) deletes all pages from the
> > 'from' list, unless it fails with -EAGAIN. So the only pages you see
> > in subsequent passes are those that failed with -EAGAIN and those
> > are not counted as nr_failed. So there shouldn't be over-count, but
> > your patch could result in under-count.
> > 
> > Perhaps a comment somewhere would clarify this.
> 
> I agree and suggest the one below.
> Joonsoo, feel free to append it to your series:)
> 
> Thanks,
> Naoya Horiguchi
> ---
> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> Date: Fri, 6 Dec 2013 13:08:15 -0500
> Subject: [PATCH] migrate: add comment about permanent failure path
> 
> Let's add a comment about where the failed page goes to, which makes
> code more readable.
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> ---
>  mm/migrate.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 661ff5f66591..c01caafa0a6f 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1118,7 +1118,12 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  				nr_succeeded++;
>  				break;
>  			default:
> -				/* Permanent failure */
> +				/*
> +				 * Permanent failure (-EBUSY, -ENOSYS, etc.):
> +				 * unlike -EAGAIN case, the failed page is
> +				 * removed from migration page list and not
> +				 * retried in the next outer loop.
> +				 */
>  				nr_failed++;
>  				break;
>  			}
> -- 
> 1.8.3.1

Hello, Naoya.

When I saw this new comment, I found that unmap_and_move_huge_page()
has a bug. It would not remove the page from the list if
hugepage_migration_support() is false.
I will include your patch into my series and send an additional patch to fix
this problem.

Thanks.

--
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 1/4] mm/migrate: correct return value of migrate_pages()

[Christoph Lameter]

On Fri, 6 Dec 2013, Joonsoo Kim wrote:

> migrate_pages() should return number of pages not migrated or error code.
> When unmap_and_move return -EAGAIN, outer loop is re-execution without
> initialising nr_failed. This makes nr_failed over-counted.
>
> So this patch correct it by initialising nr_failed in outer loop.

Well nr_retry is the total number of attempts that got EGAIN. nr_failed is
the total number of failed attempts. You are making nr_failed the number
of pages of the list that have failed to migrated. That syncs with the
description.

> diff --git a/mm/migrate.c b/mm/migrate.c
> index 3747fcd..1f59ccc 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1102,6 +1102,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page,
>
>  	for(pass = 0; pass < 10 && retry; pass++) {
>  		retry = 0;
> +		nr_failed = 0;

The initialization of nr_failed and retry earlier is then useless.

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