[PATCH] cma: cached pageblock type fixup

[PATCH] cma: cached pageblock type fixup

[Bartlomiej Zolnierkiewicz]

From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Subject: [PATCH] cma: cached pageblock type fixup

CMA pages added to per-cpu pages lists in free_hot_cold_page()
have private field set to MIGRATE_CMA pageblock type .  If this
happes just before start_isolate_page_range() in alloc_contig_range()
changes pageblock type of the page to MIGRATE_ISOLATE it may result
in the cached pageblock type being stale in free_pcppages_bulk()
(which may be triggered by drain_all_pages() in alloc_contig_range()),
page being added to MIGRATE_CMA free list instead of MIGRATE_ISOLATE
one in __free_one_page() and (if the page is reused just before
test_pages_isolated() check) causing alloc_contig_range() failure.

Fix such situation by checking whether pageblock type of the page
changed to MIGRATE_ISOLATE for MIGRATE_CMA type pages in
free_pcppages_bulk() and if so fixup the pageblock type to
MIGRATE_ISOLATE (so the page will be added to MIGRATE_ISOLATE free
list in __free_one_page() and won't be used).

Similar situation can happen if rmqueue_bulk() sets cached pageblock
of the page to MIGRATE_CMA and start_isolate_page_range() is called
before buffered_rmqueue() completes (so the page may used by
get_page_from_freelist() and cause test_pages_isolated() check
failure in alloc_contig_range()).  Fix it in buffered_rmqueue() by
changing the pageblock type of the affected page if needed, freeing
page back to buddy allocator and retrying the allocation.

Please note that even with this patch applied some page allocation
vs alloc_contig_range() races are still possible and may result in
rare test_pages_isolated() failures.

Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Mel Gorman <mgorman@suse.de>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 mm/page_alloc.c |   38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

Index: b/mm/page_alloc.c
===================================================================
--- a/mm/page_alloc.c	2012-05-14 16:19:10.052973990 +0200
+++ b/mm/page_alloc.c	2012-05-15 12:40:54.199127705 +0200
@@ -664,12 +664,24 @@
 			batch_free = to_free;
 
 		do {
+			int mt;
+
 			page = list_entry(list->prev, struct page, lru);
 			/* must delete as __free_one_page list manipulates */
 			list_del(&page->lru);
+
+			mt = page_private(page);
+			/*
+			 * cached MIGRATE_CMA pageblock type may have changed
+			 * during isolation
+			 */
+			if (is_migrate_cma(mt) &&
+			    get_pageblock_migratetype(page) == MIGRATE_ISOLATE)
+				mt = MIGRATE_ISOLATE;
+
 			/* MIGRATE_MOVABLE list may include MIGRATE_RESERVEs */
-			__free_one_page(page, zone, 0, page_private(page));
-			trace_mm_page_pcpu_drain(page, 0, page_private(page));
+			__free_one_page(page, zone, 0, mt);
+			trace_mm_page_pcpu_drain(page, 0, mt);
 		} while (--to_free && --batch_free && !list_empty(list));
 	}
 	__mod_zone_page_state(zone, NR_FREE_PAGES, count);
