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

[Vlastimil Babka <vbabka@suse.cz>]

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;
>  }
>  
>