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.
+/* 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.
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?
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.
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. * * 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; return page; }