Re: [PATCH 1/4] mm/mm_init: use deferred_init_memmap_chunk() in deferred_grow_zone()

[Mike Rapoport <rppt@kernel.org>]

On Tue, Aug 19, 2025 at 11:51:58PM +0000, Wei Yang wrote:
> On Tue, Aug 19, 2025 at 01:54:46PM +0300, Mike Rapoport wrote:
> >On Tue, Aug 19, 2025 at 09:52:23AM +0000, Wei Yang wrote:
> >> Hi, Mike
> >> 
> >> After going through the code again, I have some trivial thoughts to discuss
> >> with you. If not right, please let me know.
> >> 
> >> On Mon, Aug 18, 2025 at 09:46:12AM +0300, Mike Rapoport wrote:
> >> 
> >> In the file above this line, there is a compare between first_deferred_pfn and
> >> its original value after grab pgdat_resize_lock.
> >
> >Do you mean this one:
> >
> >	if (first_deferred_pfn != pgdat->first_deferred_pfn) {
> >		pgdat_resize_unlock(pgdat, &flags);
> >		return true;
> >	}
> > 
> 
> Yes.
> 
> I am thinking something like this:
> 
>  	if (first_deferred_pfn != pgdat->first_deferred_pfn || 
> 	    first_deferred_pfn == ULONG_MAX)
> 
> This means
> 
>   * someone else has grow zone before we grab the lock
>   * or the whole zone has already been initialized

deferred_grow_zone() can be called only before deferred_init_memmap(), so
it's very unlikely that a zone will be completely initialized here. We
start with at least one section with each deferred zone and every call to
deferred_grow_zone() adds a section.

And even if that was a case and first_deferred_pfn is ULONG_MAX, the loop
below will end immediately, so I don't think additional condition here
would be helpful.
 
> >> I am thinking to compare first_deferred_pfn with ULONG_MAX, as it compared in
> >> deferred_init_memmap(). This indicate this zone has already been initialized
> >> totally.
> >
> >It may be another CPU ran deferred_grow_zone() and won the race for resize
> >lock. Then pgdat->first_deferred_pfn will be larger than
> >first_deferred_pfn, but still not entire zone would be initialized.
> > 
> >> Current code guard this by spfn < zone_end_pfn(zone). Maybe a check ahead
> >> would be more clear?
> >
> >Not sure I follow you here. The check that we don't pass zone_end_pfn is
> >inside the loop for every section we initialize.
> > 
> 
> In case the zone has been initialized totally, first_deferred_pfn = ULONG_MAX.
> 
> Then we come to the loop with initial state:
> 
>     spfn = ULONG_MAX
>     epfn = 0 (which is wrap around)
> 
> And loop condition check (spfn < zone_end_pfn(zone)) is false, so the loop is
> skipped. This is how we handle a fully initialized zone now.
> 
> Would this be a little un-common?

Why? The important thing is (spfn < zone_end_pfn(zone)) is false, and I
think that's good enough.
 
> >> > 
> >> >-	/* If the zone is empty somebody else may have cleared out the zone */
> >> >-	if (!deferred_init_mem_pfn_range_in_zone(&i, zone, &spfn, &epfn,
> >> >-						 first_deferred_pfn)) {
> >> >-		pgdat->first_deferred_pfn = ULONG_MAX;
> >> >-		pgdat_resize_unlock(pgdat, &flags);
> >> >-		/* Retry only once. */
> >> >-		return first_deferred_pfn != ULONG_MAX;
> >> >+	/*
> >> >+	 * Initialize at least nr_pages_needed in section chunks.
> >> >+	 * If a section has less free memory than nr_pages_needed, the next
> >> >+	 * section will be also initalized.
> 
> Nit, one typo here. s/initalized/initialized/

Thanks, will fix.
 
-- 
Sincerely yours,
Mike.