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

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

On Thu, Jun 13, 2024 at 10:21:08AM +0200, David Hildenbrand wrote:
>On 12.06.24 04:04, 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()
>> 
>> Also since the range passed to deferred_[init|free]_pages is always from
>> memblock.memory for those we have already allocated memmap to cover,
>> pfn_valid() always return true. Then we can remove related check.
>> 
>
>I'm surprised that we can completely get rid if the pfn_valid checks (which
>is great!), trusting Mike's review that this is all sane :)
>

I'm surprised too :-)

>> 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>
>> ---
>
>[...]
>
>>   /*
>>    * Initialize struct pages.  We minimize pfn page lookups and scheduler checks
>>    * 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)
>
>We nowadays prefer double tabs for indentation for the second+ parameter line
>in MM, like
>
>static unsigned long __init deferred_init_pages(struct zone *zone,
>		unsigned long pfn, unsigned long end_pfn)
>
>Usually results in less churn when renaming functions ... and reduces the
>LOC.
>

Will adjust it.

>
>>   {
>>   	int nid = zone_to_nid(zone);
>> -	unsigned long nr_pages = 0;
>> +	unsigned long nr_pages = end_pfn - pfn;
>>   	int zid = zone_idx(zone);
>> -	struct page *page = NULL;
>> +	struct page *page = pfn_to_page(pfn);
>> -	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++;
>> -		}
>> +	for (; pfn < end_pfn; pfn++, page++)
>>   		__init_single_page(page, pfn, zid, nid);
>> -		nr_pages++;
>> -	}
>>   	return nr_pages;
>>   }
>> @@ -2096,7 +2049,7 @@ deferred_init_maxorder(u64 *i, struct zone *zone, unsigned long *start_pfn,
>>   			break;
>>   		t = min(mo_pfn, epfn);
>> -		deferred_free_pages(spfn, t);
>> +		deferred_free_pages(spfn, t - spfn);
>>   		if (mo_pfn <= epfn)
>>   			break;
>
>Looks like a really nice cleanup!
>

Glad you like it :-)

>-- 
>Cheers,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me