Re: [PATCH 02/12] mm/sparse: refactor sparse_sections_init()

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Thu, Sep 10, 2026 at 04:30:43PM +0200, David Hildenbrand (Arm) wrote:
> On 9/10/26 15:29, Lorenzo Stoakes (ARM) wrote:
> > On Wed, Sep 09, 2026 at 03:32:55PM +0200, David Hildenbrand (Arm) wrote:
> >> memory_present() really identifies+prepares all early sections so the
> >> initialization in sparse_init() can properly iterating them to
> >> initialize metadata.
> >>
> >> Let's just inline memory_present() into sparse_sections_init() and
> >> cleaning up the code a bit while at it: make it clear that we are operating
> >> on pfns.
> >>
> >> Note that we call set_section_nid() now only if the section
> >> was not already created earlier. Now, there is no more inconsistency
> >> between what we (temporarily) store in ms->section_mem_map and what
> >> we store in our section->nid array.
> >>
> >> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
> >
> > In general please keep move/refactor steps separate. It makes it harder to
> > review when two things are going on at one time.
> >
> > But I guess in this case the diff wouldn't be that different.
>
> Yeah, I actually want back and forth here and decided to keep it simple.

Yeah, this one's a bit of a blurry line but I think fine as it is!

>
> >
> > Anyway seems reasonable so:
> >
> > Acked-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
>
> Thanks!
>
> >
> >> ---
> >>  mm/sparse.c | 41 +++++++++++++++++------------------------
> >>  1 file changed, 17 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/mm/sparse.c b/mm/sparse.c
> >> index 6a6d258862904..36e3d854febc5 100644
> >> --- a/mm/sparse.c
> >> +++ b/mm/sparse.c
> >> @@ -179,22 +179,27 @@ static inline unsigned long first_present_section_nr(void)
> >>  	return next_present_section_nr(-1);
> >>  }
> >>
> >> -/* Record a memory area against a node. */
> >> -static void __init memory_present(int nid, unsigned long start, unsigned long end)
> >> +void __init sparse_sections_init(void)
> >>  {
> >> -	unsigned long pfn;
> >> +	unsigned long pfn, start_pfn, end_pfn;
> >> +	int i, nid;
> >> +
> >> +	sparse_extreme_init();
> >>
> >> -	start &= PAGE_SECTION_MASK;
> >> -	mminit_validate_memmodel_limits(&start, &end);
> >> -	for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {
> >> -		unsigned long section_nr = pfn_to_section_nr(pfn);
> >> -		struct mem_section *ms;
> >> +	for_each_mem_pfn_range(i, MAX_NUMNODES, &start_pfn, &end_pfn, &nid) {
> >> +		start_pfn &= PAGE_SECTION_MASK;
> >> +		mminit_validate_memmodel_limits(&start_pfn, &end_pfn);
> >>
> >> -		sparse_index_init(section_nr, nid);
> >> -		set_section_nid(section_nr, nid);
> >> +		for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
> >> +			unsigned long section_nr = pfn_to_section_nr(pfn);
> >> +			struct mem_section *ms;
> >>
> >> -		ms = __nr_to_section(section_nr);
> >> -		if (!ms->section_mem_map) {
> >> +			sparse_index_init(section_nr, nid);
> >> +			ms = __nr_to_section(section_nr);
> >> +			if (ms->section_mem_map)
> >> +				continue;
> >> +
> >> +			set_section_nid(section_nr, nid);
> >
> > So the main change seems to be calling set_section_nid() only if
> > !ms->section_mem_map (and obv. calculating ms earlier), as described in the
> > commit msg.
>
> Yes! I also played with having that in a standalone patch but judged that it's
> not really worth it. I can move it to a separate patch if you think it would be
> better!

No need, not really a big diff delta and you call it out in the commit msg!

>
> --
> Cheers,
>
> David

--
Cheers, Lorenzo