Re: [PATCH] mm/page_alloc: find_large_buddy() from start_pfn aligned order

[Zi Yan <ziy@nvidia.com>]

On 28 Aug 2025, at 5:16, Wei Yang wrote:

> We iterate pfn from order 0 to MAX_PAGE_ORDER aligned to find large
> buddy. While if the order is less than start_pfn aligned order, we would
> get the same pfn and do the same check again.
>
> Iterate from start_pfn aligned order to reduce duplicated work.
>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Zi Yan <ziy@nvidia.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: David Hildenbrand <david@redhat.com>
>
> ---
> I build this and run, but not sure how fully test this.
> ---
>  mm/page_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 27ea4c7acd15..7f2dfd30106f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2033,7 +2033,7 @@ static int move_freepages_block(struct zone *zone, struct page *page,
>  /* Look for a buddy that straddles start_pfn */
>  static unsigned long find_large_buddy(unsigned long start_pfn)
>  {
> -	int order = 0;
> +	int order = start_pfn ? __ffs(start_pfn) : MAX_PAGE_ORDER;
>  	struct page *page;
>  	unsigned long pfn = start_pfn;

I think it is right, but the code is very subtle and hard to understand
after the change. It is better to add comment to explain it.

Paste the code below for more context:

        while (!PageBuddy(page = pfn_to_page(pfn))) {
                /* Nothing found */
                if (++order > MAX_PAGE_ORDER)
                        return start_pfn;
                pfn &= ~0UL << order;
        }



The code tries to find a PageBuddy starting from start_pfn starting from
order=0. When entering the while loop, it means PageBuddy cannot be order-0
and ++order increases the order by 1. Your change fast forwards the process
based on start_pfn. If start_pfn is not an order-0 page, based on first
set bit in start_pfn and how buddy page is chosen, the next possible PageBuddy
order can only be __ffs(start_pfn) + 1. Your code starts order at __ffs(start_pfn)
and it works because "if (++order > MAX_PAGE_ORDER)" increases order
to __ffs(start_pfn) + 1.

Can you add a comment on your "int order = ..."? Something like:

If start_pfn is not an order-0 PageBuddy, next PageBuddy containing start_pfn
has minimal order of __ffs(start_pfn) + 1. Fastforward order to __ffs(start_pfn)
to remove unnecessary work in the while below.

Feel free to reword the above.

With the added comment, feel free to add Reviewed-by: Zi Yan <ziy@nvidia.com>


BTW, I also notice that when start_pfn is an order-0 PageBuddy, the
"if (pfn + (1 << buddy_order(page)) > start_pfn)" check below would be true
even if there is no buddy straddles start_pfn, although "return pfn"
gives the same results as "return start_pfn" (no straddle). The original
code before the addition of find_large_buddy() (commit fd919a85cd55 ("mm:
page_isolation: prepare for hygienic freelists")) checks start_pfn == pfn
before the straddle check, so the correct code should check start_pfn == pfn
and return early. But since current code is functionally equivalent.
Maybe adding a comment about it would be sufficient. Something like:

When the found buddy order is 0, the check would give false positive,
but the returned result is still correct, since pfn is the same as start_pfn.



--
Best Regards,
Yan, Zi