Re: [PATCH 6/9] mm: compaction: Acquire the zone-lock as late as possible
On Fri, Sep 21, 2012 at 11:46:20AM +0100, Mel Gorman wrote: Compactions free scanner acquires the zone-lock when checking for PageBuddy pages and isolating them. It does this even if there are no PageBuddy pages in the range. This patch defers acquiring the zone lock for as long as possible. In the event there are no free pages in the pageblock then the lock will not be acquired at all which reduces contention on zone-lock. Signed-off-by: Mel Gorman mgor...@suse.de Acked-by: Rik van Riel r...@redhat.com Acked-by: Minchan Kim minc...@kernel.org -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] mm: compaction: Acquire the zone-lock as late as possible
On Mon, Sep 24, 2012 at 09:52:38AM +0100, Mel Gorman wrote:
On Fri, Sep 21, 2012 at 02:35:57PM -0700, Andrew Morton wrote:
On Fri, 21 Sep 2012 11:46:20 +0100
Mel Gorman mgor...@suse.de wrote:
Compactions free scanner acquires the zone-lock when checking for
PageBuddy
pages and isolating them. It does this even if there are no PageBuddy
pages
in the range.
This patch defers acquiring the zone lock for as long as possible. In the
event there are no free pages in the pageblock then the lock will not be
acquired at all which reduces contention on zone-lock.
...
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -93,6 +93,28 @@ static inline bool compact_trylock_irqsave(spinlock_t
*lock,
return compact_checklock_irqsave(lock, flags, false, cc);
}
+/* Returns true if the page is within a block suitable for migration to
*/
+static bool suitable_migration_target(struct page *page)
+{
+
stray newline
Fixed.
+ int migratetype = get_pageblock_migratetype(page);
+
+ /* Don't interfere with memory hot-remove or the min_free_kbytes blocks
*/
+ if (migratetype == MIGRATE_ISOLATE || migratetype == MIGRATE_RESERVE)
+ return false;
+
+ /* If the page is a large free page, then allow migration */
+ if (PageBuddy(page) page_order(page) = pageblock_order)
+ return true;
+
+ /* If the block is MIGRATE_MOVABLE or MIGRATE_CMA, allow migration */
+ if (migrate_async_suitable(migratetype))
+ return true;
+
+ /* Otherwise skip the block */
+ return false;
+}
+
...
@@ -168,23 +193,38 @@ static unsigned long
isolate_freepages_block(unsigned long blockpfn,
int isolated, i;
struct page *page = cursor;
- if (!pfn_valid_within(blockpfn)) {
- if (strict)
- return 0;
- continue;
- }
+ if (!pfn_valid_within(blockpfn))
+ goto strict_check;
nr_scanned++;
- if (!PageBuddy(page)) {
- if (strict)
- return 0;
- continue;
- }
+ if (!PageBuddy(page))
+ goto strict_check;
+
+ /*
+ * The zone lock must be held to isolate freepages. This
+ * unfortunately this is a very coarse lock and can be
this this
Fixed.
+ * heavily contended if there are parallel allocations
+ * or parallel compactions. For async compaction do not
+ * spin on the lock and we acquire the lock as late as
+ * possible.
+ */
+ locked = compact_checklock_irqsave(cc-zone-lock, flags,
+ locked, cc);
+ if (!locked)
+ break;
+
+ /* Recheck this is a suitable migration target under lock */
+ if (!strict !suitable_migration_target(page))
+ break;
+
+ /* Recheck this is a buddy page under lock */
+ if (!PageBuddy(page))
+ goto strict_check;
/* Found a free page, break it into order-0 pages */
isolated = split_free_page(page);
if (!isolated strict)
- return 0;
+ goto strict_check;
total_isolated += isolated;
for (i = 0; i isolated; i++) {
list_add(page-lru, freelist);
@@ -196,9 +236,23 @@ static unsigned long
isolate_freepages_block(unsigned long blockpfn,
blockpfn += isolated - 1;
cursor += isolated - 1;
}
+
+ continue;
+
+strict_check:
+ /* Abort isolation if the caller requested strict isolation */
+ if (strict) {
+ total_isolated = 0;
+ goto out;
+ }
}
trace_mm_compaction_isolate_freepages(nr_scanned, total_isolated);
+
+out:
+ if (locked)
+ spin_unlock_irqrestore(cc-zone-lock, flags);
+
return total_isolated;
}
A a few things about this function.
Would it be cleaner if we did
if (!strict) {
if (!suitable_migration_target(page))
break;
} else {
if (!PageBuddy(page))
goto strict_check;
}
and then remove the test of `strict' from strict_check (which then
should be renamed)?
I was not able to implement what you suggested without breakage.
However, I can do something very similar if strict mode does not bail
out ASAP and instead double checks at the end that everything was
isolated correctly. This does mean that CMAs failure case is slightly
more expensive but that really is a corner
Re: [PATCH 6/9] mm: compaction: Acquire the zone-lock as late as possible
On Fri, Sep 21, 2012 at 02:35:57PM -0700, Andrew Morton wrote:
On Fri, 21 Sep 2012 11:46:20 +0100
Mel Gorman mgor...@suse.de wrote:
Compactions free scanner acquires the zone-lock when checking for PageBuddy
pages and isolating them. It does this even if there are no PageBuddy pages
in the range.
This patch defers acquiring the zone lock for as long as possible. In the
event there are no free pages in the pageblock then the lock will not be
acquired at all which reduces contention on zone-lock.
...
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -93,6 +93,28 @@ static inline bool compact_trylock_irqsave(spinlock_t
*lock,
return compact_checklock_irqsave(lock, flags, false, cc);
}
+/* Returns true if the page is within a block suitable for migration to */
+static bool suitable_migration_target(struct page *page)
+{
+
stray newline
Fixed.
+ int migratetype = get_pageblock_migratetype(page);
+
+ /* Don't interfere with memory hot-remove or the min_free_kbytes blocks
*/
+ if (migratetype == MIGRATE_ISOLATE || migratetype == MIGRATE_RESERVE)
+ return false;
+
+ /* If the page is a large free page, then allow migration */
+ if (PageBuddy(page) page_order(page) = pageblock_order)
+ return true;
+
+ /* If the block is MIGRATE_MOVABLE or MIGRATE_CMA, allow migration */
+ if (migrate_async_suitable(migratetype))
+ return true;
+
+ /* Otherwise skip the block */
+ return false;
+}
+
...
@@ -168,23 +193,38 @@ static unsigned long isolate_freepages_block(unsigned
long blockpfn,
int isolated, i;
struct page *page = cursor;
- if (!pfn_valid_within(blockpfn)) {
- if (strict)
- return 0;
- continue;
- }
+ if (!pfn_valid_within(blockpfn))
+ goto strict_check;
nr_scanned++;
- if (!PageBuddy(page)) {
- if (strict)
- return 0;
- continue;
- }
+ if (!PageBuddy(page))
+ goto strict_check;
+
+ /*
+* The zone lock must be held to isolate freepages. This
+* unfortunately this is a very coarse lock and can be
this this
Fixed.
+* heavily contended if there are parallel allocations
+* or parallel compactions. For async compaction do not
+* spin on the lock and we acquire the lock as late as
+* possible.
+*/
+ locked = compact_checklock_irqsave(cc-zone-lock, flags,
+ locked, cc);
+ if (!locked)
+ break;
+
+ /* Recheck this is a suitable migration target under lock */
+ if (!strict !suitable_migration_target(page))
+ break;
+
+ /* Recheck this is a buddy page under lock */
+ if (!PageBuddy(page))
+ goto strict_check;
/* Found a free page, break it into order-0 pages */
isolated = split_free_page(page);
if (!isolated strict)
- return 0;
+ goto strict_check;
total_isolated += isolated;
for (i = 0; i isolated; i++) {
list_add(page-lru, freelist);
@@ -196,9 +236,23 @@ static unsigned long isolate_freepages_block(unsigned
long blockpfn,
blockpfn += isolated - 1;
cursor += isolated - 1;
}
+
+ continue;
+
+strict_check:
+ /* Abort isolation if the caller requested strict isolation */
+ if (strict) {
+ total_isolated = 0;
+ goto out;
+ }
}
trace_mm_compaction_isolate_freepages(nr_scanned, total_isolated);
+
+out:
+ if (locked)
+ spin_unlock_irqrestore(cc-zone-lock, flags);
+
return total_isolated;
}
A a few things about this function.
Would it be cleaner if we did
if (!strict) {
if (!suitable_migration_target(page))
break;
} else {
if (!PageBuddy(page))
goto strict_check;
}
and then remove the test of `strict' from strict_check (which then
should be renamed)?
I was not able to implement what you suggested without breakage.
However, I can do something very similar if strict mode does not bail
out ASAP and instead double checks at the end that everything was
isolated correctly. This does mean that CMAs failure case is slightly
more expensive but that really is a corner case and I think it's
acceptable if the code is easier to follow as a
[PATCH 6/9] mm: compaction: Acquire the zone-lock as late as possible
Compactions free scanner acquires the zone-lock when checking for PageBuddy
pages and isolating them. It does this even if there are no PageBuddy pages
in the range.
This patch defers acquiring the zone lock for as long as possible. In the
event there are no free pages in the pageblock then the lock will not be
acquired at all which reduces contention on zone-lock.
Signed-off-by: Mel Gorman mgor...@suse.de
Acked-by: Rik van Riel r...@redhat.com
---
mm/compaction.c | 141 ++-
1 file changed, 78 insertions(+), 63 deletions(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index a6068ff..8e56594 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -93,6 +93,28 @@ static inline bool compact_trylock_irqsave(spinlock_t *lock,
return compact_checklock_irqsave(lock, flags, false, cc);
}
+/* 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_ISOLATE || migratetype == MIGRATE_RESERVE)
+ return false;
+
+ /* If the page is a large free page, then allow migration */
+ if (PageBuddy(page) page_order(page) = pageblock_order)
+ return true;
+
+ /* If the block is MIGRATE_MOVABLE or MIGRATE_CMA, allow migration */
+ if (migrate_async_suitable(migratetype))
+ return true;
+
+ /* Otherwise skip the block */
+ return false;
+}
+
static void compact_capture_page(struct compact_control *cc)
{
unsigned long flags;
@@ -153,13 +175,16 @@ static void compact_capture_page(struct compact_control
*cc)
* pages inside of the pageblock (even though it may still end up isolating
* some pages).
*/
-static unsigned long isolate_freepages_block(unsigned long blockpfn,
+static unsigned long isolate_freepages_block(struct compact_control *cc,
+ unsigned long blockpfn,
unsigned long end_pfn,
struct list_head *freelist,
bool strict)
{
int nr_scanned = 0, total_isolated = 0;
struct page *cursor;
+ unsigned long flags;
+ bool locked = false;
cursor = pfn_to_page(blockpfn);
@@ -168,23 +193,38 @@ static unsigned long isolate_freepages_block(unsigned
long blockpfn,
int isolated, i;
struct page *page = cursor;
- if (!pfn_valid_within(blockpfn)) {
- if (strict)
- return 0;
- continue;
- }
+ if (!pfn_valid_within(blockpfn))
+ goto strict_check;
nr_scanned++;
- if (!PageBuddy(page)) {
- if (strict)
- return 0;
- continue;
- }
+ if (!PageBuddy(page))
+ goto strict_check;
+
+ /*
+* The zone lock must be held to isolate freepages. This
+* unfortunately this is a very coarse lock and can be
+* heavily contended if there are parallel allocations
+* or parallel compactions. For async compaction do not
+* spin on the lock and we acquire the lock as late as
+* possible.
+*/
+ locked = compact_checklock_irqsave(cc-zone-lock, flags,
+ locked, cc);
+ if (!locked)
+ break;
+
+ /* Recheck this is a suitable migration target under lock */
+ if (!strict !suitable_migration_target(page))
+ break;
+
+ /* Recheck this is a buddy page under lock */
+ if (!PageBuddy(page))
+ goto strict_check;
/* Found a free page, break it into order-0 pages */
isolated = split_free_page(page);
if (!isolated strict)
- return 0;
+ goto strict_check;
total_isolated += isolated;
for (i = 0; i isolated; i++) {
list_add(page-lru, freelist);
@@ -196,9 +236,23 @@ static unsigned long isolate_freepages_block(unsigned long
blockpfn,
blockpfn += isolated - 1;
cursor += isolated - 1;
}
+
+ continue;
+
+strict_check:
+ /* Abort isolation if the caller requested strict isolation */
+ if (strict) {
+ total_isolated = 0;
+ goto out;
+ }
}
Re: [PATCH 6/9] mm: compaction: Acquire the zone-lock as late as possible
On Fri, Sep 21, 2012 at 11:46:20AM +0100, Mel Gorman wrote: Compactions free scanner acquires the zone-lock when checking for PageBuddy pages and isolating them. It does this even if there are no PageBuddy pages in the range. This patch defers acquiring the zone lock for as long as possible. In the event there are no free pages in the pageblock then the lock will not be acquired at all which reduces contention on zone-lock. Signed-off-by: Mel Gorman mgor...@suse.de Acked-by: Rik van Riel r...@redhat.com --- Acked-by: Rafael Aquini aqu...@redhat.com -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] mm: compaction: Acquire the zone-lock as late as possible
On Fri, 21 Sep 2012 11:46:20 +0100
Mel Gorman mgor...@suse.de wrote:
Compactions free scanner acquires the zone-lock when checking for PageBuddy
pages and isolating them. It does this even if there are no PageBuddy pages
in the range.
This patch defers acquiring the zone lock for as long as possible. In the
event there are no free pages in the pageblock then the lock will not be
acquired at all which reduces contention on zone-lock.
...
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -93,6 +93,28 @@ static inline bool compact_trylock_irqsave(spinlock_t
*lock,
return compact_checklock_irqsave(lock, flags, false, cc);
}
+/* Returns true if the page is within a block suitable for migration to */
+static bool suitable_migration_target(struct page *page)
+{
+
stray newline
+ int migratetype = get_pageblock_migratetype(page);
+
+ /* Don't interfere with memory hot-remove or the min_free_kbytes blocks
*/
+ if (migratetype == MIGRATE_ISOLATE || migratetype == MIGRATE_RESERVE)
+ return false;
+
+ /* If the page is a large free page, then allow migration */
+ if (PageBuddy(page) page_order(page) = pageblock_order)
+ return true;
+
+ /* If the block is MIGRATE_MOVABLE or MIGRATE_CMA, allow migration */
+ if (migrate_async_suitable(migratetype))
+ return true;
+
+ /* Otherwise skip the block */
+ return false;
+}
+
...
@@ -168,23 +193,38 @@ static unsigned long isolate_freepages_block(unsigned
long blockpfn,
int isolated, i;
struct page *page = cursor;
- if (!pfn_valid_within(blockpfn)) {
- if (strict)
- return 0;
- continue;
- }
+ if (!pfn_valid_within(blockpfn))
+ goto strict_check;
nr_scanned++;
- if (!PageBuddy(page)) {
- if (strict)
- return 0;
- continue;
- }
+ if (!PageBuddy(page))
+ goto strict_check;
+
+ /*
+ * The zone lock must be held to isolate freepages. This
+ * unfortunately this is a very coarse lock and can be
this this
+ * heavily contended if there are parallel allocations
+ * or parallel compactions. For async compaction do not
+ * spin on the lock and we acquire the lock as late as
+ * possible.
+ */
+ locked = compact_checklock_irqsave(cc-zone-lock, flags,
+ locked, cc);
+ if (!locked)
+ break;
+
+ /* Recheck this is a suitable migration target under lock */
+ if (!strict !suitable_migration_target(page))
+ break;
+
+ /* Recheck this is a buddy page under lock */
+ if (!PageBuddy(page))
+ goto strict_check;
/* Found a free page, break it into order-0 pages */
isolated = split_free_page(page);
if (!isolated strict)
- return 0;
+ goto strict_check;
total_isolated += isolated;
for (i = 0; i isolated; i++) {
list_add(page-lru, freelist);
@@ -196,9 +236,23 @@ static unsigned long isolate_freepages_block(unsigned
long blockpfn,
blockpfn += isolated - 1;
cursor += isolated - 1;
}
+
+ continue;
+
+strict_check:
+ /* Abort isolation if the caller requested strict isolation */
+ if (strict) {
+ total_isolated = 0;
+ goto out;
+ }
}
trace_mm_compaction_isolate_freepages(nr_scanned, total_isolated);
+
+out:
+ if (locked)
+ spin_unlock_irqrestore(cc-zone-lock, flags);
+
return total_isolated;
}
A a few things about this function.
Would it be cleaner if we did
if (!strict) {
if (!suitable_migration_target(page))
break;
} else {
if (!PageBuddy(page))
goto strict_check;
}
and then remove the test of `strict' from strict_check (which then
should be renamed)?
Which perhaps means that the whole `strict_check' block can go away:
if (!strict) {
if (!suitable_migration_target(page))
break;
} else {
if (!PageBuddy(page)) {
total_isolated = 0;
goto out;
}
Have a think about it? The function is a little straggly.
Secondly, it is correct/desirable to skip the (now misnamed
