Re: [PATCH] [RFC] mm/mm_init.c: simplify logic of deferred_[init|free]_pages

[Wei Yang <richard.weiyang@gmail.com>]

On Mon, Jun 10, 2024 at 09:40:33AM +0300, Mike Rapoport wrote:
>On Wed, Jun 05, 2024 at 01:07:42AM +0000, Wei Yang wrote:
>> Function deferred_[init|free]_pages are only used in
>> deferred_init_maxorder(), which makes sure the range to init/free is
>> within MAX_ORDER_NR_PAGES size.
>> 
>> With this knowledge, we can simplify these two functions. Since
>> 
>>   * only the first pfn could be IS_MAX_ORDER_ALIGNED()
>
>No, the first pfn is not necessarily IS_MAX_ORDER_ALIGNED(). Start pfn is a
>beginning of a region in memblock.memory, and there's no guarantee on it's
>alignment.
> 

Yes, I mean only the first pfn is possible to be IS_MAX_ORDER_ALIGNED(), not
must be IS_MAX_ORDER_ALIGNED().

The range passed to deferred_[init|free]_pages must be within one
MAX_ORDER_NR_PAGES. If the first pfn is not IS_MAX_ORDER_ALIGNED(), others
could not be IS_MAX_ORDER_ALIGNED().

Currently these two functions would iterate all pfn, and check
IS_MAX_ORDER_ALIGNED() to break init|free on each MAX_ORDER_NR_PAGES. But this
is only possible and necessary on the first pfn.

>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> CC: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> CC: Mike Rapoport (IBM) <rppt@kernel.org>
>> CC: David Hildenbrand <david@redhat.com>
>> 
>> ---
>> But my question is why we just test pfn_valid for the
>> IS_MAX_ORDER_ALIGNED pfn? I thought we should test pfn_valid for each
>> pfn until the first one in MAX_ORDER pages. Do I miss something?
>> ---
>>  mm/mm_init.c | 43 ++++++++++++++-----------------------------
>>  1 file changed, 14 insertions(+), 29 deletions(-)
>> 
>> diff --git a/mm/mm_init.c b/mm/mm_init.c
>> index bbaf3a2c1cfd..6a4adf9e7d9a 100644
>> --- a/mm/mm_init.c
>> +++ b/mm/mm_init.c
>> @@ -1970,21 +1970,10 @@ static inline bool __init deferred_pfn_valid(unsigned long pfn)
>>  static void __init deferred_free_pages(unsigned long pfn,
>>  				       unsigned long end_pfn)
>>  {
>> -	unsigned long nr_free = 0;
>> -
>> -	for (; pfn < end_pfn; pfn++) {
>> -		if (!deferred_pfn_valid(pfn)) {
>> -			deferred_free_range(pfn - nr_free, nr_free);
>> -			nr_free = 0;
>> -		} else if (IS_MAX_ORDER_ALIGNED(pfn)) {
>> -			deferred_free_range(pfn - nr_free, nr_free);
>> -			nr_free = 1;
>> -		} else {
>> -			nr_free++;
>> -		}
>> -	}
>> -	/* Free the last block of pages to allocator */
>> -	deferred_free_range(pfn - nr_free, nr_free);
>> +	if (!deferred_pfn_valid(pfn))
>> +		pfn++;
>> +
>> +	deferred_free_range(pfn, end_pfn - pfn);
>>  }
>>  
>>  /*
>> @@ -1992,27 +1981,23 @@ static void __init deferred_free_pages(unsigned long pfn,
>>   * by performing it only once every MAX_ORDER_NR_PAGES.
>>   * Return number of pages initialized.
>>   */
>> -static unsigned long  __init deferred_init_pages(struct zone *zone,
>> -						 unsigned long pfn,
>> -						 unsigned long end_pfn)
>> +static unsigned long __init deferred_init_pages(struct zone *zone,
>> +						unsigned long pfn,
>> +						unsigned long end_pfn)
>>  {
>>  	int nid = zone_to_nid(zone);
>>  	unsigned long nr_pages = 0;
>>  	int zid = zone_idx(zone);
>>  	struct page *page = NULL;
>>  
>> -	for (; pfn < end_pfn; pfn++) {
>> -		if (!deferred_pfn_valid(pfn)) {
>> -			page = NULL;
>> -			continue;
>> -		} else if (!page || IS_MAX_ORDER_ALIGNED(pfn)) {
>> -			page = pfn_to_page(pfn);
>> -		} else {
>> -			page++;
>> -		}
>> +	if (!deferred_pfn_valid(pfn))
>> +		pfn++;
>> +
>> +	page = pfn_to_page(pfn);
>> +	nr_pages = end_pfn - pfn;
>> +
>> +	for (; pfn < end_pfn; pfn++, page++)
>>  		__init_single_page(page, pfn, zid, nid);
>> -		nr_pages++;
>> -	}
>>  	return nr_pages;
>>  }
>>  
>> -- 
>> 2.34.1
>> 
>
>-- 
>Sincerely yours,
>Mike.

-- 
Wei Yang
Help you, Help me