diff mbox series

[RFC,10/26] mm: page_alloc: allow compaction capturing from larger blocks

Message ID 20230418191313.268131-11-hannes@cmpxchg.org (mailing list archive)
State New
Headers show
Series mm: reliable huge page allocator | expand

Commit Message

Johannes Weiner April 18, 2023, 7:12 p.m. UTC
Currently, capturing only works on matching orders and matching
migratetypes. However, if capturing is initially skipped on the
migratetype, it's possible that merging continues up to a full
pageblock, in which case the migratetype is up for grabs again.

Allow capturing to grab smaller chunks from claimed pageblocks, and
expand the remainder of the block back onto the freelists.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/page_alloc.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Mel Gorman April 21, 2023, 2:14 p.m. UTC | #1
On Tue, Apr 18, 2023 at 03:12:57PM -0400, Johannes Weiner wrote:
> Currently, capturing only works on matching orders and matching
> migratetypes. However, if capturing is initially skipped on the
> migratetype, it's possible that merging continues up to a full
> pageblock, in which case the migratetype is up for grabs again.
> 
> Allow capturing to grab smaller chunks from claimed pageblocks, and
> expand the remainder of the block back onto the freelists.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

No objections other than we're still in the preparation phase and the
series needs to be split. Out of curiousity, how often does this actually
trigger in practice? I ask because superficially, I would expect capture to
happen while pages are being merged and I'm not sure how much this actually
helps. If anything the anomaly would be merging !MOVABLE types, capturing
one pageblock and leaving the adjacent block eligible for splitting as
UNMOVABLE/RECLAIMABLE which is not necessarily desirable.

I nagged about the splitting already but ideally there would be supporting
data for all the incremental improvements before a major modification is made
to fragmentation avoidance. That way, even if the fragmentation avoidance
changes have side-effects, the incremental changes stand alone.
Johannes Weiner April 25, 2023, 3:40 p.m. UTC | #2
On Fri, Apr 21, 2023 at 03:14:47PM +0100, Mel Gorman wrote:
> On Tue, Apr 18, 2023 at 03:12:57PM -0400, Johannes Weiner wrote:
> > Currently, capturing only works on matching orders and matching
> > migratetypes. However, if capturing is initially skipped on the
> > migratetype, it's possible that merging continues up to a full
> > pageblock, in which case the migratetype is up for grabs again.
> > 
> > Allow capturing to grab smaller chunks from claimed pageblocks, and
> > expand the remainder of the block back onto the freelists.
> > 
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> No objections other than we're still in the preparation phase and the
> series needs to be split. Out of curiousity, how often does this actually
> trigger in practice? I ask because superficially, I would expect capture to
> happen while pages are being merged and I'm not sure how much this actually
> helps. If anything the anomaly would be merging !MOVABLE types, capturing
> one pageblock and leaving the adjacent block eligible for splitting as
> UNMOVABLE/RECLAIMABLE which is not necessarily desirable.

Looking at this patch independently, once merging continues to the
full block, a fallback would be allowed to claim it anyway
(can_steal_fallback() returns true). I don't quite see a downside
letting capture apply in this case. The plus is of course avoiding the
indirection through the freelist which risks an opportunist request of
a smaller order fragmenting the block and wasting the contiguity work.

In the context of the full series, this becomes even more
important. Once watermarks are required to be met in MIGRATE_FREE
blocks, and reclaim/compaction recycle full blocks, merging up to
pageblock_order happens all the time - and needs to happen for
allocations to succeed. This applies to all types of direct reclaim:
unmovable request freeing reclaimable/movable blocks, reclaimable
freeing movable blocks, movable freeing reclaimable blocks.

I see your point about smaller orders now always ending the merge at
the pageblock, even when there could be additional merging
opportunities beyond. However, I'm not sure these accidental larger
merges beyond what's needed to fulfill the request at hand are a
preferable aspect over reclaimer fairness, and thus ultimately the
reliability of orders up to the pageblock size.

I'll try to get some numbers for this patch independently, though.
This should manifest in p99 allocation latencies and near-OOM
behavior. Is there anything else you'd want me to look for?

