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

[David Hildenbrand (Arm)]

On 9/23/26 19:34, Ilya Gladyshev wrote:
> September 15, 2026 at 5:42 AM, "Zi Yan" <ziy@xxxxxxxxxx mailto:ziy@xxxxxxxxxx?to=%22Zi%20Yan%22%20%3Cziy%40nvidia.com%3E > wrote:
> 
> 
>>
>> On Sat Sep 12, 2026 at 3:50 PM EDT, Ilya Gladyshev wrote:
>>
>>>
>>> The current page refcount implementation uses a single counter value
>>>  (zero) as dead. So, to prevent incrementing a dead refcount in
>>>  folio_try_get(), it fundamentally requires a CAS loop.
>>>
>>>  This CAS loop can act as a serialization point and can become a
>>>  significant bottleneck during high-frequency file read operations
>>>  [1][2].
>>>
>>>  This patch reallocates the refcount value range:
>>>
>>>  (1) refcount < 0 means dead refcount (uninit / frozen)
>>>  (2) refcount = 0 allowed only as a temporary state (see below)
>>>  (3) refcount > 0 is a regular reference count
>>>
>>>  In other words, refcount is now split into "dead bit" and a 31-bit
>>>  counter.
>>>
>>>  Refcount decrement now works as follows:
>>>  1. Counter decrement
>>>  2. If it is now zero, try to put it deep inside the dead zone
>>>  (CAS to INT_MIN). Or you can view it as "set up frozen bit and reset
>>>  counter".
>>>  3. This CAS can fail only if someone grabbed a reference in-between --
>>>  that's okay, this page is their problem now.
>>>
>> I am not sure this works. David raised a concern on V4[1], where if a
>> folio lands to page_frag_free()'s free_frozen_pages(), it will not be
>> properly handled. "this page is their problem" only works if every
>> folio/page free functions call a generic folio/page free function that
>> checks the page type and does the proper handling.
> 
>>> Refcount decrement now works as follows:
>>> 1. Counter decrement
>>> 2. If it is now zero, try to put it deep inside the dead zone
>>>     (CAS to INT_MIN). Or you can view it as "set up frozen bit and reset
>>>     counter".
>>> 3. This CAS can fail only if someone grabbed a reference in-between --
>>>     that's okay, this page is their problem now.
>>
>> I am not sure this works. David raised a concern on V4[1], where if a
>> folio lands to page_frag_free()'s free_frozen_pages(), it will not be
>> properly handled. "this page is their problem" only works if every
>> folio/page free functions call a generic folio/page free function that
>> checks the page type and does the proper handling.
> 
> After some thinking, this scenario is indeed possible and problematic. The
> complexity of the required setup give me hope that there are some implicit
> barriers, however I wasn't able to find them. But I am not an mm guru,
> so maybe I missed something :shrug:
> 
> -- Below is a minimal buggy scenario --
> 
> Thread A: network code that calls page_frag_free()
> Thread B: calls folio_try_get(). Maybe it is the DAMON paddr scanner,
>           maybe this folio was previously used in page cache
>           (however this noticeably complicates the scenario)
> Thread C: user of folio's reincarnation. It also requires destruction via
>           folio_put() and not via free_frozen_pages() -- for example hugetlb
>           or memcg charged folio.
> 
> Refcount operations as follows (FR = FROZEN):
> 
> A: DEC 1 -> 0  [ page_ref_dec_and_test() ]
> B: INC 0 -> 1  [ folio_try_get() ] => success
> B: DEC 1 -> 0  [ page_ref_dec_and_test() ]
> B: CAS 0 -> FR [ page_ref_dec_and_test() ]
> B: * deallocates via __folio_put() *
> 
> C: * re-allocates page *
> C: DEC 1 -> 0  [ page_ref_dec_and_test() ]
> A: CAS 0 -> FR [ page_ref_dec_and_test() ] => success
> A: * deallocates directly via free_frozen_pages() * => BUG
> 
> This can be fixed with a `s/page_frag_free/folio_put/`-style patch, however
> it seems unreasonable...

There needs to be a common page freeing function that dipatches stuff.

folio_put() is not the best future-proof thing, because not all pages will be
folios sooninsh.

-- 
Cheers,

David