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

[Muhammad Usama Anjum <usama.anjum@arm.com>]

<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?
> 
> As long as it's an internal helper, that makes sense. I wouldn't want to
> expose the bool in the external interface.
> 
> Thanks!
> 

Thanks,
Usama