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

[Wei Yang <richard.weiyang@gmail.com>]

On Thu, Aug 28, 2025 at 11:02:33PM -0400, Zi Yan wrote:
>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;
>

Hi, Zi Yan

Thanks for the review.

>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.

One thing I want to point out is in __move_freepages_block_isolate(),
find_large_buddy() is always given a pageblock aligned start_pfn. This means
if start_pfn is not a free page, it would always try 10 times until give up.

>
>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.

Sure, I would add a comment above the assignment.

But in my mind, we are not farst forward order, but start check from order of
__ffs(start_pfn).


How about: (not good at commento)

	/*
	 * We start find large buddy from start_pfn order, since a
	 * !PageBuddy() means all lower order page is !PageBuddy().
	 */

>
>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:

The comment above this function says "a buddy that straddles start_pfn", this
looks good to me. An order-0 page that start from start_pfn also means
straddle start_pfn.

This may differ from original logic, but seems not wrong.

>
>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.
>

I prefer not adding this.

>
>
>--
>Best Regards,
>Yan, Zi

-- 
Wei Yang
Help you, Help me