Re: [PATCH v3 3/3] mm/page_alloc: Optimize __free_contig_frozen_range()

[Zi Yan <ziy@nvidia.com>]

On 25 Mar 2026, at 12:03, Muhammad Usama Anjum wrote:

> <snip>
>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>> index 250cc07e547b8..26eac35ef73bd 100644
>>>> --- a/mm/page_alloc.c
>>>> +++ b/mm/page_alloc.c
>>>> @@ -7038,8 +7038,30 @@ static int __alloc_contig_verify_gfp_mask(gfp_t gfp_mask, gfp_t *gfp_cc_mask)
>>>>
>>>>  static void __free_contig_frozen_range(unsigned long pfn, unsigned long nr_pages)
>>>>  {
>>>> -	for (; nr_pages--; pfn++)
>>>> -		free_frozen_pages(pfn_to_page(pfn), 0);
>>>> +	struct page *page = pfn_to_page(pfn);
>>>> +	struct page *start = NULL;
>>>> +	unsigned long start_sec;
>>>> +	unsigned long i;
>>>> +
>>>> +	for (i = 0; i < nr_pages; i++, page++) {
>>>> +		if (!free_pages_prepare(page, 0)) {
>>>> +			if (start) {
>>>> +				free_prepared_contig_range(start, page - start);
>>>> +				start = NULL;
>>>> +			}
>>>> +		} else if (start &&
>>>> +			   memdesc_section(page->flags) != start_sec) {
>>>> +			free_prepared_contig_range(start, page - start);
>>>> +			start = page;
>>>> +			start_sec = memdesc_section(page->flags);
>>>> +		} else if (!start) {
>>>> +			start = page;
>>>> +			start_sec = memdesc_section(page->flags);
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (start)
>>>> +		free_prepared_contig_range(start, page - start);
>>>>  }
>>>
>>> This looks almost the same as __free_contig_range().
>>>
>>> Two approaches to deduplicate the code:
>>>
>>> 1. __free_contig_range() first does put_page_testzero()
>>> on all pages and call __free_contig_frozen_range()
>>> on the range, __free_contig_frozen_range() will need
>>> to skip not frozen pages. It is not ideal.
>>
>> Right, let's not do that.
>>
>>>
>>> 2. add a helper function
>>> __free_contig_range_common(unsigned long pfn,
>>> unsigned long nr_pages, bool is_page_frozen),
>>> and
>>> a. call __free_contig_range_common(..., /*is_page_frozen=*/ false)
>>> in __free_contig_range(),
>>> b. __free_contig_range_common(..., /*is_page_frozen=*/ true)
>>> in __free_contig_frozen_range().
>>>
> I'm adding the common version. After the change, I'm thinking about the current functions and if
> they can be simplified further:
>
> free_contig_range()	- only calls __free_contig_range()
> 			- only visible with CONFIG_CONTIG_ALLOC
> 			- Exported as well
>
> __free_contig_range()	- only calls __free_contig_range_common(is_frozen=false)
> 			- visible even without CONFIG_CONTIG_ALLOC as vfree() uses it
> 			- Exported as well (there is no user of this export at this time)
>
> __free_contig_frozen_range()	- only calls __free_contig_range_common(is_frozen=true)
>
> __free_contig_range_common()	- it does the actual work
>
> vfree()->free_pages_bulk()	- calls __free_contig_range()
>
> Should we remove __free_contig_range() and __free_contig_frozen_range() both entirely and
> just use __free_contig_range_common() everywhere?

The idea of adding __free_contig_range_common() is to deduplicate code.
__free_contig_range() and __free_contig_frozen_range() improves readability,
since people do not need to see the /* is_page_frozen= */ true/false
in __free_contig_range_common().

>>
>> As long as it's an internal helper, that makes sense. I wouldn't want to
>> expose the bool in the external interface.
>>

Like what David said above.


Best Regards,
Yan, Zi