On 05.04.22 12:52, David Hildenbrand wrote:
> 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)
Looking once more, I think the section check might be sufficient. I
wasn't aware that we'll always have the NODE id stored in page->flags
with vmemmap. (hopefully with more page->flags users like mglru that
won't change)
--
Thanks,
David / dhildenb
