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

[David Hildenbrand <david@xxxxxxxxxx>]

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