Re: [RFC PATCH v5 01/15] mm: Consolidate freeing of typed folios on final folio_put()

[David Hildenbrand <david@redhat.com>]

On 20.01.25 11:43, Vlastimil Babka wrote:
> On 1/20/25 11:39, David Hildenbrand wrote:
>> On 17.01.25 17:29, Fuad Tabba wrote:
>>> diff --git a/mm/swap.c b/mm/swap.c
>>> index 10decd9dffa1..6f01b56bce13 100644
>>> --- a/mm/swap.c
>>> +++ b/mm/swap.c
>>> @@ -94,6 +94,20 @@ static void page_cache_release(struct folio *folio)
>>>    		unlock_page_lruvec_irqrestore(lruvec, flags);
>>>    }
>>>    
>>> +static void free_typed_folio(struct folio *folio)
>>> +{
>>> +	switch (folio_get_type(folio)) {
>>> +	case PGTY_hugetlb:
>>> +		free_huge_folio(folio);
>>> +		return;
>>> +	case PGTY_offline:
>>> +		/* Nothing to do, it's offline. */
>>> +		return;
>>
>> Please drop the PGTY_offline part for now, it was rather to highlight
>> what could be done.
>>
>> But the real goal will be to not make offline pages
>> use the refcount at all (frozen).
>>
>> If we really want the temporary PGTY_offline change, it should be
>> introduced separately.
>>
>> Apart from that LGTM!
> 
> I gues you mean the WARN_ON_ONCE(1) should be dropped from the default:
> handler as well, right? IIUC offline pages are not not yet frozen so there
> will be warnings otherwise.

If we get offline pages here, it is unexpected and wrong. All users 
clear PG_offline before handing them back to the buddy.

There is one nasty race in virtio-mem code for handling memory offlining 
with PG_offline pages, which we haven't seen so far in practice. See 
virtio_mem_fake_offline_going_offline()->page_ref_dec_and_test() for the 
nasty details. It would only trigger with some weird speculative references.

The proper fix will be do leave the refcount frozen for them, so 
speculative refcount users will just fail.

If we want to tackle that before we can do that, we should do it in a 
separate patch (and not buried in this series).

And I haven't check if the other types are
> frozen (I know slab is, very recently :)

I think if we would have that, we would end up triggering the 
free_pages_prepare()->free_page_is_bad(), because page->_mapcount == 
folio->mapcount would not be -1 for typed folios.

Right now we don't expect folios of specific types to ever get freed (in 
the future, these won't be folios anymore at all -- only guest_memfd and 
hugetlb would be folios, that need special care to be handed back to 
their pool).

-- 
Cheers,

David / dhildenb