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

[Johannes Weiner <hannes@cmpxchg.org>]

On Thu, Apr 10, 2025 at 01:55:27PM +0000, Brendan Jackman wrote:
> On Mon Apr 7, 2025 at 6:01 PM UTC, Johannes Weiner wrote:
> > find_suitable_fallback() is not as efficient as it could be, and
> > somewhat difficult to follow.
> >
> > 1. should_try_claim_block() is a loop invariant. There is no point in
> >    checking fallback areas if the caller is interested in claimable
> >    blocks but the order and the migratetype don't allow for that.
> >
> > 2. __rmqueue_steal() doesn't care about claimability, so it shouldn't
> >    have to run those tests.
> >
> > Different callers want different things from this helper:
> >
> > 1. __compact_finished() scans orders up until it finds a claimable block
> > 2. __rmqueue_claim() scans orders down as long as blocks are claimable
> > 3. __rmqueue_steal() doesn't care about claimability at all
> >
> > Move should_try_claim_block() out of the loop. Only test it for the
> > two callers who care in the first place. Distinguish "no blocks" from
> > "order + mt are not claimable" in the return value; __rmqueue_claim()
> > can stop once order becomes unclaimable, __compact_finished() can keep
> > advancing until order becomes claimable.
> 
> Nice!
> 
> My initial thought was: now we can drop the boolean arg and just have
> the callers who care about claimability just call
> should_try_claim_block() themselves. Then we can also get rid of the
> magic -2 return value and find_suitable_fallback() becomes a pretty
> obvious function.
> 
> I think it's a win on balance but it does make more verbosity at the
> callsites, and an extra interface between page_alloc.c and compaction.c
> So maybe it's a wash, maybe you already considered it and decided you
> don't prefer it.
> 
> So, LGTM either way, I will attempt to attach the optional additional
> patch for your perusal, hopefully without molesting the mail encoding
> this time...
> 
> Reviewed-by: Brendan Jackman <jackmanb@google.com>

Thanks!

> From 25f77012674db95354fb2496bc89954b8f8b4e6c 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.
> 
> 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..d735c22e71029 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 (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)