[PATCH 1/3] balancenuma: put page if avoid migrating page

[PATCH 1/3] balancenuma: put page if avoid migrating page

[Hillf Danton]

If we have to avoid migrating to a node that is nearly full, put page
and return zero.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/mm/migrate.c	Tue Nov 27 20:09:38 2012
+++ b/mm/migrate.c	Tue Nov 27 20:15:00 2012
@@ -1538,9 +1538,11 @@ int numamigrate_isolate_page(pg_data_t *
 			mod_zone_page_state(page_zone(page),
 					NR_ISOLATED_ANON + page_lru,
 					HPAGE_PMD_NR);
+		return 1;
 	}

-	return 1;
+	put_page(page);
+	return 0;
 }

 /*
--

Re: [PATCH 1/3] balancenuma: put page if avoid migrating page

[Mel Gorman]

On Tue, Nov 27, 2012 at 09:23:59PM +0800, Hillf Danton wrote:
> If we have to avoid migrating to a node that is nearly full, put page
> and return zero.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>

Correct. In this series, the bug was actually introduced back in "mm:
migrate: Introduce migrate_misplaced_page()" so I'm working the fix to
be a fix on top of that patch and will ensure it gets carried through
properly in the THP patch that happens at the end of the series. This will
work out better in terms of bisection if the tree gets merged and allows
a partial tree to be properly tested. Can I get your signed off on this
patch please? This would be applied on top of "mm: migrate: Introduce
migrate_misplaced_page()"

Thanks.

---8<---
Subject: [PATCH] mm: migrate: Drop the misplaced pages reference count if the
 target node is full

If we have to avoid migrating to a node that is nearly full, put page
and return zero.

[dhillf@gmail.com: Original patch, signed-off is preferred]
Signed-off-by: Mel Gorman <mgorman@suse.de>
---
 mm/migrate.c |   17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/mm/migrate.c b/mm/migrate.c
index a2c4567..49878d7 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1489,18 +1489,21 @@ int migrate_misplaced_page(struct page *page, int node)
 		}
 		isolated = 1;
 
-		/*
-		 * Page is isolated which takes a reference count so now the
-		 * callers reference can be safely dropped without the page
-		 * disappearing underneath us during migration
-		 */
-		put_page(page);
-
 		page_lru = page_is_file_cache(page);
 		inc_zone_page_state(page, NR_ISOLATED_ANON + page_lru);
 		list_add(&page->lru, &migratepages);
 	}
 
+	/*
+	 * Page is either isolated or there is not enough space on the target
+	 * node. If isolated, then it has taken a reference count and the
+	 * callers reference can be safely dropped without the page
+	 * disappearing underneath us during migration. Otherwise the page is
+	 * not to be migrated but the callers reference should still be
+	 * dropped so it does not leak.
+	 */
+	put_page(page);
+
 	if (isolated) {
 		int nr_remaining;
 

Re: [PATCH 1/3] balancenuma: put page if avoid migrating page

[Mel Gorman]

On Tue, Nov 27, 2012 at 02:07:05PM +0000, Mel Gorman wrote:
> On Tue, Nov 27, 2012 at 09:23:59PM +0800, Hillf Danton wrote:
> > If we have to avoid migrating to a node that is nearly full, put page
> > and return zero.
> > 
> > Signed-off-by: Hillf Danton <dhillf@gmail.com>
> 
> Correct. In this series, the bug was actually introduced back in "mm:
> migrate: Introduce migrate_misplaced_page()" so I'm working the fix to
> be a fix on top of that patch and will ensure it gets carried through
> properly in the THP patch that happens at the end of the series. This will
> work out better in terms of bisection if the tree gets merged and allows
> a partial tree to be properly tested. Can I get your signed off on this
> patch please? This would be applied on top of "mm: migrate: Introduce
> migrate_misplaced_page()"
> 
> Thanks.
> 

After slotting the fix into the right place in the series and resolving
the conflict, the overall diff looks like;

diff --git a/mm/migrate.c b/mm/migrate.c
index b0c1585..ed0bdea 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1515,6 +1515,8 @@ bool numamigrate_update_ratelimit(pg_data_t *pgdat)
 
 int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 {
+	int ret = 0;
+
 	/* Avoid migrating to a node that is nearly full */
 	if (migrate_balanced_pgdat(pgdat, 1)) {
 		int page_lru;
@@ -1524,13 +1526,8 @@ int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 			return 0;
 		}
 
-		/*
-		 * Page is isolated which takes a reference count so now the
-		 * callers reference can be safely dropped without the page
-		 * disappearing underneath us during migration
-		 */
-		put_page(page);
-
+		/* Page is isolated */
+		ret = 1;
 		page_lru = page_is_file_cache(page);
 		if (!PageTransHuge(page))
 			inc_zone_page_state(page, NR_ISOLATED_ANON + page_lru);
@@ -1540,7 +1537,17 @@ int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
 					HPAGE_PMD_NR);
 	}
 