@@ -1440,6 +1452,7 @@
 	if (likely(order == 0)) {
 		struct per_cpu_pages *pcp;
 		struct list_head *list;
+		int mt;
 
 		local_irq_save(flags);
 		pcp = &this_cpu_ptr(zone->pageset)->pcp;
@@ -1459,6 +1472,27 @@
 
 		list_del(&page->lru);
 		pcp->count--;
+
+		spin_lock(&zone->lock);
+		mt = page_private(page);
+		/*
+		 * cached MIGRATE_CMA pageblock type may have changed
+		 * during isolation
+		 */
+		if ((is_migrate_cma(mt) &&
+		     get_pageblock_migratetype(page) == MIGRATE_ISOLATE) ||
+		    mt == MIGRATE_ISOLATE) {
+			mt = MIGRATE_ISOLATE;
+
+			zone->all_unreclaimable = 0;
+			zone->pages_scanned = 0;
+
+			__free_one_page(page, zone, 0, mt);
+			__mod_zone_page_state(zone, NR_FREE_PAGES, 1);
+			spin_unlock(&zone->lock);
+			goto again;
+		} else
+			spin_unlock(&zone->lock);
 	} else {
 		if (unlikely(gfp_flags & __GFP_NOFAIL)) {
 			/*

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH] cma: cached pageblock type fixup

[Marek Szyprowski]

Hello,

On Wednesday, May 23, 2012 9:22 AM Bartlomiej Zolnierkiewicz wrote:

> CMA pages added to per-cpu pages lists in free_hot_cold_page()
> have private field set to MIGRATE_CMA pageblock type .  If this
> happes just before start_isolate_page_range() in alloc_contig_range()
> changes pageblock type of the page to MIGRATE_ISOLATE it may result
> in the cached pageblock type being stale in free_pcppages_bulk()
> (which may be triggered by drain_all_pages() in alloc_contig_range()),
> page being added to MIGRATE_CMA free list instead of MIGRATE_ISOLATE
> one in __free_one_page() and (if the page is reused just before
> test_pages_isolated() check) causing alloc_contig_range() failure.
> 
> Fix such situation by checking whether pageblock type of the page
> changed to MIGRATE_ISOLATE for MIGRATE_CMA type pages in
> free_pcppages_bulk() and if so fixup the pageblock type to
> MIGRATE_ISOLATE (so the page will be added to MIGRATE_ISOLATE free
> list in __free_one_page() and won't be used).
> 
> Similar situation can happen if rmqueue_bulk() sets cached pageblock
> of the page to MIGRATE_CMA and start_isolate_page_range() is called
> before buffered_rmqueue() completes (so the page may used by
> get_page_from_freelist() and cause test_pages_isolated() check
> failure in alloc_contig_range()).  Fix it in buffered_rmqueue() by
> changing the pageblock type of the affected page if needed, freeing
> page back to buddy allocator and retrying the allocation.
> 
> Please note that even with this patch applied some page allocation
> vs alloc_contig_range() races are still possible and may result in
> rare test_pages_isolated() failures.
> 
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Mel, do you have any suggestions or comments how can we deal with this 
issue?

> ---
>  mm/page_alloc.c |   38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> Index: b/mm/page_alloc.c
> ===================================================================
> --- a/mm/page_alloc.c	2012-05-14 16:19:10.052973990 +0200
> +++ b/mm/page_alloc.c	2012-05-15 12:40:54.199127705 +0200
> @@ -664,12 +664,24 @@
>  			batch_free = to_free;
> 
>  		do {
> +			int mt;
> +
>  			page = list_entry(list->prev, struct page, lru);
>  			/* must delete as __free_one_page list manipulates */
>  			list_del(&page->lru);
> +
> +			mt = page_private(page);
> +			/*
> +			 * cached MIGRATE_CMA pageblock type may have changed
> +			 * during isolation
> +			 */
> +			if (is_migrate_cma(mt) &&
> +			    get_pageblock_migratetype(page) == MIGRATE_ISOLATE)
> +				mt = MIGRATE_ISOLATE;
> +
>  			/* MIGRATE_MOVABLE list may include MIGRATE_RESERVEs */
> -			__free_one_page(page, zone, 0, page_private(page));
> -			trace_mm_page_pcpu_drain(page, 0, page_private(page));
> +			__free_one_page(page, zone, 0, mt);
> +			trace_mm_page_pcpu_drain(page, 0, mt);
>  		} while (--to_free && --batch_free && !list_empty(list));
>  	}
>  	__mod_zone_page_state(zone, NR_FREE_PAGES, count);
> @@ -1440,6 +1452,7 @@
>  	if (likely(order == 0)) {
>  		struct per_cpu_pages *pcp;
>  		struct list_head *list;
> +		int mt;
> 
>  		local_irq_save(flags);
>  		pcp = &this_cpu_ptr(zone->pageset)->pcp;
> @@ -1459,6 +1472,27 @@
> 
>  		list_del(&page->lru);
>  		pcp->count--;
> +
> +		spin_lock(&zone->lock);
> +		mt = page_private(page);
> +		/*
> +		 * cached MIGRATE_CMA pageblock type may have changed
> +		 * during isolation
> +		 */
> +		if ((is_migrate_cma(mt) &&
> +		     get_pageblock_migratetype(page) == MIGRATE_ISOLATE) ||
> +		    mt == MIGRATE_ISOLATE) {
> +			mt = MIGRATE_ISOLATE;
> +
> +			zone->all_unreclaimable = 0;
> +			zone->pages_scanned = 0;
> +
> +			__free_one_page(page, zone, 0, mt);
> +			__mod_zone_page_state(zone, NR_FREE_PAGES, 1);
> +			spin_unlock(&zone->lock);
> +			goto again;
> +		} else
> +			spin_unlock(&zone->lock);
>  	} else {
>  		if (unlikely(gfp_flags & __GFP_NOFAIL)) {
>  			/*

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center



--
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] cma: cached pageblock type fixup

[Mel Gorman]

On Wed, May 23, 2012 at 09:22:00AM +0200, Bartlomiej Zolnierkiewicz wrote:
> From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Subject: [PATCH] cma: cached pageblock type fixup
> 
> CMA pages added to per-cpu pages lists in free_hot_cold_page()
> have private field set to MIGRATE_CMA pageblock type .  If this
> happes just before start_isolate_page_range() in alloc_contig_range()
> changes pageblock type of the page to MIGRATE_ISOLATE it may result
> in the cached pageblock type being stale in free_pcppages_bulk()
> (which may be triggered by drain_all_pages() in alloc_contig_range()),

So what?

The pages get freed to the MIGRATE_CMA region. At worst they will be
used for an allocation request that is compatible with being migrated by
CMA. This will delay the allocation time of alloc_contig_range() but is
hardly critical.

Your fix on the other hand adds another call to get_pageblock_type() to
free_pcppages_bulk which is expensive. The change made to
buffered_rmqueue() is horrific. It takes the per-cpu page allocation
path and adds a spin lock to it which completely defeats the purpose of
having the per-cpu allocation avoid taking locks. This will have a very
heavy impact on performance, particularly on parallel workloads.

As the impact of the race should be marginal and the cost of the fix is
so unbelivably high I'm nacking this patch. If this race is a problem then
it should be handled in alloc_contig_range() not in the allocator fast paths.

-- 
Mel Gorman
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] cma: cached pageblock type fixup

[Bartlomiej Zolnierkiewicz]

On Tuesday 19 June 2012 11:00:15 Mel Gorman wrote:
> On Wed, May 23, 2012 at 09:22:00AM +0200, Bartlomiej Zolnierkiewicz wrote:
> > From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > Subject: [PATCH] cma: cached pageblock type fixup
> > 
> > CMA pages added to per-cpu pages lists in free_hot_cold_page()
> > have private field set to MIGRATE_CMA pageblock type .  If this
> > happes just before start_isolate_page_range() in alloc_contig_range()
> > changes pageblock type of the page to MIGRATE_ISOLATE it may result
> > in the cached pageblock type being stale in free_pcppages_bulk()
> > (which may be triggered by drain_all_pages() in alloc_contig_range()),
> 
> So what?

"page being added to MIGRATE_CMA free list instead of MIGRATE_ISOLATE
one in __free_one_page() and (if the page is reused just before
test_pages_isolated() check) causing alloc_contig_range() failure."

> The pages get freed to the MIGRATE_CMA region. At worst they will be
> used for an allocation request that is compatible with being migrated by
> CMA. This will delay the allocation time of alloc_contig_range() but is
> hardly critical.

There is no waiting on MIGRATE_CMA pages in alloc_contig_range() to
become free again.  If the conversion to MIGRATE_ISOLATE fails then
the whole alloc_contig_range() allocation fails on test_pages_isolated()
check.

> Your fix on the other hand adds another call to get_pageblock_type() to
> free_pcppages_bulk which is expensive. The change made to
> buffered_rmqueue() is horrific. It takes the per-cpu page allocation
> path and adds a spin lock to it which completely defeats the purpose of
> having the per-cpu allocation avoid taking locks. This will have a very
> heavy impact on performance, particularly on parallel workloads.
> 
> As the impact of the race should be marginal and the cost of the fix is
> so unbelivably high I'm nacking this patch. If this race is a problem then
> it should be handled in alloc_contig_range() not in the allocator fast paths.

Do you have any idea how this race can be handled in alloc_contig_range()?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung Poland R&D Center

--
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] cma: cached pageblock type fixup

[Mel Gorman]

On Tue, Jun 19, 2012 at 01:28:50PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Tuesday 19 June 2012 11:00:15 Mel Gorman wrote:
> > On Wed, May 23, 2012 at 09:22:00AM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > > Subject: [PATCH] cma: cached pageblock type fixup
> > > 
> > > CMA pages added to per-cpu pages lists in free_hot_cold_page()
> > > have private field set to MIGRATE_CMA pageblock type .  If this
> > > happes just before start_isolate_page_range() in alloc_contig_range()
> > > changes pageblock type of the page to MIGRATE_ISOLATE it may result
> > > in the cached pageblock type being stale in free_pcppages_bulk()
> > > (which may be triggered by drain_all_pages() in alloc_contig_range()),
> > 
> > So what?
> 
> "page being added to MIGRATE_CMA free list instead of MIGRATE_ISOLATE
> one in __free_one_page() and (if the page is reused just before
> test_pages_isolated() check) causing alloc_contig_range() failure."
> 

And.... so what?

If it's on the wrong free list then it can still be isolated from that
list if it has not been allocated. If it has been allocated then it must
be a MIGRATE_CMA-compatible allocation so the page can be migrated and
the operation retried.

> > The pages get freed to the MIGRATE_CMA region. At worst they will be
> > used for an allocation request that is compatible with being migrated by
> > CMA. This will delay the allocation time of alloc_contig_range() but is
> > hardly critical.
> 
> There is no waiting on MIGRATE_CMA pages in alloc_contig_range() to
> become free again.  If the conversion to MIGRATE_ISOLATE fails then
> the whole alloc_contig_range() allocation fails on test_pages_isolated()
> check.
> 

Then retry the operation if alloc_contig_range() finds it raced.

> > Your fix on the other hand adds another call to get_pageblock_type() to
> > free_pcppages_bulk which is expensive. The change made to
> > buffered_rmqueue() is horrific. It takes the per-cpu page allocation
> > path and adds a spin lock to it which completely defeats the purpose of
> > having the per-cpu allocation avoid taking locks. This will have a very
> > heavy impact on performance, particularly on parallel workloads.
> > 
> > As the impact of the race should be marginal and the cost of the fix is
> > so unbelivably high I'm nacking this patch. If this race is a problem then
> > it should be handled in alloc_contig_range() not in the allocator fast paths.
> 
> Do you have any idea how this race can be handled in alloc_contig_range()?
> 

If the page is on the wrong free list, just isolate it or move it to the
MIGRATE_ISOLATE free list at that point. If it has been allocated then
migrate it and move the resulting free page to the MIGRATE_ISOLATE list.

-- 
Mel Gorman
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] cma: cached pageblock type fixup

[Bartlomiej Zolnierkiewicz]

On Tuesday 19 June 2012 14:00:45 Mel Gorman wrote:
> On Tue, Jun 19, 2012 at 01:28:50PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Tuesday 19 June 2012 11:00:15 Mel Gorman wrote:
> > > On Wed, May 23, 2012 at 09:22:00AM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > > > Subject: [PATCH] cma: cached pageblock type fixup
> > > > 
> > > > CMA pages added to per-cpu pages lists in free_hot_cold_page()
> > > > have private field set to MIGRATE_CMA pageblock type .  If this
> > > > happes just before start_isolate_page_range() in alloc_contig_range()
> > > > changes pageblock type of the page to MIGRATE_ISOLATE it may result
> > > > in the cached pageblock type being stale in free_pcppages_bulk()
> > > > (which may be triggered by drain_all_pages() in alloc_contig_range()),
> > > 
> > > So what?
> > 
> > "page being added to MIGRATE_CMA free list instead of MIGRATE_ISOLATE
> > one in __free_one_page() and (if the page is reused just before
> > test_pages_isolated() check) causing alloc_contig_range() failure."
> > 
> 
> And.... so what?
> 
> If it's on the wrong free list then it can still be isolated from that
> list if it has not been allocated. If it has been allocated then it must
> be a MIGRATE_CMA-compatible allocation so the page can be migrated and
> the operation retried.
> 
> > > The pages get freed to the MIGRATE_CMA region. At worst they will be
> > > used for an allocation request that is compatible with being migrated by
> > > CMA. This will delay the allocation time of alloc_contig_range() but is
> > > hardly critical.
> > 
> > There is no waiting on MIGRATE_CMA pages in alloc_contig_range() to
> > become free again.  If the conversion to MIGRATE_ISOLATE fails then
> > the whole alloc_contig_range() allocation fails on test_pages_isolated()
> > check.
> > 
> 
> Then retry the operation if alloc_contig_range() finds it raced.

Right, my other patch is trying to go in this direction:
http://marc.info/?l=linux-mm&m=133775796822644&w=2

> > > Your fix on the other hand adds another call to get_pageblock_type() to
> > > free_pcppages_bulk which is expensive. The change made to
> > > buffered_rmqueue() is horrific. It takes the per-cpu page allocation
> > > path and adds a spin lock to it which completely defeats the purpose of
> > > having the per-cpu allocation avoid taking locks. This will have a very
> > > heavy impact on performance, particularly on parallel workloads.
> > > 
> > > As the impact of the race should be marginal and the cost of the fix is
> > > so unbelivably high I'm nacking this patch. If this race is a problem then
> > > it should be handled in alloc_contig_range() not in the allocator fast paths.
> > 
> > Do you have any idea how this race can be handled in alloc_contig_range()?
> > 
> 
> If the page is on the wrong free list, just isolate it or move it to the
> MIGRATE_ISOLATE free list at that point. If it has been allocated then
> migrate it and move the resulting free page to the MIGRATE_ISOLATE list.

Thanks, this makes sense but still leaves us with some page allocation vs
alloc_contig_range() races (i.e. pages "in-flight" state being added/removed
to/from pcp lists so not being on the freelists and not being allocated).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung Poland R&D Center

--
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] cma: cached pageblock type fixup

[Mel Gorman]

On Tue, Jun 19, 2012 at 03:04:59PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > If the page is on the wrong free list, just isolate it or move it to the
> > MIGRATE_ISOLATE free list at that point. If it has been allocated then
> > migrate it and move the resulting free page to the MIGRATE_ISOLATE list.
> 
> Thanks, this makes sense but still leaves us with some page allocation vs
> alloc_contig_range() races (i.e. pages "in-flight" state being added/removed
> to/from pcp lists so not being on the freelists and not being allocated).
> 

The races should not be forever or open-ended. Once the pageblock is
marked ISOLATE it should only take one pass to either move it from
MIGRATE_CMA to MIGRATE_ISOLATE free lists or to migrate the page and
free it to the MIGRATE_ISOLATE.

I would be very surprised if this cannot be properly handled in
alloc_contig_range() in a manner that does not wreck the page allocator
fast paths.

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