Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1578677 > unrolled thread

[PATCH v2 07/10] mm, compaction: restrict async compaction to pageblocks of same migratetype

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 07/10] mm, compaction: restrict async compaction to pageblocks of same migratetype Vlastimil Babka <vbabka@suse.cz> - 2017-02-10 19:20 +0100
    Re: [PATCH v2 07/10] mm, compaction: restrict async compaction to  pageblocks of same migratetype Mel Gorman <mgorman@techsingularity.net> - 2017-02-13 12:00 +0100
    Re: [PATCH v2 07/10] mm, compaction: restrict async compaction to  pageblocks of same migratetype Johannes Weiner <hannes@cmpxchg.org> - 2017-02-14 21:20 +0100
      Re: [PATCH v2 07/10] mm, compaction: restrict async compaction to  pageblocks of same migratetype Vlastimil Babka <vbabka@suse.cz> - 2017-02-17 17:40 +0100
        Re: [PATCH v2 07/10] mm, compaction: restrict async compaction to  pageblocks of same migratetype Johannes Weiner <hannes@cmpxchg.org> - 2017-02-17 19:30 +0100

#1578677 — [PATCH v2 07/10] mm, compaction: restrict async compaction to pageblocks of same migratetype

The migrate scanner in async compaction is currently limited to MIGRATE_MOVABLE
pageblocks. This is a heuristic intended to reduce latency, based on the
assumption that non-MOVABLE pageblocks are unlikely to contain movable pages.

However, with the exception of THP's, most high-order allocations are not
movable. Should the async compaction succeed, this increases the chance that
the non-MOVABLE allocations will fallback to a MOVABLE pageblock, making the
long-term fragmentation worse.

