Re: [PATCH 2/3] mm/percpu.c: optimize the code in pcpu_setup_first_chunk() a little bit

[Baoquan He]

On 07/21/23 at 02:01pm, Dennis Zhou wrote:
> Hello,
> 
> On Fri, Jul 21, 2023 at 09:17:59PM +0800, Baoquan He wrote:
> > This removes the need of local varibale 'chunk', and optimize the code
> > calling pcpu_alloc_first_chunk() to initialize reserved chunk and
> > dynamic chunk to make it simpler.
> > 
> > Signed-off-by: Baoquan He <bhe@xxxxxxxxxx>
> > ---
> >  mm/percpu.c | 32 +++++++++++++-------------------
> >  1 file changed, 13 insertions(+), 19 deletions(-)
> > 
> > diff --git a/mm/percpu.c b/mm/percpu.c
> > index 1480bf283d11..c25b058a46ad 100644
> > --- a/mm/percpu.c
> > +++ b/mm/percpu.c
> > @@ -2581,7 +2581,6 @@ void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
> >  {
> >  	size_t size_sum = ai->static_size + ai->reserved_size + ai->dyn_size;
> >  	size_t static_size, dyn_size;
> > -	struct pcpu_chunk *chunk;
> >  	unsigned long *group_offsets;
> >  	size_t *group_sizes;
> >  	unsigned long *unit_off;
> > @@ -2697,7 +2696,7 @@ void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
> >  	pcpu_unit_pages = ai->unit_size >> PAGE_SHIFT;
> >  	pcpu_unit_size = pcpu_unit_pages << PAGE_SHIFT;
> >  	pcpu_atom_size = ai->atom_size;
> > -	pcpu_chunk_struct_size = struct_size(chunk, populated,
> > +	pcpu_chunk_struct_size = struct_size((struct pcpu_chunk *)0, populated,
> >  					     BITS_TO_LONGS(pcpu_unit_pages));
> >  
> >  	pcpu_stats_save_ai(ai);
> > @@ -2735,28 +2734,23 @@ void __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
> >  
> >  	/*
> >  	 * Initialize first chunk.
> > -	 * If the reserved_size is non-zero, this initializes the reserved
> > -	 * chunk.  If the reserved_size is zero, the reserved chunk is NULL
> > -	 * and the dynamic region is initialized here.  The first chunk,
> > -	 * pcpu_first_chunk, will always point to the chunk that serves
> > -	 * the dynamic region.
> > +	 * If the reserved_size is non-zero, initializes the reserved chunk
>                                          ^initialize
> > +	 * firstly. If the reserved_size is zero, the reserved chunk is NULL
>         ^ can remove firstly.
> > +	 * and the dynamic region is initialized directly. The first chunk,
> > +	 * pcpu_first_chunk, will always point to the chunk that serves the
> > +	 * dynamic region.
> 
> Reading this, I'll probably reword this comment to explain the reserved
> chunk better.

Agree. The expression is a little messy and too colloquial.

> 
> >  	 */
> >  	tmp_addr = (unsigned long)base_addr + static_size;
> > -	map_size = ai->reserved_size ?: dyn_size;
> > -	chunk = pcpu_alloc_first_chunk(tmp_addr, map_size);
> > -
> > -	/* init dynamic chunk if necessary */
> >  	if (ai->reserved_size) {
> > -		pcpu_reserved_chunk = chunk;
> > -
> > -		tmp_addr = (unsigned long)base_addr + static_size +
> > -			   ai->reserved_size;
> > -		map_size = dyn_size;
> > -		chunk = pcpu_alloc_first_chunk(tmp_addr, map_size);
> > +		map_size = ai->reserved_size;
> > +		pcpu_reserved_chunk = pcpu_alloc_first_chunk(tmp_addr, map_size);
> >  	}
> >  
> > -	/* link the first chunk in */
> > -	pcpu_first_chunk = chunk;
> > +	/* init dynamic chunk if necessary */
> > +	tmp_addr += (unsigned long)ai->reserved_size;
> 
> I'm not a big fan of += the tmp_addr as I personally find it easier to
> read if it's just laid out explicitly.

OK, will change.

> 
> > +	map_size = dyn_size;
> > +	pcpu_first_chunk = pcpu_alloc_first_chunk(tmp_addr, map_size);
> > +
> >  	pcpu_nr_empty_pop_pages = pcpu_first_chunk->nr_empty_pop_pages;
> >  	pcpu_chunk_relocate(pcpu_first_chunk, -1);
> >  
> > -- 
> > 2.34.1
> > 
> 
> Overall, I think this is good, but I'd go 1 step further and get rid of
> map_size. Regarding tmp_addr, I'd prefer if we kept all the math
> together.

Makes sense. Thanks a lot for your careful review and great suggestions.

According to your comments, I made a draft v2. Please help check if I
have got them correctly and if the new change is OK to you.