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

[David Hildenbrand (Arm)]

On 3/6/26 15:29, Gladyshev Ilya wrote:
>>>
>>> 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.

I agree.

refcount == 0 will only be allowed temporarily while transitioning to
the frozen state.

In fact, you should then rename PAGEREF_LOCKED_BIT to PAGEREF_FROZEN_BIT
and page_ref_add_unless_zero() to page_ref_add_unless_frozen.

That then matches the terminology we use for inactive refcounts
(page_ref_freeze(), alloc_frozen_pages()).

And it will be a lot easier to comprehend compared to using a different
"locking" terminology when traditionally the "page lock" is something
completely different.

-- 
Cheers,

David