This patch attempts to help the situation by changing async direct compaction
so that the migrate scanner only scans the pageblocks of the requested
migratetype. If it's a non-MOVABLE type and there are such pageblocks that do
contain movable pages, chances are that the allocation can succeed within one
of such pageblocks, removing the need for a fallback. If that fails, the
subsequent sync attempt will ignore this restriction.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/compaction.c | 11 +++++++++--
 mm/page_alloc.c | 20 +++++++++++++-------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index b7094700712b..84ef44c3b1c9 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -994,10 +994,17 @@ isolate_migratepages_range(struct compact_control *cc, unsigned long start_pfn,
 static bool suitable_migration_source(struct compact_control *cc,
 							struct page *page)
 {
-	if (cc->mode != MIGRATE_ASYNC)
+	int block_mt;
+
+	if ((cc->mode != MIGRATE_ASYNC) || !cc->direct_compaction)
 		return true;
 
-	return is_migrate_movable(get_pageblock_migratetype(page));
+	block_mt = get_pageblock_migratetype(page);
+
+	if (cc->migratetype == MIGRATE_MOVABLE)
+		return is_migrate_movable(block_mt);
+	else
+		return block_mt == cc->migratetype;
 }
 
 /* Returns true if the page is within a block suitable for migration to */
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a7d33818610f..6d9ba640a12d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3523,6 +3523,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 						struct alloc_context *ac)
 {
 	bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
+	const bool costly_order = order > PAGE_ALLOC_COSTLY_ORDER;
 	struct page *page = NULL;
 	unsigned int alloc_flags;
 	unsigned long did_some_progress;
@@ -3572,12 +3573,17 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 
 	/*
 	 * For costly allocations, try direct compaction first, as it's likely
-	 * that we have enough base pages and don't need to reclaim. Don't try
-	 * that for allocations that are allowed to ignore watermarks, as the
-	 * ALLOC_NO_WATERMARKS attempt didn't yet happen.
+	 * that we have enough base pages and don't need to reclaim. For non-
+	 * movable high-order allocations, do that as well, as compaction will
+	 * try prevent permanent fragmentation by migrating from blocks of the
+	 * same migratetype.
+	 * Don't try this for allocations that are allowed to ignore
+	 * watermarks, as the ALLOC_NO_WATERMARKS attempt didn't yet happen.
 	 */
-	if (can_direct_reclaim && order > PAGE_ALLOC_COSTLY_ORDER &&
-		!gfp_pfmemalloc_allowed(gfp_mask)) {
+	if (can_direct_reclaim &&
+			(costly_order ||
+			   (order > 0 && ac->migratetype != MIGRATE_MOVABLE))
+			&& !gfp_pfmemalloc_allowed(gfp_mask)) {
 		page = __alloc_pages_direct_compact(gfp_mask, order,
 						alloc_flags, ac,
 						INIT_COMPACT_PRIORITY,
@@ -3589,7 +3595,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		 * Checks for costly allocations with __GFP_NORETRY, which
 		 * includes THP page fault allocations
 		 */
-		if (gfp_mask & __GFP_NORETRY) {
+		if (costly_order && (gfp_mask & __GFP_NORETRY)) {
 			/*
 			 * If compaction is deferred for high-order allocations,
 			 * it is because sync compaction recently failed. If
@@ -3684,7 +3690,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	 * Do not retry costly high order allocations unless they are
 	 * __GFP_REPEAT
 	 */
-	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
+	if (costly_order && !(gfp_mask & __GFP_REPEAT))
 		goto nopage;
 
 	/* Make sure we know about allocations which stall for too long */
-- 
2.11.0

[toc] | [next] | [standalone]

#1579612 — Re: [PATCH v2 07/10] mm, compaction: restrict async compaction to pageblocks of same migratetype

On Fri, Feb 10, 2017 at 06:23:40PM +0100, Vlastimil Babka wrote:
> The migrate scanner in async compaction is currently limited to MIGRATE_MOVABLE
> pageblocks. This is a heuristic intended to reduce latency, based on the
> assumption that non-MOVABLE pageblocks are unlikely to contain movable pages.
> 
> However, with the exception of THP's, most high-order allocations are not
> movable. Should the async compaction succeed, this increases the chance that
> the non-MOVABLE allocations will fallback to a MOVABLE pageblock, making the
> long-term fragmentation worse.
> 
> This patch attempts to help the situation by changing async direct compaction
> so that the migrate scanner only scans the pageblocks of the requested
> migratetype. If it's a non-MOVABLE type and there are such pageblocks that do
> contain movable pages, chances are that the allocation can succeed within one
> of such pageblocks, removing the need for a fallback. If that fails, the
> subsequent sync attempt will ignore this restriction.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Ok, I really like this idea. The thinking of async originally was to
reduce latency but that was also at the time when THP allocations were
stalling for long periods of time. Now that the default has changed,
this idea makes a lot of sense. A few months ago I would have thought
that this will increase the changes that a high-order allocation for the
stack may have a higher chance of failing but with VMAP_STACK, this is
much less of a concern. It would be very nice to know for this patch if
the number of times the extfrag tracepoint is triggered is reduced or
increased by this patch. Do you have that data?

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1580832 — Re: [PATCH v2 07/10] mm, compaction: restrict async compaction to pageblocks of same migratetype

On Fri, Feb 10, 2017 at 06:23:40PM +0100, Vlastimil Babka wrote:
> The migrate scanner in async compaction is currently limited to MIGRATE_MOVABLE
> pageblocks. This is a heuristic intended to reduce latency, based on the
> assumption that non-MOVABLE pageblocks are unlikely to contain movable pages.
> 
> However, with the exception of THP's, most high-order allocations are not
> movable. Should the async compaction succeed, this increases the chance that
> the non-MOVABLE allocations will fallback to a MOVABLE pageblock, making the
> long-term fragmentation worse.
> 
> This patch attempts to help the situation by changing async direct compaction
> so that the migrate scanner only scans the pageblocks of the requested
> migratetype. If it's a non-MOVABLE type and there are such pageblocks that do
> contain movable pages, chances are that the allocation can succeed within one
> of such pageblocks, removing the need for a fallback. If that fails, the
> subsequent sync attempt will ignore this restriction.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Yes, IMO we should make the async compaction scanner decontaminate
unmovable blocks. This is because we fall back to other-typed blocks
before we reclaim, so any unmovable blocks that aren't perfectly
occupied will fill with greedy page cache (and order-0 doesn't steal
blocks back to make them compactable again). Subsequent unmovable
higher-order allocations in turn are more likely to fall back and
steal more movable blocks.

As long as we have vastly more movable blocks than unmovable blocks,
continuous page cache turnover will counteract this negative trend -
pages are reclaimed mostly from movable blocks and some unmovable
blocks, while new cache allocations are placed into the freed movable
blocks - slowly moving cache out from unmovable blocks into movable
ones. But that effect is independent of the rate of higher-order
allocations and can be overwhelmed, so I think it makes sense to
involve compaction directly in decontamination.

The thing I'm not entirely certain about is the aggressiveness of this
patch. Instead of restricting the async scanner to blocks of the same
migratetype, wouldn't it be better (in terms of allocation latency) to
simply let it compact *all* block types? Maybe changing it to look at
unmovable blocks is enough to curb cross-contamination. Sure there
will still be some, but now we're matching the decontamination rate to
the rate of !movable higher-order allocations and don't just rely on
the independent cache turnover rate, which during higher-order bursts
might not be high enough to prevent an expansion of unmovable blocks.

Does that make sense?

[toc] | [prev] | [next] | [standalone]

#1583588 — Re: [PATCH v2 07/10] mm, compaction: restrict async compaction to pageblocks of same migratetype

On 02/14/2017 09:10 PM, Johannes Weiner wrote:
> On Fri, Feb 10, 2017 at 06:23:40PM +0100, Vlastimil Babka wrote:
>> The migrate scanner in async compaction is currently limited to MIGRATE_MOVABLE
>> pageblocks. This is a heuristic intended to reduce latency, based on the
>> assumption that non-MOVABLE pageblocks are unlikely to contain movable pages.
>> 
>> However, with the exception of THP's, most high-order allocations are not
>> movable. Should the async compaction succeed, this increases the chance that
>> the non-MOVABLE allocations will fallback to a MOVABLE pageblock, making the
>> long-term fragmentation worse.
>> 
>> This patch attempts to help the situation by changing async direct compaction
>> so that the migrate scanner only scans the pageblocks of the requested
>> migratetype. If it's a non-MOVABLE type and there are such pageblocks that do
>> contain movable pages, chances are that the allocation can succeed within one
>> of such pageblocks, removing the need for a fallback. If that fails, the
>> subsequent sync attempt will ignore this restriction.
>> 
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Yes, IMO we should make the async compaction scanner decontaminate
> unmovable blocks. This is because we fall back to other-typed blocks
> before we reclaim,

Which we could change too, patch 9 is a step in that direction.

> so any unmovable blocks that aren't perfectly
> occupied will fill with greedy page cache (and order-0 doesn't steal
> blocks back to make them compactable again).

order-0 allocation can actually steal the block back, the decisions to steal are
based on the order of the free pages in the fallback block, not on the
allocation order. But maybe I'm not sure what exactly you meant here.

> Subsequent unmovable
> higher-order allocations in turn are more likely to fall back and
> steal more movable blocks.

Yes.

> As long as we have vastly more movable blocks than unmovable blocks,
> continuous page cache turnover will counteract this negative trend -
> pages are reclaimed mostly from movable blocks and some unmovable
> blocks, while new cache allocations are placed into the freed movable
> blocks - slowly moving cache out from unmovable blocks into movable
> ones. But that effect is independent of the rate of higher-order
> allocations and can be overwhelmed, so I think it makes sense to
> involve compaction directly in decontamination.

Interesting observation, I agree.

> The thing I'm not entirely certain about is the aggressiveness of this
> patch. Instead of restricting the async scanner to blocks of the same
> migratetype, wouldn't it be better (in terms of allocation latency) to
> simply let it compact *all* block types?

Yes it would help allocation latency, but I'm afraid it will remove most of the
decontamination effect.

> Maybe changing it to look at
> unmovable blocks is enough to curb cross-contamination. Sure there
> will still be some, but now we're matching the decontamination rate to
> the rate of !movable higher-order allocations and don't just rely on
> the independent cache turnover rate, which during higher-order bursts
> might not be high enough to prevent an expansion of unmovable blocks.

The rate of compaction attempts is matched with allocations, but the probability
of compaction scanner being in unmovable block is low when the majority of
blocks are movable. So the decontamination rate is proportional but much smaller.

> Does that make sense?

I guess I can try and look at the stats, but I have doubts.

Thanks for the feedback!

[toc] | [prev] | [next] | [standalone]

#1583639 — Re: [PATCH v2 07/10] mm, compaction: restrict async compaction to pageblocks of same migratetype

On Fri, Feb 17, 2017 at 05:32:00PM +0100, Vlastimil Babka wrote:
> On 02/14/2017 09:10 PM, Johannes Weiner wrote:
> > On Fri, Feb 10, 2017 at 06:23:40PM +0100, Vlastimil Babka wrote:
> >> The migrate scanner in async compaction is currently limited to MIGRATE_MOVABLE
> >> pageblocks. This is a heuristic intended to reduce latency, based on the
> >> assumption that non-MOVABLE pageblocks are unlikely to contain movable pages.
> >> 
> >> However, with the exception of THP's, most high-order allocations are not
> >> movable. Should the async compaction succeed, this increases the chance that
> >> the non-MOVABLE allocations will fallback to a MOVABLE pageblock, making the
> >> long-term fragmentation worse.
> >> 
> >> This patch attempts to help the situation by changing async direct compaction
> >> so that the migrate scanner only scans the pageblocks of the requested
> >> migratetype. If it's a non-MOVABLE type and there are such pageblocks that do
> >> contain movable pages, chances are that the allocation can succeed within one
> >> of such pageblocks, removing the need for a fallback. If that fails, the
> >> subsequent sync attempt will ignore this restriction.
> >> 
> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> > 
> > Yes, IMO we should make the async compaction scanner decontaminate
> > unmovable blocks. This is because we fall back to other-typed blocks
> > before we reclaim,
> 
> Which we could change too, patch 9 is a step in that direction.

Yep, patch 9 looks good to me too, pending data that confirms it.

> > so any unmovable blocks that aren't perfectly
> > occupied will fill with greedy page cache (and order-0 doesn't steal
> > blocks back to make them compactable again).
> 
> order-0 allocation can actually steal the block back, the decisions to steal are
> based on the order of the free pages in the fallback block, not on the
> allocation order. But maybe I'm not sure what exactly you meant here.

No, that was me misreading the code. Scratch what's in parentheses.

> > The thing I'm not entirely certain about is the aggressiveness of this
> > patch. Instead of restricting the async scanner to blocks of the same
> > migratetype, wouldn't it be better (in terms of allocation latency) to
> > simply let it compact *all* block types?
> 
> Yes it would help allocation latency, but I'm afraid it will remove most of the
> decontamination effect.
> 
> > Maybe changing it to look at
> > unmovable blocks is enough to curb cross-contamination. Sure there
> > will still be some, but now we're matching the decontamination rate to
> > the rate of !movable higher-order allocations and don't just rely on
> > the independent cache turnover rate, which during higher-order bursts
> > might not be high enough to prevent an expansion of unmovable blocks.
> 
> The rate of compaction attempts is matched with allocations, but the probability
> of compaction scanner being in unmovable block is low when the majority of
> blocks are movable. So the decontamination rate is proportional but much smaller.

Yeah, you're right. The unmovable blocks would still expand, we'd just
turn it into a logarithmic curve.

> > Does that make sense?
> 
> I guess I can try and look at the stats, but I have doubts.

I don't insist. Your patch is implementing a good thing, we can just
keep an eye out for a change in allocation latencies before spending
time trying to mitigate a potential non-issue.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web