Re: [PATCH 2/2] mm: page_alloc: tighten up find_suitable_fallback()

[Brendan Jackman <jackmanb@google.com>]

On Fri Apr 11, 2025 at 1:45 PM UTC, Johannes Weiner wrote:
>> -		if (find_suitable_fallback(area, order, migratetype, true) >= 0)
>> +		if (should_try_claim_block(order, migratetype) &&
>> +		    find_fallback_migratetype(area, order, migratetype) >= 0)
>
> So I agree with pushing the test into the callers. However, I think
> the name "should_try_claim_block()" is not great for this. It makes
> sense in the alloc/fallback path, but compaction here doesn't claim
> anything. It just wants to know if this order + migratetype is
> eligible under block claiming rules.
>
> IMO this would be more readable with the old terminology:
>
> 		if (can_claim_block(order, migratetype) &&
> 		    find_fallback_migratetype(area, order, migratetype) >= 0)

Sure, that makes sense, here's a modified version of the patch:

---

From 85be0fca4627c5b832a3382c92b6310609e14ca4 Mon Sep 17 00:00:00 2001
From: Brendan Jackman <jackmanb@google.com>
Date: Thu, 10 Apr 2025 13:22:58 +0000
Subject: [PATCH] mm: page_alloc: Split up find_suitable_fallback()

Now that it's been simplified, it's clear that the bool arg isn't
needed, callers can just use should_try_claim_block(). Once that logic
is stripped out, the function becomes very obvious and can get a more
straightforward name and comment.

Since should_try_claim_block() is now exported to compaction.c, give it
a name that makes more sense outside the context of allocation -
should_claim_block() seems confusing in code that has no interest in
actually claiming a block.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 mm/compaction.c |  3 ++-
 mm/internal.h   |  5 +++--
 mm/page_alloc.c | 33 +++++++++++++--------------------
 3 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 39a4d178dff3c..0528996c40507 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2363,7 +2363,8 @@ static enum compact_result __compact_finished(struct compact_control *cc)
 		 * Job done if allocation would steal freepages from
 		 * other migratetype buddy lists.
 		 */
-		if (find_suitable_fallback(area, order, migratetype, true) >= 0)
+		if (can_claim_block(order, migratetype) &&
+		    find_fallback_migratetype(area, order, migratetype) >= 0)
 			/*
 			 * Movable pages are OK in any pageblock. If we are
 			 * stealing for a non-movable allocation, make sure
diff --git a/mm/internal.h b/mm/internal.h
index 4e0ea83aaf1c8..5450ea7f5b1ec 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -914,8 +914,9 @@ static inline void init_cma_pageblock(struct page *page)
 #endif
 
 
-int find_suitable_fallback(struct free_area *area, unsigned int order,
-			   int migratetype, bool claimable);
+int find_fallback_migratetype(struct free_area *area, unsigned int order,
+			      int migratetype);
+bool can_claim_block(unsigned int order, int start_mt);
 
 static inline bool free_area_empty(struct free_area *area, int migratetype)
 {
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0a1f28bf5255c..c27a106ec5985 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2034,7 +2034,7 @@ static inline bool boost_watermark(struct zone *zone)
  * try to claim an entire block to satisfy further allocations, instead of
  * polluting multiple pageblocks?
  */
-static bool should_try_claim_block(unsigned int order, int start_mt)
+bool can_claim_block(unsigned int order, int start_mt)
 {
 	/*
 	 * Leaving this order check is intended, although there is
@@ -2076,20 +2076,12 @@ static bool should_try_claim_block(unsigned int order, int start_mt)
 	return false;
 }
 
-/*
- * Check whether there is a suitable fallback freepage with requested order.
- * If claimable is true, this function returns fallback_mt only if
- * we would do this whole-block claiming. This would help to reduce
- * fragmentation due to mixed migratetype pages in one pageblock.
- */
-int find_suitable_fallback(struct free_area *area, unsigned int order,
-			   int migratetype, bool claimable)
+/* Find a fallback migratetype with at least one page of the given order. */
+int find_fallback_migratetype(struct free_area *area, unsigned int order,
+			      int migratetype)
 {
 	int i;
 
-	if (claimable && !should_try_claim_block(order, migratetype))
-		return -2;
-
 	if (area->nr_free == 0)
 		return -1;
 
@@ -2209,18 +2201,19 @@ __rmqueue_claim(struct zone *zone, int order, int start_migratetype,
 	 */
 	for (current_order = MAX_PAGE_ORDER; current_order >= min_order;
 				--current_order) {
+
+		/* Advanced into orders too low to claim, abort */
+		if (!can_claim_block(order, start_migratetype))
+			break;
+
 		area = &(zone->free_area[current_order]);
-		fallback_mt = find_suitable_fallback(area, current_order,
-						     start_migratetype, true);
+		fallback_mt = find_fallback_migratetype(area, current_order,
+							start_migratetype);
 
 		/* No block in that order */
 		if (fallback_mt == -1)
 			continue;
 
-		/* Advanced into orders too low to claim, abort */
-		if (fallback_mt == -2)
-			break;
-
 		page = get_page_from_free_area(area, fallback_mt);
 		page = try_to_claim_block(zone, page, current_order, order,
 					  start_migratetype, fallback_mt,
@@ -2249,8 +2242,8 @@ __rmqueue_steal(struct zone *zone, int order, int start_migratetype)
 
 	for (current_order = order; current_order < NR_PAGE_ORDERS; current_order++) {
 		area = &(zone->free_area[current_order]);
-		fallback_mt = find_suitable_fallback(area, current_order,
-						     start_migratetype, false);
+		fallback_mt = find_fallback_migratetype(area, current_order,
+							start_migratetype);
 		if (fallback_mt == -1)
 			continue;
 
-- 
2.49.0.604.gff1f9ca942-goog