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

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

On Sat, Aug 30, 2025 at 09:28:24PM -0400, Zi Yan wrote:
>On 29 Aug 2025, at 21:25, Wei Yang wrote:
>
>> 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.
>
>find_large_buddy() is used to deal with free pages, so start_pfn is likely
>to be a free page.
>

I am not that familiar with the background, my question here.

I see the call flow is:

alloc_contig_pages_noprof()
    __alloc_contig_pages(pfn, )          <-- start_pfn
        start_isolate_page_range()
            isolate_single_pageblock()
	        set_migratetype_isolate()
		    pageblock_isolate_and_move_free_pages()
		        find_large_buddy()

If my understanding is correct, the start_pfn comes from
__alloc_contig_pages(), which is get from zone's pfn iteration.

I don't see it filter non-free page. So the possibility of free/non-free is
equal to me.

Maybe I missed something?

[...]

>> How about: (not good at commento)
>>
>> 	/*
>> 	 * We start find large buddy from start_pfn order, since a
>
>It is unclear what start_pfn order means.
>
>> 	 * !PageBuddy() means all lower order page is !PageBuddy().
>> 	 */
>
>Here you assume start_pfn is not PageBuddy() already, but it
>can be an order-0 PageBuddy(). That is why my comment explicitly
>excluded the case to begin with.
>
>How about?
>
>If start_pfn is not an order-0 PageBuddy, next PageBuddy containing start_pfn
>has minimal order of __ffs(start_pfn) + 1. Start checking the order with
>__ffs(start_pfn). If start_pfn is order-0, the starting order does not matter.
>

Thanks, will use this one.

>>
>>>
>>> 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.
>
>Straddle means a buddy page has start_pfn in the middle and the caller

Ok, maybe the word "straddle" literally means it. If so, it really confuse.

>in __move_freepages_block_isolate() needs to split the buddy page.

Do you mean if start_pfn is the head of a large buddy page, we don't split it
in __move_freepages_block_isolate()?

Per my understanding, if this pageblock is a part of a large buddy, no matter
it is at the beginning or end, we should split it.

Just want to confirm the behavior with you in case I misunderstand.

-- 
Wei Yang
Help you, Help me