Re: [PATCH v3 4/4] mm/sparse: Optimize memmap allocation during sparse_init()

[Baoquan He <bhe@redhat.com>]

Hi Dave,

Sorry for late reply.

On 04/11/18 at 08:48am, Dave Hansen wrote:
> On 04/08/2018 01:20 AM, Baoquan He wrote:
> > On 04/06/18 at 07:50am, Dave Hansen wrote:
> >> The code looks fine to me.  It's a bit of a shame that there's no
> >> verification to ensure that idx_present never goes beyond the shiny new
> >> nr_present_sections. 
> > 
> > This is a good point. Do you think it's OK to replace (section_nr <
> > NR_MEM_SECTIONS) with (section_nr < nr_present_sections) in below
> > for_each macro? This for_each_present_section_nr() is only used
> > during sparse_init() execution.
> > 
> > #define for_each_present_section_nr(start, section_nr)          \
> >         for (section_nr = next_present_section_nr(start-1);     \
> >              ((section_nr >= 0) &&                              \
> >               (section_nr < NR_MEM_SECTIONS) &&                 \                                                                                 
> >               (section_nr <= __highest_present_section_nr));    \
> >              section_nr = next_present_section_nr(section_nr))
> 
> I was more concerned about the loops that "consume" the section maps.
> It seems like they might run over the end of the array.



> 
> >>> @@ -583,6 +592,7 @@ void __init sparse_init(void)
> >>>  	unsigned long *usemap;
> >>>  	unsigned long **usemap_map;
> >>>  	int size;
> >>> +	int idx_present = 0;
> >>
> >> I wonder whether idx_present is a good name.  Isn't it the number of
> >> consumed mem_map[]s or usemaps?
> > 
> > Yeah, in sparse_init(), it's the index of present memory sections, and
> > also the number of consumed mem_map[]s or usemaps. And I remember you
> > suggested nr_consumed_maps instead. seems nr_consumed_maps is a little
> > long to index array to make code line longer than 80 chars. How about
> > name it idx_present in sparse_init(), nr_consumed_maps in
> > alloc_usemap_and_memmap(), the maps allocation function? I am also fine
> > to use nr_consumed_maps for all of them.
> 
> Does the large array index make a bunch of lines wrap or something?  If
> not, I'd just use the long name.

I am fine with the long name, will use 'nr_consumed_maps' you suggested
earlier to replace.

> 
> >>>  		if (!map) {
> >>>  			ms->section_mem_map = 0;
> >>> +			idx_present++;
> >>>  			continue;
> >>>  		}
> >>>  
> >>
> >>
> >> This hunk seems logically odd to me.  I would expect a non-used section
> >> to *not* consume an entry from the temporary array.  Why does it?  The
> >> error and success paths seem to do the same thing.
> > 
> > Yes, this place is the hardest to understand. The temorary arrays are
> > allocated beforehand with the size of 'nr_present_sections'. The error
> > paths you mentioned is caused by allocation failure of mem_map or
> > map_map, but whatever it's error or success paths, the sections must be
> > marked as present in memory_present(). Error or success paths happened
> > in alloc_usemap_and_memmap(), while checking if it's erorr or success
> > paths happened in the last for_each_present_section_nr() of
> > sparse_init(), and clear the ms->section_mem_map if it goes along error
> > paths. This is the key point of this new allocation way.
> 
> I think you owe some commenting because this is so hard to understand.

I can arrange and write a code comment above sparse_init() according to
this patch's git log, do you think it's OK?

Honestly, it took me several days to write code, while I spent more
than one week to write the patch log. Writing patch log is really a
headache to me.

Thanks
Baoquan