Re: [PATCH v2 6/9] mm: page_alloc: add alloc_contig_frozen_pages()

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2025/9/10 3:08, Zi Yan wrote:
> On 9 Sep 2025, at 14:55, Matthew Wilcox wrote:
> 
>> On Tue, Sep 02, 2025 at 08:48:17PM +0800, Kefeng Wang wrote:
>>> Introduce a ACR_FLAGS_FROZEN flags to indicate that we want to
>>> allocate a frozen compound pages by alloc_contig_range(), also
>>> provide alloc_contig_frozen_pages() to allocate pages without
>>> incrementing their refcount, which may be beneficial to some
>>> users (eg hugetlb).
>>
>> No, this is not the way to do it.  We're moving towards a future where
>> pages don't have refcounts (their owning structs eg folios may have a
>> refcount, but struct page does not have a refcount).  So work towards
>> that; if you need to keep compatibility with some users, then by all
>> means keep alloc_contig_range_noprof() as a wrapper that calls
>> alloc_contig_range_frozen_noprof() and then calls set_page_refcounted()
>> if alloc_contig_range_frozen_noprof() does not return NULL.  But I'd
>> be looking to see if alloc_contig_range_noprof() can always return a
>> non-refcounted page.
> 

Yes, Zi has been comment this shown below, alloc_contig_pages_noprof()
is called by may driver(directly or indirectly), I originally only hoped
to minimize the changes, if we follow your plan, we can do a bit more,
let me try the new way. Also I will separate this patchset to let the 
cleanup patch could be merged first.

> Something like:
> 
> struct page *alloc_contig_pages_noprof(unsigned long nr_pages, gfp_t gfp_mask,
> 		int nid, nodemask_t *nodemask)
> {
> 	struct page *page = __alloc_frozen_contig_pages_noprof(...);
> 	
> 	// just added page == NULL check
> 	if (!page)
> 		return NULL;
> 
> 	if (gfp_mask & __GFP_COMP)
> 		set_page_refcounted(page);
> 	else {
> 		unsigned long i;
> 
> 		for (i = 0; i < nr_pages; i++)
> 			set_page_refcounted(page + i);
> 	}
> 	return page;
> }
> 
> as I suggested in another email[1] (of course I forgot to check page == NULL).
> 
> And other TODOs:
> 
> 1. remove set_page_refcounted() in split_free_pages();
> 2. a new split_frozen_page() to do split_page() work without
>     set_page_refcounted() and split_page() can just call it;
> 3. a new free_frozen_contig_range() and free_contig_range()
>     calls it.
> 


Thanks Matthew/Zi.


> 
> [1] https://lore.kernel.org/linux-mm/298B40E0-8EA5-4667-86EF-22F88B832839@nvidia.com/
> 
> Best Regards,
> Yan, Zi