On Tue, Apr 08, 2025 at 05:22:00PM +0000, Brendan Jackman wrote:
On Mon Apr 7, 2025 at 6:01 PM UTC, Johannes Weiner wrote:
--- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -2194,11 +2194,11 @@ try_to_claim_block(struct zone *zone, struct page *page,
- The use of signed ints for order and current_order is a deliberate
- deviation from the rest of this file, to make the for loop
- condition simpler.
*/
- Return the stolen page, or NULL if none can be found.
This commentary is pretty confusing now, there's a block of text that kinda vaguely applies to the aggregate of __rmqueue_steal(), __rmqueue_fallback() and half of __rmqueue(). I think this new code does a better job of speaking for itself so I think we should just delete this block comment and replace it with some more verbosity elsewhere.
I'm glad you think so, let's remove it then!
+/* Try to claim a whole foreign block, take a page, expand the remainder */
Also on the commentary front, I am not a fan of "foreign" and "native":
"Foreign" is already used in this file to mean NUMA-nonlocal.
We already have "start" and "fallback" being used in identifiers as adjectives to describe the mitegratetype concept.
I wouldn't say those are _better_, "native" and "foreign" might be clearer, but it's not worth introducing inconsistency IMO.
That's a fair point, no objection to renaming them.
static __always_inline struct page * -__rmqueue_fallback(struct zone *zone, int order, int start_migratetype, +__rmqueue_claim(struct zone *zone, int order, int start_migratetype, unsigned int alloc_flags) { struct free_area *area;
[pasting in more context that wasn't in the original diff..]
/* * Find the largest available free page in the other list. This roughly * approximates finding the pageblock with the most free pages, which * would be too costly to do exactly. */ for (current_order = MAX_PAGE_ORDER; current_order >= min_order; --current_order) {
IIUC we could go one step further here and also avoid repeating this iteration? Maybe something for a separate patch though?
That might be worth a test, but agree this should be a separate patch.
AFAICS, in the most common configurations MAX_PAGE_ORDER is only one step above pageblock_order or even the same. It might not be worth the complication.
Anyway, the approach seems like a clear improvement, thanks. I will need to take a closer look at it tomorrow, I've run out of brain juice today.
Much appreciate you taking a look, thanks.
Here's what I got from redistributing the block comment and flipping the terminology:
diff --git i/mm/page_alloc.c w/mm/page_alloc.c index dfb2b3f508af..b8142d605691 100644 --- i/mm/page_alloc.c +++ w/mm/page_alloc.c @@ -2183,21 +2183,13 @@ try_to_claim_block(struct zone *zone, struct page *page, } /*
- Try finding a free buddy page on the fallback list.
- This will attempt to claim a whole pageblock for the requested type
- to ensure grouping of such requests in the future.
- If a whole block cannot be claimed, steal an individual page, regressing to
- __rmqueue_smallest() logic to at least break up as little contiguity as
- possible.
- Try to allocate from some fallback migratetype by claiming the entire block,
*/
- i.e. converting it to the allocation's start migratetype.
- The use of signed ints for order and current_order is a deliberate
- deviation from the rest of this file, to make the for loop
- condition simpler.
-/* Try to claim a whole foreign block, take a page, expand the remainder */ static __always_inline struct page * __rmqueue_claim(struct zone *zone, int order, int start_migratetype, unsigned int alloc_flags) @@ -2247,7 +2239,10 @@ __rmqueue_claim(struct zone *zone, int order, int start_migratetype, return NULL; } -/* Try to steal a single page from a foreign block */ +/*
- Try to steal a single page from some fallback migratetype. Leave the rest of
- the block as its current migratetype, potentially causing fragmentation.
- */
static __always_inline struct page * __rmqueue_steal(struct zone *zone, int order, int start_migratetype) { @@ -2307,7 +2302,9 @@ __rmqueue(struct zone *zone, unsigned int order, int migratetype, } /*
* Try the different freelists, native then foreign.
* First try the freelists of the requested migratetype, then try* fallbacks. Roughly, each fallback stage poses more of a fragmentation* risk.
How about "then try fallback modes with increasing levels of fragmentation risk."
* The fallback logic is expensive and rmqueue_bulk() calls in * a loop with the zone->lock held, meaning the freelists are@@ -2332,7 +2329,7 @@ __rmqueue(struct zone *zone, unsigned int order, int migratetype, case RMQUEUE_CLAIM: page = __rmqueue_claim(zone, order, migratetype, alloc_flags); if (page) {
/* Replenished native freelist, back to normal mode */
/* Replenished requested migratetype's freelist, back to normal mode */ *mode = RMQUEUE_NORMAL;
This line is kind of long now. How about:
/* Replenished preferred freelist, back to normal mode */
But yeah, I like your proposed changes. Would you care to send a proper patch?
Acked-by: Johannes Weiner hannes@cmpxchg.org