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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 11, 2024 at 12:58:30PM +0300, Mike Rapoport wrote:
>On Mon, Jun 10, 2024 at 02:54:57PM +0000, Wei Yang wrote:
>> 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.
>
>Thinking more about it, it looks to me that deferred_pfn_valid() cannot
>ever return false.
>
>The ranges that are passed to deferred_{free,init}_pages are always from
>memblock.memory and we allocate the memory map to cover memblock.memory
>with at least MAX_ORDER_NR_PAGES alignment.
>

I need another look into this, forget where it ensure this requirement.

>So I don't see how pfn_valid() in deferred_pfn_valid() may ever return
>false.
> 

The code is originally intended to work for pageblock, so maybe we can remove
this and simplify it more.

>> >> Signed-off-by: Wei Yang <richard.weiyang@xxxxxxxxx>
>> >> CC: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
>> >> CC: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>
>> >> CC: David Hildenbrand <david@xxxxxxxxxx>
>> >> 
>> >> ---
>> >> 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
>
>-- 
>Sincerely yours,
>Mike.

-- 
Wei Yang
Help you, Help me




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux