[PATCH 0/5] compaction related commits

[PATCH 0/5] compaction related commits

[Joonsoo Kim]

This patchset is related to the compaction.

patch 1 fixes contrary implementation of the purpose of compaction.
patch 2~4 are for optimization.
patch 5 is just for clean-up.

I tested this patchset with stress-highalloc benchmark on Mel's mmtest
and cannot find any regression in terms of success rate. And I find
much reduced system time. Below is result of 3 runs.

* Before
time :: stress-highalloc 3276.26 user 740.52 system 1664.79 elapsed
time :: stress-highalloc 3640.71 user 771.32 system 1633.83 elapsed
time :: stress-highalloc 3691.64 user 775.44 system 1638.05 elapsed

avg system: 1645 s

* After
time :: stress-highalloc 3225.51 user 732.40 system 1542.76 elapsed
time :: stress-highalloc 3524.31 user 749.63 system 1512.88 elapsed
time :: stress-highalloc 3610.55 user 757.20 system 1505.70 elapsed

avg system: 1519 s

That is 7% reduced system time.

Thanks.

Joonsoo Kim (5):
  mm/compaction: disallow high-order page for migration target
  mm/compaction: do not call suitable_migration_target() on every page
  mm/compaction: change the timing to check to drop the spinlock
  mm/compaction: check pageblock suitability once per pageblock
  mm/compaction: clean-up code on success of ballon isolation

 mm/compaction.c |   75 +++++++++++++++++++++++++++++--------------------------
 1 file changed, 39 insertions(+), 36 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] mm/compaction: disallow high-order page for migration target

[Joonsoo Kim]

Purpose of compaction is to get a high order page. Currently, if we find
high-order page while searching migration target page, we break it to
order-0 pages and use them as migration target. It is contrary to purpose
of compaction, so disallow high-order page to be used for
migration target.

Additionally, clean-up logic in suitable_migration_target() to simply.
There is no functional changes from this clean-up.

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

