Re: [PATCH 1/1] mm: implement page refcount locking via dedicated bit

[Gladyshev Ilya <gladyshev.ilya1@h-partners.com>]

>>> For example, all pages we add to the page allocator through
>>> __free_pages_core().
>>
>> You are right that refcount = 0 is tricky. However, for a bad outcome
>> you will need:
>> 1. Some external reference to this page, through which you try to
>> increment the refcount;
>> 2. set_page_count(0) somewhere between freeing and "it is safe to alloc"
>> state.
> 
> That is way, way, way too dodgy.
> 
> What you should likely do is
> 
> (a) Make set_page_count(0) set it to PAGEREF_LOCKED_BIT
> (b) Make any places that might skip set_page_count(0) to use it
> (c) Document this all extremely thoroughly.
Okay, I agree. My reasoning was more about "it's not _that_ bad if we 
miss something outside the allocator" than "everything is fine as it 
is", but maybe it is really that bad...

> Alternatively, disallow set_page_count(0) ccompletely and add something
> like "initialize_page_as_frozen()" or sth. like that.

(Below are my thoughts, not active proposal)

Actually, we can keep "zero is locked" property if we change scheme a 
little:

If we invert locked bit so that 0 means locked, then we don't care about 
memset() / GFP_ZERO / set_page(0). But then there is extra bit on each 
active refcount, and (un)masking will be required on each refcount 
read/set/etc operation.

We can place locked bit in the highest bit like now, or move it to the 
lowest. This doesn't make any difference except for how masking is 
performed (and or shift). I can't really tell which approach is faster 
(and more optimizable after inline).

This approach doesn't look appealing to me, mostly because of redundant 
masking. So for now I will try to add proper initialization / 
documentation in all risky places.
>> So adding new pages with zeroed refcount to allocator is safe, because
>> there are no external references. Zeroing tail page's refcount is safe,
>> unless someone actually tries to increment its refcount (and this is bug).
>>
>> Generally, the only unsafe set_page_count() (or any other zeroing) will
>> be in allocator itself between freeing and allocating. Or maybe I missed
>> something, and this approach is indeed incorrect
>>
>> Probably we can think of some debug checks to prevent bugs in "safe"
>> scenarious
> 
> Just take a look at do_migrate_range(), where we do a
> 
> page = pfn_to_page(pfn);
> folio = page_folio(page);
> 
> if (!folio_try_get(folio))
> 	continue;
> 
> If that is a page that was added to the buddy (set_page_count(0)) but
> either (a) not allocated yet or (b) allocated as frozen that's a problem.
> 
> We really rely on
> 
> (1) Frozen pages
> (2) Buddy pages
> (3) Tail pages (compound or non-compoud)
 >
> To have a refcount of 0 that *reliably* makes folio_try_get() etc. fail.
> 
> Anything else is just playing with fire.
>