Thanks!
Mel Gorman April 28, 2023, 10:41 a.m. UTC | #3
On Tue, Apr 25, 2023 at 11:40:26AM -0400, Johannes Weiner wrote:
> On Fri, Apr 21, 2023 at 03:14:47PM +0100, Mel Gorman wrote:
> > On Tue, Apr 18, 2023 at 03:12:57PM -0400, Johannes Weiner wrote:
> > > Currently, capturing only works on matching orders and matching
> > > migratetypes. However, if capturing is initially skipped on the
> > > migratetype, it's possible that merging continues up to a full
> > > pageblock, in which case the migratetype is up for grabs again.
> > > 
> > > Allow capturing to grab smaller chunks from claimed pageblocks, and
> > > expand the remainder of the block back onto the freelists.
> > > 
> > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> > 
> > No objections other than we're still in the preparation phase and the
> > series needs to be split. Out of curiousity, how often does this actually
> > trigger in practice? I ask because superficially, I would expect capture to
> > happen while pages are being merged and I'm not sure how much this actually
> > helps. If anything the anomaly would be merging !MOVABLE types, capturing
> > one pageblock and leaving the adjacent block eligible for splitting as
> > UNMOVABLE/RECLAIMABLE which is not necessarily desirable.
> 
> Looking at this patch independently, once merging continues to the
> full block, a fallback would be allowed to claim it anyway
> (can_steal_fallback() returns true). I don't quite see a downside
> letting capture apply in this case. The plus is of course avoiding the
> indirection through the freelist which risks an opportunist request of
> a smaller order fragmenting the block and wasting the contiguity work.
> 
> In the context of the full series, this becomes even more
> important. Once watermarks are required to be met in MIGRATE_FREE
> blocks, and reclaim/compaction recycle full blocks, merging up to
> pageblock_order happens all the time - and needs to happen for
> allocations to succeed. This applies to all types of direct reclaim:
> unmovable request freeing reclaimable/movable blocks, reclaimable
> freeing movable blocks, movable freeing reclaimable blocks.
> 
> I see your point about smaller orders now always ending the merge at
> the pageblock, even when there could be additional merging
> opportunities beyond. However, I'm not sure these accidental larger
> merges beyond what's needed to fulfill the request at hand are a
> preferable aspect over reclaimer fairness, and thus ultimately the
> reliability of orders up to the pageblock size.
> 
> I'll try to get some numbers for this patch independently, though.
> This should manifest in p99 allocation latencies and near-OOM
> behavior. Is there anything else you'd want me to look for?
> 

Any major change in the number of the mm_page_alloc_extfrag trace event
triggering. Also put the patch at the end of the preparation series of
possible or even do it as a separate follow-up patch after the bulk of
the series has been handled. While I cannot 100% convince myself either
way, I wonder if variable high-order allocation requests smaller than a
pageblock could cause premature mixing due to capture.
diff mbox series

Patch

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index cd86f80d7bbe..5ebfcf18537b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1067,10 +1067,10 @@  static inline struct capture_control *task_capc(struct zone *zone)
 }
 
 static inline bool
-compaction_capture(struct capture_control *capc, struct page *page,
-		   int order, int migratetype)
+compaction_capture(struct zone *zone, struct page *page, int order,
+		   int migratetype, struct capture_control *capc)
 {
-	if (!capc || order != capc->cc->order)
+	if (!capc || order < capc->cc->order)
 		return false;
 
 	/* Do not accidentally pollute CMA or isolated regions*/
@@ -1092,6 +1092,9 @@  compaction_capture(struct capture_control *capc, struct page *page,
 		return false;
 	}
 
+	if (order > capc->cc->order)
+		expand(zone, page, capc->cc->order, order, migratetype);
+
 	capc->page = page;
 	return true;
 }
@@ -1103,8 +1106,8 @@  static inline struct capture_control *task_capc(struct zone *zone)
 }
 
 static inline bool
-compaction_capture(struct capture_control *capc, struct page *page,
-		   int order, int migratetype)
+compaction_capture(struct zone *zone, struct page *page, int order,
+		   int migratetype, struct capture_control *capc)
 {
 	return false;
 }
@@ -1180,7 +1183,7 @@  static inline void __free_one_page(struct page *page,
 	while (order < MAX_ORDER - 1) {
 		int buddy_mt;
 
-		if (compaction_capture(capc, page, order, migratetype))
+		if (compaction_capture(zone, page, order, migratetype, capc))
 			return;
 
 		buddy = find_buddy_page_pfn(page, pfn, order, &buddy_pfn);