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

["David Hildenbrand (Arm)" <david@kernel.org>]

>>>
>>>       rcu_read_lock();
>>>       /* avoid writing to the vmemmap area being remapped */
>>> -    if (page_count_writable(page))
>>> -        ret = atomic_add_unless(&page->_refcount, nr, 0);
>>> +    if (page_count_writable(page)) {
>>> +        val = atomic_add_return(nr, &page->_refcount);
>>> +        ret = !(val & PAGEREF_LOCKED_BIT);
>>> +
>>> +        /* Undo atomic_add() if counter is locked and scary big */
>>> +        while (unlikely((unsigned int)val >= _PAGEREF_LOCKED_LIMIT))
>>> +            val = atomic_cmpxchg_relaxed(&page->_refcount, val,
> PAGEREF_LOCKED_BIT);
>>
>> I assume we can't do an atomic_dec(), because we might have concurrent
>> unfreezing (or similar things) happening that overwrote whatever was in
>> there.
> 
> Not only that, but also you probably don't want to handle "atomic_dec()
> returned 0" situations.

Right, and then we'd have to free the page here instead, which is rather
awkward.


>>
>> I was wondering is whether page_ref_freeze() could actually leave the
>> references set, and only set the PAGEREF_LOCKED_BIT bit, whereby
>> page_ref_unfreeze() would only clear the PAGEREF_LOCKED_BIT bit.
>>
>> Similarly, the set_page_refcounted() could add a reference and clear the
>> PAGEREF_LOCKED_BIT. That'd be more expensive on the allocation path ...
>> and not sure if that would really help to turn this
>> atomic_cmpxchg_relaxed() into an simpler atomic_dec() my brain could
>> more easily understand 🙂
> 
> I believe resetting whole counter via single CAS (CPU-wise) is cheaper
> than atomic_dec() for each individual attempt.
> 

Right, that's what I meant with "more expensive".

[...]

>>
>> 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.

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


A refcount of 0 must really only be temporarily allowed in the system
when transitioning from 0 to PAGEREF_LOCKED_BIT. Otherwise this just
screams for trouble.

> 
> 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.

-- 
Cheers,

David