Re: [PATCH v2 1/2] mm: drop page refcount zero state semantics

[Gorbunov Ivan <gorbunov.ivan@h-partners.com>]

On 4/23/2026 10:32 PM, Zi Yan wrote:
> On 20 Apr 2026, at 4:01, Gorbunov Ivan wrote:
> 
>> This patch proposes 2 changes
>> 1) Deprecate invariant that the value stored in reference count of frozen page is 0
>> (Getter functions folio_ref_count/page_ref_count must still return 0 for frozen pages)
>> 2) Allow modification operations like page_ref_add to be used only with
>>     pages with owners
> 
> Should we also ban calling set_page_count() except init_page_count() and
> set_page_count_as_frozen()? So that no one can manipulate page refcount
> randomly. The same applies to folio_set_count().
> 
> All set_page_count(..., 0) are converted to set_page_count_as_frozen().
> All set_page_count(..., 1) are converted to init_page_count().
> 
> We might need an init_folio_count() for folio_set_count(..., 1) users
> and folio_set_count() has no other use.
> 
> I see set_page_count(page, PAGE_FRAG_CACHE_MAX_SIZE + 1); from
> mm/page_frag_cache.c and it could be converted to page_ref_unfreeze(),
> since above it there is free_frozen_page(), which makes me think the
> page is frozen.
> 

Overall I agree, but this is out of scope for this patch main change. 
This could be handled as an independent patch later.

>>   static inline int page_ref_dec_return(struct page *page)
>>   {
>>   	int ret = atomic_dec_return(&page->_refcount);
>> +	VM_BUG_ON(__page_count_is_frozen(ret + 1));
>>
>>   	if (page_ref_tracepoint_active(page_ref_mod_and_return))
>>   		__page_ref_mod_and_return(page, -1, ret);
> 
> VM_WARN_ON_ONCE() might be better?
> 

With "locking via dedicated bit" patch modification, any function which 
drop reference counter to zero changes the behavior of 
refcount(page_add_unless would succeed and etc.). This could lead to 
hazardous results. Therefore, we decided to prohibit that completely and 
make zero value only possible as middle state during dec_and_test.


> 
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 65e702fade61..27734cf795da 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -1639,14 +1639,14 @@ void __meminit __free_pages_core(struct page *page, unsigned int order,
>>   		for (loop = 0; loop < nr_pages; loop++, p++) {
>>   			VM_WARN_ON_ONCE(PageReserved(p));
>>   			__ClearPageOffline(p);
>> -			set_page_count(p, 0);
>> +			set_page_count_as_frozen(p);
>>   		}
>>
>>   		adjust_managed_page_count(page, nr_pages);
>>   	} else {
>>   		for (loop = 0; loop < nr_pages; loop++, p++) {
>>   			__ClearPageReserved(p);
>> -			set_page_count(p, 0);
>> +			set_page_count_as_frozen(p);
>>   		}
>>
>>   		/* memblock adjusts totalram_pages() manually. */
>> -- 
> 
> Not sure about these two, they freeze p and p goes into buddy without
> unfreeze to refcount==0. But at the beginning, you said there are two
> states:
> 1) Unfrozen page which right now has no explicit owner
> 2) Frozen page
> 
> 1) means free pages in buddy. But the above change brings frozen pages
> into buddy, mixing the two states into one. Is that intended?


Since state 1 only appears during dec_and_test, pages in the buddy 
allocator are considered frozen. So there is no mixing of states.