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

[Mike Rapoport <rppt@kernel.org>]

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.

So I don't see how pfn_valid() in deferred_pfn_valid() may ever return
false.
 
> >> 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

-- 
Sincerely yours,
Mike.