[PATCH v3 0/2] mm/page_alloc: Some clarifications for migratetype fallback

[PATCH v3 0/2] mm/page_alloc: Some clarifications for migratetype fallback

[Brendan Jackman]

A couple of patches to try and make the code easier to follow.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
Changes in v3:
- Rebased onto mm-unstable to get on top of Johannes' cleanups:
  https://lore.kernel.org/linux-mm/20250225001023.1494422-1-hannes@cmpxchg.org/
- Fixed missing renames
- Link to v2: https://lore.kernel.org/r/20250224-clarify-steal-v2-0-be24da656764@google.com

Changes in v2:
- Tweaked some naming
- Added clarifications of should_claim_block()
- Link to v1: https://lore.kernel.org/r/20250214-clarify-steal-v1-1-79dc5adf1b79@google.com

---
Brendan Jackman (2):
      mm/page_alloc: Clarify terminology in migratetype fallback code
      mm/page_alloc: Clarify should_claim_block() commentary

 mm/compaction.c |  4 +--
 mm/internal.h   |  2 +-
 mm/page_alloc.c | 95 +++++++++++++++++++++++++++++++--------------------------
 3 files changed, 54 insertions(+), 47 deletions(-)
---
base-commit: 801d47bd96ce22acd43809bc09e004679f707c39
change-id: 20250214-clarify-steal-f244880441c1

Best regards,
-- 
Brendan Jackman <jackmanb@google.com>

[PATCH v3 1/2] mm/page_alloc: Clarify terminology in migratetype fallback code

[Brendan Jackman]

This code is rather confusing because:

 1. "Steal" is sometimes used to refer to the general concept of
    allocating from a from a block of a fallback migratetype
    (steal_suitable_fallback()) but sometimes it refers specifically to
    converting a whole block's migratetype (can_steal_fallback()).

 2. can_steal_fallback() sounds as though it's answering the question "am
    I functionally permitted to allocate from that other type" but in
    fact it is encoding a heuristic preference.

 3. The same piece of data has different names in different places:
    can_steal vs whole_block. This reinforces point 2 because it looks
    like the different names reflect a shift in intent from "am I
    allowed to steal" to "do I want to steal", but no such shift exists.

Fix 1. by avoiding the term "steal" in ambiguous contexts. Start using
the term "claim" to refer to the special case of stealing the entire
block.

Fix 2. by using "should" instead of "can", and also rename its
parameters and add some commentary to make it more explicit what they
mean.

Fix 3. by adopting the new "claim" terminology universally for this
set of variables.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 mm/compaction.c |  4 ++--
 mm/internal.h   |  2 +-
 mm/page_alloc.c | 72 ++++++++++++++++++++++++++++-----------------------------
 3 files changed, 39 insertions(+), 39 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 0992106d4ea751f7f1f8ebf7c75cd433d676cbe0..550ce50218075509ccb5f9485fd84f5d1f3d23a7 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -2333,7 +2333,7 @@ static enum compact_result __compact_finished(struct compact_control *cc)
 	ret = COMPACT_NO_SUITABLE_PAGE;
 	for (order = cc->order; order < NR_PAGE_ORDERS; order++) {
 		struct free_area *area = &cc->zone->free_area[order];
-		bool can_steal;
+		bool claim_block;
 
 		/* Job done if page is free of the right migratetype */
 		if (!free_area_empty(area, migratetype))
@@ -2350,7 +2350,7 @@ static enum compact_result __compact_finished(struct compact_control *cc)
 		 * other migratetype buddy lists.
 		 */
 		if (find_suitable_fallback(area, order, migratetype,
-						true, &can_steal) != -1)
+						true, &claim_block) != -1)
 			/*
 			 * 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 b07550db2bfd1d152fa90f91b3687b0fa1a9f653..aa30282a774ae26349944a75da854ae6a3da2a98 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -863,7 +863,7 @@ static inline void init_cma_pageblock(struct page *page)
 
 
 int find_suitable_fallback(struct free_area *area, unsigned int order,
-			int migratetype, bool only_stealable, bool *can_steal);
+			int migratetype, bool claim_only, bool *claim_block);
 
 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 5d8e274c8b1d500d263a17ef36fe190f60b88196..5e694046ef92965b34d4831e96d92f02681a8b45 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1942,22 +1942,22 @@ static inline bool boost_watermark(struct zone *zone)
 
 /*
  * When we are falling back to another migratetype during allocation, try to
- * steal extra free pages from the same pageblocks to satisfy further
- * allocations, instead of polluting multiple pageblocks.
+ * claim entire blocks to satisfy further allocations, instead of polluting
+ * multiple pageblocks.
  *
- * If we are stealing a relatively large buddy page, it is likely there will
- * be more free pages in the pageblock, so try to steal them all. For
- * reclaimable and unmovable allocations, we steal regardless of page size,
- * as fragmentation caused by those allocations polluting movable pageblocks
- * is worse than movable allocations stealing from unmovable and reclaimable
- * pageblocks.
+ * If we are stealing a relatively large buddy page, it is likely there will be
+ * more free pages in the pageblock, so try to claim the whole block. For
+ * reclaimable and unmovable allocations, we claim the whole block regardless of
+ * page size, as fragmentation caused by those allocations polluting movable
+ * pageblocks is worse than movable allocations stealing from unmovable and
+ * reclaimable pageblocks.
  */