diff --git a/mm/compaction.c b/mm/compaction.c
index 3a91a2e..bbe1260 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -217,21 +217,12 @@ static inline bool compact_trylock_irqsave(spinlock_t *lock,
 /* Returns true if the page is within a block suitable for migration to */
 static bool suitable_migration_target(struct page *page)
 {
-	int migratetype = get_pageblock_migratetype(page);
-
-	/* Don't interfere with memory hot-remove or the min_free_kbytes blocks */
-	if (migratetype == MIGRATE_RESERVE)
-		return false;
-
-	if (is_migrate_isolate(migratetype))
-		return false;
-
-	/* If the page is a large free page, then allow migration */
+	/* If the page is a large free page, then disallow migration */
 	if (PageBuddy(page) && page_order(page) >= pageblock_order)
-		return true;
+		return false;
 
 	/* If the block is MIGRATE_MOVABLE or MIGRATE_CMA, allow migration */
-	if (migrate_async_suitable(migratetype))
+	if (migrate_async_suitable(get_pageblock_migratetype(page)))
 		return true;
 
 	/* Otherwise skip the block */
-- 
1.7.9.5

[PATCH 2/5] mm/compaction: do not call suitable_migration_target() on every page

[Joonsoo Kim]

suitable_migration_target() checks that pageblock is suitable for
migration target. In isolate_freepages_block(), it is called on every
page and this is inefficient. So make it called once per pageblock.

suitable_migration_target() also checks if page is highorder or not,
but it's criteria for highorder is pageblock order. So calling it once
within pageblock range has no problem.

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

diff --git a/mm/compaction.c b/mm/compaction.c
index bbe1260..0d821a2 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -245,6 +245,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 	unsigned long nr_strict_required = end_pfn - blockpfn;
 	unsigned long flags;
 	bool locked = false;
+	bool checked_pageblock = false;
 
 	cursor = pfn_to_page(blockpfn);
 
@@ -275,8 +276,16 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
 			break;
 
 		/* Recheck this is a suitable migration target under lock */
-		if (!strict && !suitable_migration_target(page))
-			break;
+		if (!strict && !checked_pageblock) {
+			/*
+			 * We need to check suitability of pageblock only once
+			 * and this isolate_freepages_block() is called with
+			 * pageblock range, so just check once is sufficient.
+			 */
+			checked_pageblock = true;
+			if (!suitable_migration_target(page))
+				break;
+		}
 
 		/* Recheck this is a buddy page under lock */
 		if (!PageBuddy(page))
-- 
1.7.9.5

[PATCH 3/5] mm/compaction: change the timing to check to drop the spinlock

[Joonsoo Kim]

It is odd to drop the spinlock when we scan (SWAP_CLUSTER_MAX - 1) th pfn
page. This may results in below situation while isolating migratepage.

1. try isolate 0x0 ~ 0x200 pfn pages.
2. When low_pfn is 0x1ff, ((low_pfn+1) % SWAP_CLUSTER_MAX) == 0, so drop
the spinlock.
3. Then, to complete isolating, retry to aquire the lock.

I think that it is better to use SWAP_CLUSTER_MAX th pfn for checking
the criteria about dropping the lock. This has no harm 0x0 pfn, because,
at this time, locked variable would be false.

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

diff --git a/mm/compaction.c b/mm/compaction.c
index 0d821a2..b1ba297 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -481,7 +481,7 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 	cond_resched();
 	for (; low_pfn < end_pfn; low_pfn++) {
 		/* give a chance to irqs before checking need_resched() */
-		if (locked && !((low_pfn+1) % SWAP_CLUSTER_MAX)) {
+		if (locked && !(low_pfn % SWAP_CLUSTER_MAX)) {
 			if (should_release_lock(&zone->lru_lock)) {
 				spin_unlock_irqrestore(&zone->lru_lock, flags);
 				locked = false;
-- 
1.7.9.5

[PATCH 4/5] mm/compaction: check pageblock suitability once per pageblock

[Joonsoo Kim]

isolation_suitable() and migrate_async_suitable() is used to be sure
that this pageblock range is fine to be migragted. It isn't needed to
call it on every page. Current code do well if not suitable, but, don't
do well when suitable. It re-check it on every valid pageblock.
This patch fix this situation by updating last_pageblock_nr.

Additionally, move PageBuddy() check after pageblock unit check,
since pageblock check is the first thing we should do and makes things
more simple.

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

diff --git a/mm/compaction.c b/mm/compaction.c
index b1ba297..985b782 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -520,26 +520,32 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 
 		/* If isolation recently failed, do not retry */
 		pageblock_nr = low_pfn >> pageblock_order;
-		if (!isolation_suitable(cc, page))
-			goto next_pageblock;
+		if (last_pageblock_nr != pageblock_nr) {
+			int mt;
+
+			if (!isolation_suitable(cc, page))
+				goto next_pageblock;
+
+			/*
+			 * For async migration, also only scan in MOVABLE
+			 * blocks. Async migration is optimistic to see if
+			 * the minimum amount of work satisfies the allocation
+			 */
+			mt = get_pageblock_migratetype(page);
+			if (!cc->sync && !migrate_async_suitable(mt)) {
+				cc->finished_update_migrate = true;
+				skipped_async_unsuitable = true;
+				goto next_pageblock;
+			}
+
+			last_pageblock_nr = pageblock_nr;
+		}
 
 		/* Skip if free */
 		if (PageBuddy(page))
 			continue;
 
 		/*
-		 * For async migration, also only scan in MOVABLE blocks. Async
-		 * migration is optimistic to see if the minimum amount of work
-		 * satisfies the allocation
-		 */
-		if (!cc->sync && last_pageblock_nr != pageblock_nr &&
-		    !migrate_async_suitable(get_pageblock_migratetype(page))) {
-			cc->finished_update_migrate = true;
-			skipped_async_unsuitable = true;
-			goto next_pageblock;
-		}
-
-		/*
 		 * Check may be lockless but that's ok as we recheck later.
 		 * It's possible to migrate LRU pages and balloon pages
 		 * Skip any other type of page
-- 
1.7.9.5

[PATCH 5/5] mm/compaction: clean-up code on success of ballon isolation

[Joonsoo Kim]

It is just for clean-up to reduce code size and improve readability.
There is no functional change.

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

diff --git a/mm/compaction.c b/mm/compaction.c
index 985b782..7a4e3b7 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -554,11 +554,7 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 			if (unlikely(balloon_page_movable(page))) {
 				if (locked && balloon_page_isolate(page)) {
 					/* Successfully isolated */
-					cc->finished_update_migrate = true;
-					list_add(&page->lru, migratelist);
-					cc->nr_migratepages++;
-					nr_isolated++;
-					goto check_compact_cluster;
+					goto isolate_success;
 				}
 			}
 			continue;
@@ -610,13 +606,14 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
 		VM_BUG_ON(PageTransCompound(page));
 
 		/* Successfully isolated */
-		cc->finished_update_migrate = true;
 		del_page_from_lru_list(page, lruvec, page_lru(page));
+
+isolate_success:
+		cc->finished_update_migrate = true;
 		list_add(&page->lru, migratelist);
 		cc->nr_migratepages++;
 		nr_isolated++;
 
-check_compact_cluster:
 		/* Avoid isolating too much */
 		if (cc->nr_migratepages == COMPACT_CLUSTER_MAX) {
 			++low_pfn;
-- 
1.7.9.5

Re: [PATCH 0/5] compaction related commits

[Vlastimil Babka]

On 02/07/2014 06:08 AM, Joonsoo Kim wrote:
> This patchset is related to the compaction.
> 
> patch 1 fixes contrary implementation of the purpose of compaction.
> patch 2~4 are for optimization.
> patch 5 is just for clean-up.
> 
> I tested this patchset with stress-highalloc benchmark on Mel's mmtest
> and cannot find any regression in terms of success rate. And I find
> much reduced system time. Below is result of 3 runs.

What was the memory size? Mel told me this test shouldn't be run with more than 4GB.

> * Before
> time :: stress-highalloc 3276.26 user 740.52 system 1664.79 elapsed
> time :: stress-highalloc 3640.71 user 771.32 system 1633.83 elapsed
> time :: stress-highalloc 3691.64 user 775.44 system 1638.05 elapsed
> 
> avg system: 1645 s
> 
> * After
> time :: stress-highalloc 3225.51 user 732.40 system 1542.76 elapsed
> time :: stress-highalloc 3524.31 user 749.63 system 1512.88 elapsed
> time :: stress-highalloc 3610.55 user 757.20 system 1505.70 elapsed
> 
> avg system: 1519 s
> 
> That is 7% reduced system time.

Why not post the whole compare-mmtests output? There are more metrics in there and extra
eyes never hurt.

Vlastimil

> Thanks.
> 
> Joonsoo Kim (5):
>   mm/compaction: disallow high-order page for migration target
>   mm/compaction: do not call suitable_migration_target() on every page
>   mm/compaction: change the timing to check to drop the spinlock
>   mm/compaction: check pageblock suitability once per pageblock
>   mm/compaction: clean-up code on success of ballon isolation
> 
>  mm/compaction.c |   75 +++++++++++++++++++++++++++++--------------------------
>  1 file changed, 39 insertions(+), 36 deletions(-)
> 

Re: [PATCH 1/5] mm/compaction: disallow high-order page for migration target

[Vlastimil Babka]

On 02/07/2014 06:08 AM, Joonsoo Kim wrote:
> Purpose of compaction is to get a high order page. Currently, if we find
> high-order page while searching migration target page, we break it to
> order-0 pages and use them as migration target. It is contrary to purpose
> of compaction, so disallow high-order page to be used for
> migration target.

I guess this actually didn't trigger often because with large free blocks available,
compaction shouldn't even be running (unless started manually). But the change makes sense.

> Additionally, clean-up logic in suitable_migration_target() to simply.

simply -> simplify the code.

> There is no functional changes from this clean-up.
> 
> 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 3a91a2e..bbe1260 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -217,21 +217,12 @@ static inline bool compact_trylock_irqsave(spinlock_t *lock,
>  /* Returns true if the page is within a block suitable for migration to */
>  static bool suitable_migration_target(struct page *page)
>  {
> -	int migratetype = get_pageblock_migratetype(page);
> -
> -	/* Don't interfere with memory hot-remove or the min_free_kbytes blocks */
> -	if (migratetype == MIGRATE_RESERVE)
> -		return false;
> -
> -	if (is_migrate_isolate(migratetype))
> -		return false;
> -
> -	/* If the page is a large free page, then allow migration */
> +	/* If the page is a large free page, then disallow migration */
>  	if (PageBuddy(page) && page_order(page) >= pageblock_order)
> -		return true;
> +		return false;
>  
>  	/* If the block is MIGRATE_MOVABLE or MIGRATE_CMA, allow migration */
> -	if (migrate_async_suitable(migratetype))
> +	if (migrate_async_suitable(get_pageblock_migratetype(page)))
>  		return true;
>  
>  	/* Otherwise skip the block */
> 

Re: [PATCH 2/5] mm/compaction: do not call suitable_migration_target() on every page

[Vlastimil Babka]

On 02/07/2014 06:08 AM, Joonsoo Kim wrote:
> suitable_migration_target() checks that pageblock is suitable for
> migration target. In isolate_freepages_block(), it is called on every
> page and this is inefficient. So make it called once per pageblock.

Hmm but in sync compaction, compact_checklock_irqsave() may drop the zone->lock,
reschedule and reacquire it and thus possibly invalidate your previous check. Async
compaction is ok as that will quit immediately. So you could probably communicate that
this happened and invalidate checked_pageblock in such case. Or maybe this would not
happen too enough to worry about rare suboptimal migrations?

Vlastimil

> suitable_migration_target() also checks if page is highorder or not,
> but it's criteria for highorder is pageblock order. So calling it once
> within pageblock range has no problem.
> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index bbe1260..0d821a2 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -245,6 +245,7 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>  	unsigned long nr_strict_required = end_pfn - blockpfn;
>  	unsigned long flags;
>  	bool locked = false;
> +	bool checked_pageblock = false;
>  
>  	cursor = pfn_to_page(blockpfn);
>  
> @@ -275,8 +276,16 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
>  			break;
>  
>  		/* Recheck this is a suitable migration target under lock */
> -		if (!strict && !suitable_migration_target(page))
> -			break;
> +		if (!strict && !checked_pageblock) {
> +			/*
> +			 * We need to check suitability of pageblock only once
> +			 * and this isolate_freepages_block() is called with
> +			 * pageblock range, so just check once is sufficient.
> +			 */
> +			checked_pageblock = true;
> +			if (!suitable_migration_target(page))
> +				break;
> +		}
>  
>  		/* Recheck this is a buddy page under lock */
>  		if (!PageBuddy(page))
> 

Re: [PATCH 3/5] mm/compaction: change the timing to check to drop the spinlock

[Vlastimil Babka]

On 02/07/2014 06:08 AM, Joonsoo Kim wrote:
> It is odd to drop the spinlock when we scan (SWAP_CLUSTER_MAX - 1) th pfn
> page. This may results in below situation while isolating migratepage.
> 
> 1. try isolate 0x0 ~ 0x200 pfn pages.
> 2. When low_pfn is 0x1ff, ((low_pfn+1) % SWAP_CLUSTER_MAX) == 0, so drop
> the spinlock.
> 3. Then, to complete isolating, retry to aquire the lock.
> 
> I think that it is better to use SWAP_CLUSTER_MAX th pfn for checking
> the criteria about dropping the lock. This has no harm 0x0 pfn, because,
> at this time, locked variable would be false.
> 
> 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 0d821a2..b1ba297 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -481,7 +481,7 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
>  	cond_resched();
>  	for (; low_pfn < end_pfn; low_pfn++) {
>  		/* give a chance to irqs before checking need_resched() */
> -		if (locked && !((low_pfn+1) % SWAP_CLUSTER_MAX)) {
> +		if (locked && !(low_pfn % SWAP_CLUSTER_MAX)) {
>  			if (should_release_lock(&zone->lru_lock)) {
>  				spin_unlock_irqrestore(&zone->lru_lock, flags);
>  				locked = false;
> 

Re: [PATCH 4/5] mm/compaction: check pageblock suitability once per pageblock

[Vlastimil Babka]

On 02/07/2014 06:08 AM, Joonsoo Kim wrote:
> isolation_suitable() and migrate_async_suitable() is used to be sure
> that this pageblock range is fine to be migragted. It isn't needed to
> call it on every page. Current code do well if not suitable, but, don't
> do well when suitable. It re-check it on every valid pageblock.
> This patch fix this situation by updating last_pageblock_nr.

It took me a while to understand that the problem with migrate_async_suitable() was the
lack of last_pageblock_nr updates (when the code doesn't go through next_pageblock:
label), while the problem with isolation_suitable() was the lack of doing the test only
when last_pageblock_nr != pageblock_nr (so two different things). How bout making it
clearer in the changelog by replacing the paragraph above with something like:

<snip>
isolation_suitable() and migrate_async_suitable() is used to be sure
that this pageblock range is fine to be migragted. It isn't needed to
call it on every page. Current code do well if not suitable, but, don't
do well when suitable.

1) It re-checks isolation_suitable() on each page of a pageblock that was already
estabilished as suitable.
2) It re-checks migrate_async_suitable() on each page of a pageblock that was not entered
through the next_pageblock: label, because last_pageblock_nr is not otherwise updated.

This patch fixes situation by 1) calling isolation_suitable() only once per pageblock and
2) always updating last_pageblock_nr to the pageblock that was just checked.
</snip>

> Additionally, move PageBuddy() check after pageblock unit check,
> since pageblock check is the first thing we should do and makes things
> more simple.

You should also do this, since it becomes redundant and might only confuse people:

 next_pageblock:
                 low_pfn = ALIGN(low_pfn + 1, pageblock_nr_pages) - 1;
-                last_pageblock_nr = pageblock_nr;


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

With the above resolved, consider the patch to be

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

> diff --git a/mm/compaction.c b/mm/compaction.c
> index b1ba297..985b782 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -520,26 +520,32 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
>  
>  		/* If isolation recently failed, do not retry */
>  		pageblock_nr = low_pfn >> pageblock_order;
> -		if (!isolation_suitable(cc, page))
> -			goto next_pageblock;
> +		if (last_pageblock_nr != pageblock_nr) {
> +			int mt;
> +
> +			if (!isolation_suitable(cc, page))
> +				goto next_pageblock;
> +
> +			/*
> +			 * For async migration, also only scan in MOVABLE
> +			 * blocks. Async migration is optimistic to see if
> +			 * the minimum amount of work satisfies the allocation
> +			 */
> +			mt = get_pageblock_migratetype(page);
> +			if (!cc->sync && !migrate_async_suitable(mt)) {
> +				cc->finished_update_migrate = true;
> +				skipped_async_unsuitable = true;
> +				goto next_pageblock;
> +			}
> +
> +			last_pageblock_nr = pageblock_nr;
> +		}
>  
>  		/* Skip if free */
>  		if (PageBuddy(page))
>  			continue;
>  
>  		/*
> -		 * For async migration, also only scan in MOVABLE blocks. Async
> -		 * migration is optimistic to see if the minimum amount of work
> -		 * satisfies the allocation
> -		 */
> -		if (!cc->sync && last_pageblock_nr != pageblock_nr &&
> -		    !migrate_async_suitable(get_pageblock_migratetype(page))) {
> -			cc->finished_update_migrate = true;
> -			skipped_async_unsuitable = true;
> -			goto next_pageblock;
> -		}
> -
> -		/*
>  		 * Check may be lockless but that's ok as we recheck later.
>  		 * It's possible to migrate LRU pages and balloon pages
>  		 * Skip any other type of page
> 

Re: [PATCH 5/5] mm/compaction: clean-up code on success of ballon isolation

[Vlastimil Babka]

On 02/07/2014 06:08 AM, Joonsoo Kim wrote:
> It is just for clean-up to reduce code size and improve readability.
> There is no functional change.
> 
> 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 985b782..7a4e3b7 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -554,11 +554,7 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
>  			if (unlikely(balloon_page_movable(page))) {
>  				if (locked && balloon_page_isolate(page)) {
>  					/* Successfully isolated */
> -					cc->finished_update_migrate = true;
> -					list_add(&page->lru, migratelist);
> -					cc->nr_migratepages++;
> -					nr_isolated++;
> -					goto check_compact_cluster;
> +					goto isolate_success;
>  				}
>  			}
>  			continue;
> @@ -610,13 +606,14 @@ isolate_migratepages_range(struct zone *zone, struct compact_control *cc,
>  		VM_BUG_ON(PageTransCompound(page));
>  
>  		/* Successfully isolated */
> -		cc->finished_update_migrate = true;
>  		del_page_from_lru_list(page, lruvec, page_lru(page));
> +
> +isolate_success:
> +		cc->finished_update_migrate = true;
>  		list_add(&page->lru, migratelist);
>  		cc->nr_migratepages++;
>  		nr_isolated++;
>  
> -check_compact_cluster:
>  		/* Avoid isolating too much */
>  		if (cc->nr_migratepages == COMPACT_CLUSTER_MAX) {
>  			++low_pfn;
> 

Re: [PATCH 0/5] compaction related commits

[Joonsoo Kim]

On Fri, Feb 07, 2014 at 10:14:26AM +0100, Vlastimil Babka wrote:
> On 02/07/2014 06:08 AM, Joonsoo Kim wrote:
> > This patchset is related to the compaction.
> > 
> > patch 1 fixes contrary implementation of the purpose of compaction.
> > patch 2~4 are for optimization.
> > patch 5 is just for clean-up.
> > 
> > I tested this patchset with stress-highalloc benchmark on Mel's mmtest
> > and cannot find any regression in terms of success rate. And I find
> > much reduced system time. Below is result of 3 runs.
> 
> What was the memory size? Mel told me this test shouldn't be run with more than 4GB.

Yep, I know!
My system is 4GB quad core system.

> > * Before
> > time :: stress-highalloc 3276.26 user 740.52 system 1664.79 elapsed
> > time :: stress-highalloc 3640.71 user 771.32 system 1633.83 elapsed
> > time :: stress-highalloc 3691.64 user 775.44 system 1638.05 elapsed
> > 
> > avg system: 1645 s
> > 
> > * After
> > time :: stress-highalloc 3225.51 user 732.40 system 1542.76 elapsed
> > time :: stress-highalloc 3524.31 user 749.63 system 1512.88 elapsed
> > time :: stress-highalloc 3610.55 user 757.20 system 1505.70 elapsed
> > 
> > avg system: 1519 s
> > 
> > That is 7% reduced system time.

Oops, It should be elapsed time ;)

> Why not post the whole compare-mmtests output? There are more metrics in there and extra
> eyes never hurt.
> 

Reason is that I can't average the result of 10 runs easily, since I'm not
familiar to mmtest. Do you share your method to get the average of 10 runs?
Anyway, I did it manually and below is the average result of 10 runs.

compaction-base+ is based on 3.13.0 with Vlastimil's recent fixes.
compaction-fix+ has this patch series on top of compaction-base+.

Thanks.
							
stress-highalloc	
			3.13.0			3.13.0
			compaction-base+	compaction-fix+
Success 1		14.10				15.00
Success 2		20.20				20.00
Success 3		68.30				73.40
																			
			3.13.0			3.13.0
			compaction-base+	compaction-fix+
User			3486.02				3437.13
System			757.92				741.15
Elapsed			1638.52				1488.32
																			
			3.13.0			3.13.0
			compaction-base+	compaction-fix+
Minor Faults 			172591561		167116621
Major Faults 			     984		     859
Swap Ins 			     743		     653
Swap Outs 			    3657		    3535
Direct pages scanned 		  129742		  127344
Kswapd pages scanned 		 1852277		 1817825
Kswapd pages reclaimed 		 1838000		 1804212
Direct pages reclaimed 		  129719		  127327
Kswapd efficiency 		     98%		     98%
Kswapd velocity 		1130.066		1221.296
Direct efficiency 		     99%		     99%
Direct velocity 		  79.367		  85.585
Percentage direct scans 	      6%		      6%
Zone normal velocity 		 231.829		 246.097
Zone dma32 velocity 		 972.589		1055.158
Zone dma velocity 		   5.015		   5.626
Page writes by reclaim 		    6287		    6534
Page writes file 		    2630		    2999
Page writes anon 		    3657		    3535
Page reclaim immediate 		    2187		    2080
Sector Reads 			 2917808		 2877336
Sector Writes 			11477891		11206722
Page rescued immediate 		       0		       0
Slabs scanned 			 2214118		 2168524
Direct inode steals 		   12181		    9788
Kswapd inode steals 		  144830		  132109
Kswapd skipped wait 		       0		       0
THP fault alloc 		       0		       0
THP collapse alloc 		       0		       0
THP splits 			       0		       0
THP fault fallback 		       0		       0
THP collapse fail 		       0		       0
Compaction stalls 		     738		     714
Compaction success 		     194		     207
Compaction failures 		     543		     507
Page migrate success 		 1806083		 1464014
Page migrate failure 		       0		       0
Compaction pages isolated 	 3873329	 	 3162974
Compaction migrate scanned 	74594862	 	59874420
Compaction free scanned 	125888854	 	110868637
Compaction cost 		    2469		    1998

Re: [PATCH 2/5] mm/compaction: do not call suitable_migration_target() on every page

[Joonsoo Kim]

On Fri, Feb 07, 2014 at 10:36:13AM +0100, Vlastimil Babka wrote:
> On 02/07/2014 06:08 AM, Joonsoo Kim wrote:
> > suitable_migration_target() checks that pageblock is suitable for
> > migration target. In isolate_freepages_block(), it is called on every
> > page and this is inefficient. So make it called once per pageblock.
> 
> Hmm but in sync compaction, compact_checklock_irqsave() may drop the zone->lock,
> reschedule and reacquire it and thus possibly invalidate your previous check. Async
> compaction is ok as that will quit immediately. So you could probably communicate that
> this happened and invalidate checked_pageblock in such case. Or maybe this would not
> happen too enough to worry about rare suboptimal migrations?

So, the result of previous check can be changed only if *this* pageblock's migratetype
is changed while we drop the lock. I guess that this is really rare event, and,
in this case, this pageblock already has mixed migratetype pages, so it has
no serious problem.

Thanks.

Re: [PATCH 4/5] mm/compaction: check pageblock suitability once per pageblock

[Joonsoo Kim]

On Fri, Feb 07, 2014 at 11:30:02AM +0100, Vlastimil Babka wrote:
> On 02/07/2014 06:08 AM, Joonsoo Kim wrote:
> > isolation_suitable() and migrate_async_suitable() is used to be sure
> > that this pageblock range is fine to be migragted. It isn't needed to
> > call it on every page. Current code do well if not suitable, but, don't
> > do well when suitable. It re-check it on every valid pageblock.
> > This patch fix this situation by updating last_pageblock_nr.
> 
> It took me a while to understand that the problem with migrate_async_suitable() was the
> lack of last_pageblock_nr updates (when the code doesn't go through next_pageblock:
> label), while the problem with isolation_suitable() was the lack of doing the test only
> when last_pageblock_nr != pageblock_nr (so two different things). How bout making it
> clearer in the changelog by replacing the paragraph above with something like:

Really nice!!
Sorry for bad description and thanks for taking time to understand it.

> 
> <snip>
> isolation_suitable() and migrate_async_suitable() is used to be sure
> that this pageblock range is fine to be migragted. It isn't needed to
> call it on every page. Current code do well if not suitable, but, don't
> do well when suitable.
> 
> 1) It re-checks isolation_suitable() on each page of a pageblock that was already
> estabilished as suitable.
> 2) It re-checks migrate_async_suitable() on each page of a pageblock that was not entered
> through the next_pageblock: label, because last_pageblock_nr is not otherwise updated.
> 
> This patch fixes situation by 1) calling isolation_suitable() only once per pageblock and
> 2) always updating last_pageblock_nr to the pageblock that was just checked.
> </snip>
> 
> > Additionally, move PageBuddy() check after pageblock unit check,
> > since pageblock check is the first thing we should do and makes things
> > more simple.
> 
> You should also do this, since it becomes redundant and might only confuse people:
> 
>  next_pageblock:
>                  low_pfn = ALIGN(low_pfn + 1, pageblock_nr_pages) - 1;
> -                last_pageblock_nr = pageblock_nr;
> 

Okay.

> > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> With the above resolved, consider the patch to be
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

I will do it.
Thanks!

Re: [PATCH 1/5] mm/compaction: disallow high-order page for migration target

[Mel Gorman]

On Fri, Feb 07, 2014 at 02:08:42PM +0900, Joonsoo Kim wrote:
> Purpose of compaction is to get a high order page. Currently, if we find
> high-order page while searching migration target page, we break it to
> order-0 pages and use them as migration target. It is contrary to purpose
> of compaction, so disallow high-order page to be used for
> migration target.
> 
> Additionally, clean-up logic in suitable_migration_target() to simply.
> There is no functional changes from this clean-up.
> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 3a91a2e..bbe1260 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -217,21 +217,12 @@ static inline bool compact_trylock_irqsave(spinlock_t *lock,
>  /* Returns true if the page is within a block suitable for migration to */
>  static bool suitable_migration_target(struct page *page)
>  {
> -	int migratetype = get_pageblock_migratetype(page);
> -
> -	/* Don't interfere with memory hot-remove or the min_free_kbytes blocks */
> -	if (migratetype == MIGRATE_RESERVE)
> -		return false;
> -

Why is this check removed? The reservation blocks are preserved as
short-lived high-order atomic allocations depend on them.

> -	if (is_migrate_isolate(migratetype))
> -		return false;
> -

Why is this check removed?

> -	/* If the page is a large free page, then allow migration */
> +	/* If the page is a large free page, then disallow migration */
>  	if (PageBuddy(page) && page_order(page) >= pageblock_order)
> -		return true;
> +		return false;
>  

The reason why this was originally allowed was to allow pageblocks that were
marked MIGRATE_UNMOVABLE or MIGRATE_RECLAIMABLE to be used as compaction
targets. However, compaction should not even be running if this is the
case so the change makes sense.

>  	/* If the block is MIGRATE_MOVABLE or MIGRATE_CMA, allow migration */
> -	if (migrate_async_suitable(migratetype))
> +	if (migrate_async_suitable(get_pageblock_migratetype(page)))
>  		return true;
>  
>  	/* Otherwise skip the block */
> -- 
> 1.7.9.5
> 

-- 
Mel Gorman
SUSE Labs

Re: [PATCH 1/5] mm/compaction: disallow high-order page for migration target

[Joonsoo Kim]

On Mon, Feb 10, 2014 at 01:26:34PM +0000, Mel Gorman wrote:
> On Fri, Feb 07, 2014 at 02:08:42PM +0900, Joonsoo Kim wrote:
> > Purpose of compaction is to get a high order page. Currently, if we find
> > high-order page while searching migration target page, we break it to
> > order-0 pages and use them as migration target. It is contrary to purpose
> > of compaction, so disallow high-order page to be used for
> > migration target.
> > 
> > Additionally, clean-up logic in suitable_migration_target() to simply.
> > There is no functional changes from this clean-up.
> > 
> > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > 
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 3a91a2e..bbe1260 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -217,21 +217,12 @@ static inline bool compact_trylock_irqsave(spinlock_t *lock,
> >  /* Returns true if the page is within a block suitable for migration to */
> >  static bool suitable_migration_target(struct page *page)
> >  {
> > -	int migratetype = get_pageblock_migratetype(page);
> > -
> > -	/* Don't interfere with memory hot-remove or the min_free_kbytes blocks */
> > -	if (migratetype == MIGRATE_RESERVE)
> > -		return false;
> > -
> 
> Why is this check removed? The reservation blocks are preserved as
> short-lived high-order atomic allocations depend on them.

Hello,

After disallowing high-order page to be used for migration target,
we only allow pages from movable or CMA pageblock for migration target on
migrate_async_suitable() check. So checking whether page comes from reserve or
isolate pageblock is useless.

> 
> > -	if (is_migrate_isolate(migratetype))
> > -		return false;
> > -
> 
> Why is this check removed?
> 
> > -	/* If the page is a large free page, then allow migration */
> > +	/* If the page is a large free page, then disallow migration */
> >  	if (PageBuddy(page) && page_order(page) >= pageblock_order)
> > -		return true;
> > +		return false;
> >  
> 
> The reason why this was originally allowed was to allow pageblocks that were
> marked MIGRATE_UNMOVABLE or MIGRATE_RECLAIMABLE to be used as compaction
> targets. However, compaction should not even be running if this is the
> case so the change makes sense.

Okay!

Thanks.