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