-static bool can_steal_fallback(unsigned int order, int start_mt)
+static bool should_claim_block(unsigned int order, int start_mt)
 {
 	/*
 	 * Leaving this order check is intended, although there is
 	 * relaxed order check in next check. The reason is that
-	 * we can actually steal whole pageblock if this condition met,
+	 * we can actually claim the whole pageblock if this condition met,
 	 * but, below check doesn't guarantee it and that is just heuristic
 	 * so could be changed anytime.
 	 */
@@ -1970,7 +1970,7 @@ static bool can_steal_fallback(unsigned int order, int start_mt)
 	 * reclaimable pages that are closest to the request size.  After a
 	 * while, memory compaction may occur to form large contiguous pages,
 	 * and the next movable allocation may not need to steal.  Unmovable and
-	 * reclaimable allocations need to actually steal pages.
+	 * reclaimable allocations need to actually claim the whole block.
 	 */
 	if (order >= pageblock_order / 2 ||
 		start_mt == MIGRATE_RECLAIMABLE ||
@@ -1983,12 +1983,14 @@ static bool can_steal_fallback(unsigned int order, int start_mt)
 
 /*
  * Check whether there is a suitable fallback freepage with requested order.
- * If only_stealable is true, this function returns fallback_mt only if
- * we can steal other freepages all together. This would help to reduce
+ * Sets *claim_block to instruct the caller whether it should convert a whole
+ * pageblock to the returned migratetype.
+ * If only_claim 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 only_stealable, bool *can_steal)
+			int migratetype, bool only_claim, bool *claim_block)
 {
 	int i;
 	int fallback_mt;
@@ -1996,19 +1998,16 @@ int find_suitable_fallback(struct free_area *area, unsigned int order,
 	if (area->nr_free == 0)
 		return -1;
 
-	*can_steal = false;
+	*claim_block = false;
 	for (i = 0; i < MIGRATE_PCPTYPES - 1 ; i++) {
 		fallback_mt = fallbacks[migratetype][i];
 		if (free_area_empty(area, fallback_mt))
 			continue;
 
-		if (can_steal_fallback(order, migratetype))
-			*can_steal = true;
+		if (should_claim_block(order, migratetype))
+			*claim_block = true;
 
-		if (!only_stealable)
-			return fallback_mt;
-
-		if (*can_steal)
+		if (*claim_block || !only_claim)
 			return fallback_mt;
 	}
 
@@ -2016,14 +2015,14 @@ int find_suitable_fallback(struct free_area *area, unsigned int order,
 }
 
 /*
- * This function implements actual steal behaviour. If order is large enough, we
- * can claim the whole pageblock for the requested migratetype. If not, we check
- * the pageblock for constituent pages; if at least half of the pages are free
- * or compatible, we can still claim the whole block, so pages freed in the
- * future will be put on the correct free list.
+ * This function implements actual block claiming behaviour. If order is large
+ * enough, we can claim the whole pageblock for the requested migratetype. If
+ * not, we check the pageblock for constituent pages; if at least half of the
+ * pages are free or compatible, we can still claim the whole block, so pages
+ * freed in the future will be put on the correct free list.
  */
 static struct page *
-try_to_steal_block(struct zone *zone, struct page *page,
+try_to_claim_block(struct zone *zone, struct page *page,
 		   int current_order, int order, int start_type,
 		   int block_type, unsigned int alloc_flags)
 {
@@ -2091,11 +2090,12 @@ try_to_steal_block(struct zone *zone, struct page *page,
 /*
  * Try finding a free buddy page on the fallback list.
  *
- * This will attempt to steal a whole pageblock for the requested type
+ * 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 stolen, regress to __rmqueue_smallest()
- * logic to at least break up as little contiguity as possible.
+ * 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.
  *
  * 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
@@ -2112,7 +2112,7 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype,
 	int min_order = order;
 	struct page *page;
 	int fallback_mt;
-	bool can_steal;
+	bool claim_block;
 
 	/*
 	 * Do not steal pages from freelists belonging to other pageblocks
@@ -2131,15 +2131,15 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype,
 				--current_order) {
 		area = &(zone->free_area[current_order]);
 		fallback_mt = find_suitable_fallback(area, current_order,
-				start_migratetype, false, &can_steal);
+				start_migratetype, false, &claim_block);
 		if (fallback_mt == -1)
 			continue;
 
-		if (!can_steal)
+		if (!claim_block)
 			break;
 
 		page = get_page_from_free_area(area, fallback_mt);
-		page = try_to_steal_block(zone, page, current_order, order,
+		page = try_to_claim_block(zone, page, current_order, order,
 					  start_migratetype, fallback_mt,
 					  alloc_flags);
 		if (page)
@@ -2149,11 +2149,11 @@ __rmqueue_fallback(struct zone *zone, int order, int start_migratetype,
 	if (alloc_flags & ALLOC_NOFRAGMENT)
 		return NULL;
 
-	/* No luck stealing blocks. Find the smallest fallback page */
+	/* No luck claiming pageblock. Find the smallest fallback page */
 	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, &can_steal);
+				start_migratetype, false, &claim_block);
 		if (fallback_mt == -1)
 			continue;
 

-- 
2.48.1.658.g4767266eb4-goog

Re: [PATCH v3 1/2] mm/page_alloc: Clarify terminology in migratetype fallback code

[Vlastimil Babka]

On 2/25/25 4:29 PM, Brendan Jackman wrote:
> This code is rather confusing because:
> 
>  1. "Steal" is sometimes used to refer to the general concept of
>     allocating from a from a block of a fallback migratetype
>     (steal_suitable_fallback()) but sometimes it refers specifically to
>     converting a whole block's migratetype (can_steal_fallback()).
> 
>  2. can_steal_fallback() sounds as though it's answering the question "am
>     I functionally permitted to allocate from that other type" but in
>     fact it is encoding a heuristic preference.
> 
>  3. The same piece of data has different names in different places:
>     can_steal vs whole_block. This reinforces point 2 because it looks
>     like the different names reflect a shift in intent from "am I
>     allowed to steal" to "do I want to steal", but no such shift exists.
> 
> Fix 1. by avoiding the term "steal" in ambiguous contexts. Start using
> the term "claim" to refer to the special case of stealing the entire
> block.
> 
> Fix 2. by using "should" instead of "can", and also rename its
> parameters and add some commentary to make it more explicit what they
> mean.
> 
> Fix 3. by adopting the new "claim" terminology universally for this
> set of variables.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

Reviewed-by: Vlastimil Babka <vbabka@suse.cz>

Some nits:

> ---
>  mm/compaction.c |  4 ++--
>  mm/internal.h   |  2 +-
>  mm/page_alloc.c | 72 ++++++++++++++++++++++++++++-----------------------------
>  3 files changed, 39 insertions(+), 39 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 0992106d4ea751f7f1f8ebf7c75cd433d676cbe0..550ce50218075509ccb5f9485fd84f5d1f3d23a7 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -2333,7 +2333,7 @@ static enum compact_result __compact_finished(struct compact_control *cc)
>  	ret = COMPACT_NO_SUITABLE_PAGE;
>  	for (order = cc->order; order < NR_PAGE_ORDERS; order++) {
>  		struct free_area *area = &cc->zone->free_area[order];
> -		bool can_steal;
> +		bool claim_block;
>  
>  		/* Job done if page is free of the right migratetype */
>  		if (!free_area_empty(area, migratetype))
> @@ -2350,7 +2350,7 @@ static enum compact_result __compact_finished(struct compact_control *cc)
>  		 * other migratetype buddy lists.
>  		 */
>  		if (find_suitable_fallback(area, order, migratetype,
> -						true, &can_steal) != -1)
> +						true, &claim_block) != -1)
>  			/*
>  			 * 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 b07550db2bfd1d152fa90f91b3687b0fa1a9f653..aa30282a774ae26349944a75da854ae6a3da2a98 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -863,7 +863,7 @@ static inline void init_cma_pageblock(struct page *page)
>  
>  
>  int find_suitable_fallback(struct free_area *area, unsigned int order,
> -			int migratetype, bool only_stealable, bool *can_steal);
> +			int migratetype, bool claim_only, bool *claim_block);
>  
>  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 5d8e274c8b1d500d263a17ef36fe190f60b88196..5e694046ef92965b34d4831e96d92f02681a8b45 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1942,22 +1942,22 @@ static inline bool boost_watermark(struct zone *zone)
>  
>  /*
>   * When we are falling back to another migratetype during allocation, try to
> - * steal extra free pages from the same pageblocks to satisfy further
> - * allocations, instead of polluting multiple pageblocks.
> + * claim entire blocks to satisfy further allocations, instead of polluting
> + * multiple pageblocks.
>   *
> - * If we are stealing a relatively large buddy page, it is likely there will
> - * be more free pages in the pageblock, so try to steal them all. For
> - * reclaimable and unmovable allocations, we steal regardless of page size,
> - * as fragmentation caused by those allocations polluting movable pageblocks
> - * is worse than movable allocations stealing from unmovable and reclaimable
> - * pageblocks.
> + * If we are stealing a relatively large buddy page, it is likely there will be
> + * more free pages in the pageblock, so try to claim the whole block. For
> + * reclaimable and unmovable allocations, we claim the whole block regardless of

It's also "try to claim" here as it may still fail due to not enough
free/compatible pages even for those migratetypes. Maybe the question
(out of scope of the patch) if they should get a lower threshold than
half. Before migratetype hygiene, the "we steal regardless" meant that
we really would steal all free pages even if not claiming the pageblock.

> + * page size, as fragmentation caused by those allocations polluting movable
> + * pageblocks is worse than movable allocations stealing from unmovable and
> + * reclaimable pageblocks.
>   */
> -static bool can_steal_fallback(unsigned int order, int start_mt)
> +static bool should_claim_block(unsigned int order, int start_mt)

So technically it's should_try_claim_block() if we want to be precise
(but longer).

>  {
>  	/*
>  	 * Leaving this order check is intended, although there is
>  	 * relaxed order check in next check. The reason is that
> -	 * we can actually steal whole pageblock if this condition met,
> +	 * we can actually claim the whole pageblock if this condition met,

try claiming

>  	 * but, below check doesn't guarantee it and that is just heuristic
>  	 * so could be changed anytime.
>  	 */
> @@ -1970,7 +1970,7 @@ static bool can_steal_fallback(unsigned int order, int start_mt)
>  	 * reclaimable pages that are closest to the request size.  After a
>  	 * while, memory compaction may occur to form large contiguous pages,
>  	 * and the next movable allocation may not need to steal.  Unmovable and
> -	 * reclaimable allocations need to actually steal pages.
> +	 * reclaimable allocations need to actually claim the whole block.

also

>  	 */
>  	if (order >= pageblock_order / 2 ||
>  		start_mt == MIGRATE_RECLAIMABLE ||

Re: [PATCH v3 1/2] mm/page_alloc: Clarify terminology in migratetype fallback code

[Brendan Jackman]

On Wed, Feb 26, 2025 at 04:08:09PM +0100, Vlastimil Babka wrote:
> On 2/25/25 4:29 PM, Brendan Jackman wrote:
> >  /*
> >   * When we are falling back to another migratetype during allocation, try to
> > - * steal extra free pages from the same pageblocks to satisfy further
> > - * allocations, instead of polluting multiple pageblocks.
> > + * claim entire blocks to satisfy further allocations, instead of polluting
> > + * multiple pageblocks.
> >   *
> > - * If we are stealing a relatively large buddy page, it is likely there will
> > - * be more free pages in the pageblock, so try to steal them all. For
> > - * reclaimable and unmovable allocations, we steal regardless of page size,
> > - * as fragmentation caused by those allocations polluting movable pageblocks
> > - * is worse than movable allocations stealing from unmovable and reclaimable
> > - * pageblocks.
> > + * If we are stealing a relatively large buddy page, it is likely there will be
> > + * more free pages in the pageblock, so try to claim the whole block. For
> > + * reclaimable and unmovable allocations, we claim the whole block regardless of
> 
> It's also "try to claim" here as it may still fail due to not enough
> free/compatible pages even for those migratetypes. 

Yeah thanks, I seems worthwhile to be precise about this.

> Maybe the question
> (out of scope of the patch) if they should get a lower threshold than
> half. Before migratetype hygiene, the "we steal regardless" meant that
> we really would steal all free pages even if not claiming the pageblock.

I wonder if there's some benefit to deciding when to claim blocks also
based on the count of blocks per type or something? Like, if we're
under memory pressure so all the freelists are empty, but there are
already a bunch of unmovable blocks (that currently have movable pages
in them), there's not much point in claiming a block, instead we'd
prefer to reclaim/migrate those movable pages?

> > + * page size, as fragmentation caused by those allocations polluting movable
> > + * pageblocks is worse than movable allocations stealing from unmovable and
> > + * reclaimable pageblocks.
> >   */
> > -static bool can_steal_fallback(unsigned int order, int start_mt)
> > +static bool should_claim_block(unsigned int order, int start_mt)
> 
> So technically it's should_try_claim_block() if we want to be precise
> (but longer).

Yep, that seems worth it (especially given the function is effectively
just an internal block of find_suitable_fallback()).

> >  {
> >  	/*
> >  	 * Leaving this order check is intended, although there is
> >  	 * relaxed order check in next check. The reason is that
> > -	 * we can actually steal whole pageblock if this condition met,
> > +	 * we can actually claim the whole pageblock if this condition met,
> 
> try claiming

But I think in this case we really do claim unconditionally.

Re: [PATCH v3 1/2] mm/page_alloc: Clarify terminology in migratetype fallback code

[Vlastimil Babka]

On 2/27/25 11:13, Brendan Jackman wrote:
> On Wed, Feb 26, 2025 at 04:08:09PM +0100, Vlastimil Babka wrote:
>> On 2/25/25 4:29 PM, Brendan Jackman wrote:
>> >  /*
>> >   * When we are falling back to another migratetype during allocation, try to
>> > - * steal extra free pages from the same pageblocks to satisfy further
>> > - * allocations, instead of polluting multiple pageblocks.
>> > + * claim entire blocks to satisfy further allocations, instead of polluting
>> > + * multiple pageblocks.
>> >   *
>> > - * If we are stealing a relatively large buddy page, it is likely there will
>> > - * be more free pages in the pageblock, so try to steal them all. For
>> > - * reclaimable and unmovable allocations, we steal regardless of page size,
>> > - * as fragmentation caused by those allocations polluting movable pageblocks
>> > - * is worse than movable allocations stealing from unmovable and reclaimable
>> > - * pageblocks.
>> > + * If we are stealing a relatively large buddy page, it is likely there will be
>> > + * more free pages in the pageblock, so try to claim the whole block. For
>> > + * reclaimable and unmovable allocations, we claim the whole block regardless of
>> 
>> It's also "try to claim" here as it may still fail due to not enough
>> free/compatible pages even for those migratetypes. 
> 
> Yeah thanks, I seems worthwhile to be precise about this.
> 
>> Maybe the question
>> (out of scope of the patch) if they should get a lower threshold than
>> half. Before migratetype hygiene, the "we steal regardless" meant that
>> we really would steal all free pages even if not claiming the pageblock.
> 
> I wonder if there's some benefit to deciding when to claim blocks also
> based on the count of blocks per type or something? Like, if we're
> under memory pressure so all the freelists are empty, but there are
> already a bunch of unmovable blocks (that currently have movable pages
> in them), there's not much point in claiming a block, instead we'd
> prefer to reclaim/migrate those movable pages?

Hm we don't currently have counters for that, do we? But that would be the
feasible part. "that currently have movable pages in them" would be
expensive to determine, and an attempt to reclaim/migrate would be expensive
too. At best we could try kicking some asynchronous attempt I think.

>> > + * page size, as fragmentation caused by those allocations polluting movable
>> > + * pageblocks is worse than movable allocations stealing from unmovable and
>> > + * reclaimable pageblocks.
>> >   */
>> > -static bool can_steal_fallback(unsigned int order, int start_mt)
>> > +static bool should_claim_block(unsigned int order, int start_mt)
>> 
>> So technically it's should_try_claim_block() if we want to be precise
>> (but longer).
> 
> Yep, that seems worth it (especially given the function is effectively
> just an internal block of find_suitable_fallback()).
> 
>> >  {
>> >  	/*
>> >  	 * Leaving this order check is intended, although there is
>> >  	 * relaxed order check in next check. The reason is that
>> > -	 * we can actually steal whole pageblock if this condition met,
>> > +	 * we can actually claim the whole pageblock if this condition met,
>> 
>> try claiming
> 
> But I think in this case we really do claim unconditionally.

Ah right, thanks.

[PATCH v3 2/2] mm/page_alloc: Clarify should_claim_block() commentary

[Brendan Jackman]

There's lots of text here but it's a little hard to follow, this is an
attempt to break it up and align its structure more closely with the
code.

Reword the top-level function comment to just explain what question the
function answers from the point of view of the caller.

Break up the internal logic into different sections that can have their
own commentary describing why that part of the rationale is present.

Note the page_groupy_by_mobility_disabled logic is not explained in the
commentary, that is outside the scope of this patch...

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 mm/page_alloc.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5e694046ef92965b34d4831e96d92f02681a8b45..475ec1284033acec69da4a39dd4e7d7fbaee6d0f 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1941,16 +1941,9 @@ static inline bool boost_watermark(struct zone *zone)
 }
 
 /*
- * When we are falling back to another migratetype during allocation, try to
- * claim entire blocks to satisfy further allocations, instead of polluting
- * multiple pageblocks.
- *
- * If we are stealing a relatively large buddy page, it is likely there will be
- * more free pages in the pageblock, so try to claim the whole block. For
- * reclaimable and unmovable allocations, we claim the whole block regardless of
- * page size, as fragmentation caused by those allocations polluting movable
- * pageblocks is worse than movable allocations stealing from unmovable and
- * reclaimable pageblocks.
+ * When we are falling back to another migratetype during allocation, should we
+ * try to claim an entire block to satisfy further allocations, instead of
+ * polluting multiple pageblocks?
  */
 static bool should_claim_block(unsigned int order, int start_mt)
 {
@@ -1964,6 +1957,26 @@ static bool should_claim_block(unsigned int order, int start_mt)
 	if (order >= pageblock_order)
 		return true;
 
+	/*
+	 * Above a certain threshold, always try to claim, as it's likely there
+	 * will be more free pages in the pageblock.
+	 */
+	if (order >= pageblock_order / 2)
+		return true;
+
+	/*
+	 * Unmovable/reclaimable allocations would cause permanent
+	 * fragmentations if they fell back to allocating from a movable block
+	 * (polluting it), so we try to claim the whole block regardless of the
+	 * allocation size. Later movable allocations can always steal from this
+	 * block, which is less problematic.
+	 */
+	if (start_mt == MIGRATE_RECLAIMABLE || start_mt == MIGRATE_UNMOVABLE)
+		return true;
+
+	if (page_group_by_mobility_disabled)
+		return true;
+
 	/*
 	 * Movable pages won't cause permanent fragmentation, so when you alloc
 	 * small pages, you just need to temporarily steal unmovable or
@@ -1972,12 +1985,6 @@ static bool should_claim_block(unsigned int order, int start_mt)
 	 * and the next movable allocation may not need to steal.  Unmovable and
 	 * reclaimable allocations need to actually claim the whole block.
 	 */
-	if (order >= pageblock_order / 2 ||
-		start_mt == MIGRATE_RECLAIMABLE ||
-		start_mt == MIGRATE_UNMOVABLE ||
-		page_group_by_mobility_disabled)
-		return true;
-
 	return false;
 }
 

-- 
2.48.1.658.g4767266eb4-goog

Re: [PATCH v3 2/2] mm/page_alloc: Clarify should_claim_block() commentary

[Vlastimil Babka]

On 2/25/25 4:29 PM, Brendan Jackman wrote:
> There's lots of text here but it's a little hard to follow, this is an
> attempt to break it up and align its structure more closely with the
> code.
> 
> Reword the top-level function comment to just explain what question the
> function answers from the point of view of the caller.
> 
> Break up the internal logic into different sections that can have their
> own commentary describing why that part of the rationale is present.
> 
> Note the page_groupy_by_mobility_disabled logic is not explained in the

grouping

> commentary, that is outside the scope of this patch...
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

Johannes suggested moving the checks to the caller and removing this
function but with this kind of detailed commentary I guess it's better
to keep it as a separate function.

> ---
>  mm/page_alloc.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 5e694046ef92965b34d4831e96d92f02681a8b45..475ec1284033acec69da4a39dd4e7d7fbaee6d0f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1941,16 +1941,9 @@ static inline bool boost_watermark(struct zone *zone)
>  }
>  
>  /*
> - * When we are falling back to another migratetype during allocation, try to
> - * claim entire blocks to satisfy further allocations, instead of polluting
> - * multiple pageblocks.
> - *
> - * If we are stealing a relatively large buddy page, it is likely there will be
> - * more free pages in the pageblock, so try to claim the whole block. For
> - * reclaimable and unmovable allocations, we claim the whole block regardless of
> - * page size, as fragmentation caused by those allocations polluting movable
> - * pageblocks is worse than movable allocations stealing from unmovable and
> - * reclaimable pageblocks.
> + * When we are falling back to another migratetype during allocation, should we
> + * try to claim an entire block to satisfy further allocations, instead of
> + * polluting multiple pageblocks?
>   */
>  static bool should_claim_block(unsigned int order, int start_mt)
>  {
> @@ -1964,6 +1957,26 @@ static bool should_claim_block(unsigned int order, int start_mt)
>  	if (order >= pageblock_order)
>  		return true;
>  
> +	/*
> +	 * Above a certain threshold, always try to claim, as it's likely there
> +	 * will be more free pages in the pageblock.
> +	 */
> +	if (order >= pageblock_order / 2)
> +		return true;
> +
> +	/*
> +	 * Unmovable/reclaimable allocations would cause permanent
> +	 * fragmentations if they fell back to allocating from a movable block
> +	 * (polluting it), so we try to claim the whole block regardless of the
> +	 * allocation size. Later movable allocations can always steal from this
> +	 * block, which is less problematic.
> +	 */
> +	if (start_mt == MIGRATE_RECLAIMABLE || start_mt == MIGRATE_UNMOVABLE)
> +		return true;
> +
> +	if (page_group_by_mobility_disabled)
> +		return true;
> +
>  	/*
>  	 * Movable pages won't cause permanent fragmentation, so when you alloc
>  	 * small pages, you just need to temporarily steal unmovable or
> @@ -1972,12 +1985,6 @@ static bool should_claim_block(unsigned int order, int start_mt)
>  	 * and the next movable allocation may not need to steal.  Unmovable and
>  	 * reclaimable allocations need to actually claim the whole block.
>  	 */

This block could be also massaged? I'd unify the style so it's "we"
everywhere and not suddenly "you". The last sentence is redundant with
the comment you added above reclaimable/unmovable. The text should be
just about movable allocations in the case of small enough order.

> -	if (order >= pageblock_order / 2 ||
> -		start_mt == MIGRATE_RECLAIMABLE ||
> -		start_mt == MIGRATE_UNMOVABLE ||
> -		page_group_by_mobility_disabled)
> -		return true;
> -
>  	return false;
>  }
>  
>