-	return 1;
+	/*
+	 * Page is either isolated or there is not enough space on the target
+	 * node. If isolated, then it has taken a reference count and the
+	 * callers reference can be safely dropped without the page
+	 * disappearing underneath us during migration. Otherwise the page is
+	 * not to be migrated but the callers reference should still be
+	 * dropped so it does not leak.
+	 */
+	put_page(page);
+
+	return ret;
 }
 
 /*

Re: [PATCH 1/3] balancenuma: put page if avoid migrating page

[Hillf Danton]

On 11/27/12, Mel Gorman <mgorman@suse.de> wrote:
> Can I get your signed off on this
> patch please? This would be applied on top of "mm: migrate: Introduce
> migrate_misplaced_page()"

Feel free to do so, Mel.

Best Regards

Hillf

Re: [PATCH 1/3] balancenuma: put page if avoid migrating page

[Hugh Dickins]

On Tue, 27 Nov 2012, Mel Gorman wrote:
> On Tue, Nov 27, 2012 at 02:07:05PM +0000, Mel Gorman wrote:
> > On Tue, Nov 27, 2012 at 09:23:59PM +0800, Hillf Danton wrote:
> > > If we have to avoid migrating to a node that is nearly full, put page
> > > and return zero.
> > > 
> > > Signed-off-by: Hillf Danton <dhillf@gmail.com>
> > 
> > Correct. In this series, the bug was actually introduced back in "mm:
> > migrate: Introduce migrate_misplaced_page()" so I'm working the fix to
> > be a fix on top of that patch and will ensure it gets carried through
> > properly in the THP patch that happens at the end of the series. This will
> > work out better in terms of bisection if the tree gets merged and allows
> > a partial tree to be properly tested. Can I get your signed off on this
> > patch please? This would be applied on top of "mm: migrate: Introduce
> > migrate_misplaced_page()"
> > 
> > Thanks.
> > 
> 
> After slotting the fix into the right place in the series and resolving
> the conflict, the overall diff looks like;

A big thankyou to Hillf for this.

Fixing the missed-putpage leak is nice, but much more important is that
it (either his or your rework) fixes the way numamigrate_isolate_page()'s
return value was claiming to have isolated a page when it had not done so.

I never found time to investigate, but applying the patch shows that was
what caused the page->lru corruptions and crashes (and bad page states)
that I mentioned to you last week.

Hugh

> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index b0c1585..ed0bdea 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1515,6 +1515,8 @@ bool numamigrate_update_ratelimit(pg_data_t *pgdat)
>  
>  int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  {
> +	int ret = 0;
> +
>  	/* Avoid migrating to a node that is nearly full */
>  	if (migrate_balanced_pgdat(pgdat, 1)) {
>  		int page_lru;
> @@ -1524,13 +1526,8 @@ int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  			return 0;
>  		}
>  
> -		/*
> -		 * Page is isolated which takes a reference count so now the
> -		 * callers reference can be safely dropped without the page
> -		 * disappearing underneath us during migration
> -		 */
> -		put_page(page);
> -
> +		/* Page is isolated */
> +		ret = 1;
>  		page_lru = page_is_file_cache(page);
>  		if (!PageTransHuge(page))
>  			inc_zone_page_state(page, NR_ISOLATED_ANON + page_lru);
> @@ -1540,7 +1537,17 @@ int numamigrate_isolate_page(pg_data_t *pgdat, struct page *page)
>  					HPAGE_PMD_NR);
>  	}
>  
> -	return 1;
> +	/*
> +	 * Page is either isolated or there is not enough space on the target
> +	 * node. If isolated, then it has taken a reference count and the
> +	 * callers reference can be safely dropped without the page
> +	 * disappearing underneath us during migration. Otherwise the page is
> +	 * not to be migrated but the callers reference should still be
> +	 * dropped so it does not leak.
> +	 */
> +	put_page(page);
> +
> +	return ret;
>  }
>  
>  /*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>