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

["David Hildenbrand (Arm)" <david@kernel.org>]

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.

> 
> 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!

-- 
Cheers,

David