On 05.04.22 12:42, Vlastimil Babka wrote:
> On 4/5/22 12:29, David Hildenbrand wrote:
>>> +
>>> +/*
>>> + * This function checks whether a page is free && is the buddy
>>> + * we can coalesce a page and its buddy if
>>> + * (a) the buddy is not in a hole (check before calling!) &&
>>> + * (b) the buddy is in the buddy system &&
>>> + * (c) a page and its buddy have the same order &&
>>> + * (d) a page and its buddy are in the same zone.
>>> + *
>>> + * For recording whether a page is in the buddy system, we set PageBuddy.
>>> + * Setting, clearing, and testing PageBuddy is serialized by zone->lock.
>>> + *
>>> + * For recording page's order, we use page_private(page).
>>> + */
>>> +static inline bool page_is_buddy(struct page *page, struct page *buddy,
>>> + unsigned int order)
>>
>> Nit: Strange indentation of the last parameter. But it's already in the
>> code you're moving, so ...
>>
>>> +{
>>> + if (!page_is_guard(buddy) && !PageBuddy(buddy))
>>> + return false;
>>> +
>>> + if (buddy_order(buddy) != order)
>>> + return false;
>>> +
>>> + /*
>>> + * zone check is done late to avoid uselessly calculating
>>> + * zone/node ids for pages that could never merge.
>>
>> I know, this is from existing code. But there really isn't a node check
>> when relying on the zone id. I hope we are not missing a node check :)
>
> AFAIK "zone_id" is in fact the combination of node+zone (or section+zone),
> so it works as long as in config where it's a section
> (NODE_NOT_IN_PAGE_FLAGS is defined) then there can't be multiple nodes in a
> single section.
Oh, okay. I was reading "We are not really identifying the zone since we
could be using the section number id if we do not have node id available
in page flags.. We only guarantee that it will return the same value for
two combinable pages in a zone."
an naturally wondered if this could be problematic. Thanks for clarifying.
(once we would support MAX_ORDER-1 exceed a single memory section this
could be problematic)
>
>>> + */
>>> + if (page_zone_id(page) != page_zone_id(buddy))
>>> + return false;
>>> +
>>> + VM_BUG_ON_PAGE(page_count(buddy) != 0, buddy);
>>> +
>>> + return true;
>>> +}
>>> +
>>> +/*
>>> * Locate the struct page for both the matching buddy in our
>>> * pair (buddy1) and the combined O(n+1) page they form (page).
>>> *
>>> @@ -234,6 +295,35 @@ __find_buddy_pfn(unsigned long page_pfn,
>>> return page_pfn ^ (1 << order);
>>> }
>>>
>>> +/*
>>> + * Find the buddy of @page and validate it.
>>> + * @page: The input page
>>> + * @pfn: The pfn of the page, it saves a call to page_to_pfn() when the
>>> + * function is used in the performance-critical __free_one_page().
>>> + * @order: The order of the page
>>> + * @buddy_pfn: The output pointer to the buddy pfn, it also saves a call to
>>> + * page_to_pfn().
>>> + *
>>> + * The found buddy can be a non PageBuddy, out of @page's zone, or its order is
>>> + * not the same as @page. The validation is necessary before use it.
>>> + *
>>> + * Return: the found buddy page or NULL if not found.
>>> + */
>>> +static inline struct page *find_buddy_page_pfn(struct page *page,
>>> + unsigned long pfn, unsigned int order, unsigned long *buddy_pfn)
>>> +{
>>> + struct page *buddy;
>>> + unsigned long __buddy_pfn = __find_buddy_pfn(pfn, order);
>>
>> Nit: reverse christmas tree
>
> Was ever reverse xmas tree part of mm/ style or whole Linux styel outside of
> net?
I assume it's used fairly inconsistently.
--
Thanks,
David / dhildenb
