Re: [PATCH 08/12] mm/sparse: move __highest_used_section_nr handling

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

On Thu, Sep 10, 2026 at 04:29:06PM +0200, David Hildenbrand (Arm) wrote:
> On 9/10/26 16:16, Lorenzo Stoakes (ARM) wrote:
> > On Wed, Sep 09, 2026 at 03:33:01PM +0200, David Hildenbrand (Arm) wrote:
> >> Let's move it to sparse_init_one_section(). However, to keep early
> >> boot processing working, we also have to initialize it in
> >> sparse_sections_init().
> >
> > A why might be nice :)
>
> Agreed, I'll mention that.
>
> >
> > I guess preparing for removal of __section_mark_present()?
> >
>
> Right intuition :)
>
> > Also good to have arguments as to why this is equivalent of previous
> > behaviour.
> >
> > E.g. higher pfn = higher section nr so naturally the highest is the one you
> > end up wtih at the end of sparse_sections_init()?
>
> We go over all sections, just earlier.
>
> >
> >>
> >> Should we use READ_ONCE/WRITE_ONCE with __highest_used_section_nr?
> >> Probably, something for another day.
> >
> > It might be worth expanding this a bit. Do multiple threads read/write this
> > concurrently?
>
> Hah, I'll probably just drop it. I was just stumbling over readers vs.
> concurrent updates and thought "that looks suspicious".

Ack. KCSAN will bring our sins back to bear if they matter anyway :>)

>
> >
> > No functional change intended here or is one intended? :)
> >
>
> Certainly no change intended ;)
>
> > Before it was:
> >
> > mm_core_init_early() -> sparse_sections_init() -> __section_mark_present()
> >                           sparse_add_section() -> __section_mark_present()
> >
> > Now:
> >
> >                                                  mm_core_init_early() -> sparse_sections_init() [early]
> >                                                  sparse_add_section() -> sparse_init_one_section()
> >
> > But also called from mm_core_init_early():
> >
> > sparse_init() -> sparse_metadata_init() -> sparse_metadata_init_nid() -> sparse_init_one_section()
> >
> > Are both required?
>
> sparse_metadata_init() relies on __highest_used_section_nr in the
> for_each_early_section_nr / for_each_present_section_nr, so it is required.

Ahh yeah, makes sense. Worth spelling that out :)

>
> I could probable move the update on the hotplug side into sparse_add_section()
> instead!

Ack yeah would separate things out a bit between the two!

>
> >
> >
> >>
> >> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
> >> ---
> >>  mm/sparse.c | 5 +++--
> >>  mm/sparse.h | 6 +++---
> >>  2 files changed, 6 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/mm/sparse.c b/mm/sparse.c
> >> index 2b41ae36f20b8..2d0f2db34f4cf 100644
> >> --- a/mm/sparse.c
> >> +++ b/mm/sparse.c
> >> @@ -177,7 +177,7 @@ static inline unsigned long first_present_section_nr(void)
> >>
> >>  void __init sparse_sections_init(void)
> >>  {
> >> -	unsigned long pfn, start_pfn, end_pfn;
> >> +	unsigned long pfn, start_pfn, end_pfn, section_nr;
> >>  	int i, nid;
> >>
> >>  	sparse_extreme_init();
> >> @@ -187,9 +187,9 @@ void __init sparse_sections_init(void)
> >>  		mminit_validate_memmodel_limits(&start_pfn, &end_pfn);
> >>
> >>  		for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
> >> -			unsigned long section_nr = pfn_to_section_nr(pfn);
> >>  			struct mem_section *ms;
> >>
> >> +			section_nr = pfn_to_section_nr(pfn);
> >>  			sparse_index_init(section_nr, nid);
> >>  			ms = __nr_to_section(section_nr);
> >>  			if (ms->section_mem_map)
> >> @@ -201,6 +201,7 @@ void __init sparse_sections_init(void)
> >>  			__section_mark_present(ms, section_nr);
> >>  		}
> >>  	}
> >> +	__highest_used_section_nr = section_nr;
> >>  }
> >>
> >>  #ifndef CONFIG_SPARSEMEM_VMEMMAP
> >> diff --git a/mm/sparse.h b/mm/sparse.h
> >> index 7c5d82ceb7142..a3af4967fd5c5 100644
> >> --- a/mm/sparse.h
> >> +++ b/mm/sparse.h
> >> @@ -97,6 +97,9 @@ static inline void sparse_init_one_section(struct mem_section *ms,
> >>
> >>  	BUILD_BUG_ON(SECTION_MAP_LAST_BIT > PFN_SECTION_SHIFT);
> >>
> >> +	if (section_nr > __highest_used_section_nr)
> >> +		__highest_used_section_nr = section_nr;
> >> +
> >
> > Could also be:
> >
> > 	section_nr = max(section_nr, __highest_used_section_nr);
>
> Ack!
>
> --
> Cheers,
>
> David

--
Cheers, Lorenzo