Re: [PATCH RFC 4/4] mm/page_alloc: place pages to tail in __free_pages_core()

David Hildenbrand
2020-09-28T08:36:20Z

On 28.09.20 09:58, Oscar Salvador wrote:
> On Wed, Sep 16, 2020 at 08:34:11PM +0200, David Hildenbrand wrote:
>> @@ -1523,7 +1524,13 @@ void __free_pages_core(struct page *page, unsigned int order)
>>
>> atomic_long_add(nr_pages, &page_zone(page)->managed_pages);
>> set_page_refcounted(page);
>> - __free_pages(page, order);
>> +
>> + /*
>> + * Bypass PCP and place fresh pages right to the tail, primarily
>> + * relevant for memory onlining.
>> + */
>> + page_ref_dec(page);
>> + __free_pages_ok(page, order, FOP_TO_TAIL);
>
> Sorry, I must be missing something obvious here, but I am a bit confused here.
> I get the part of placing them at the tail so rmqueue_bulk() won't
> find them, but I do not get why we decrement page's refcount.
> IIUC, its refcount will be 0, but why do we want to do that?
>
> Another thing a bit unrelated... we mess three times with page's refcount
> (two before this patch).
> Why do we have this dance in place?

Hi Oscar!

Old code:

set_page_refcounted(): sets the refcount to 1.
__free_pages()
-> put_page_testzero(): sets it to 0
-> free_the_page()->__free_pages_ok()

New code:

set_page_refcounted(): sets the refcount to 1.
page_ref_dec(page): sets it to 0
__free_pages_ok():

We could skip the set_page_refcounted() + page_ref_dec(page) and lose a
couple of sanity checks but we could simply use a
VM_BUG_ON_PAGE(page_ref_count(page), page), which is what we really care
about when onlining memory.

--
Thanks,

David / dhildenb