changes for v2
o include more experiment data in cover letter
o deal with vlastimil's comments mostly about commit description on 4/5
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(9%) elapsed time.
Below is the average result of 10 runs on my 4GB quad core system.
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
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, 38 insertions(+), 37 deletions(-)
--
1.7.9.5
It is just for clean-up to reduce code size and improve readability.
There is no functional change.
Acked-by: Vlastimil Babka <[email protected]>
Signed-off-by: Joonsoo Kim <[email protected]>
diff --git a/mm/compaction.c b/mm/compaction.c
index 56536d3..a1a9270 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -553,11 +553,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;
@@ -609,13 +605,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
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.
Additionally, move PageBuddy() check after pageblock unit check,
since pageblock check is the first thing we should do and makes things
more simple.
[[email protected]: rephrase commit description]
Acked-by: Vlastimil Babka <[email protected]>
Signed-off-by: Joonsoo Kim <[email protected]>
diff --git a/mm/compaction.c b/mm/compaction.c
index b1ba297..56536d3 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -520,26 +520,31 @@ 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;
+
+ last_pageblock_nr = pageblock_nr;
+ 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;
+ }
+ }
/* 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
@@ -621,7 +626,6 @@ check_compact_cluster:
next_pageblock:
low_pfn = ALIGN(low_pfn + 1, pageblock_nr_pages) - 1;
- last_pageblock_nr = pageblock_nr;
}
acct_isolated(zone, locked, cc);
--
1.7.9.5
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 <[email protected]>
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
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 simplify
the code. There is no functional changes from this clean-up.
Acked-by: Vlastimil Babka <[email protected]>
Signed-off-by: Joonsoo Kim <[email protected]>
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
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.
Acked-by: Vlastimil Babka <[email protected]>
Signed-off-by: Joonsoo Kim <[email protected]>
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
On 02/14/2014 07:54 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.
>
> 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 <[email protected]>
Acked-by: Vlastimil Babka <[email protected]>
> 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))
>