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

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

On Fri, Aug 29, 2025 at 08:20:04PM -0700, Vishal Moola (Oracle) wrote:
>On Sat, Aug 30, 2025 at 01:25:05AM +0000, 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 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().
>> 	 */
>
>Personally, the way Zi worded it makes more sense to me. Maybe replacing
>the second sentence with your comment is fine, but I think Zi's first
>sentence provides important context in a clear way.

Hi, Vishal 

Thanks for your comment. I am ok with it. Let's see Zi's opnion. And I will
send a new version. :-)

-- 
Wei Yang
Help you, Help me