Re: [PATCH 3/9] mm, page_alloc: remove setup_pageset()

[Vlastimil Babka]

On 10/5/20 2:59 PM, Michal Hocko wrote:
> On Tue 22-09-20 16:37:06, Vlastimil Babka wrote:
> > We initialize boot-time pagesets with setup_pageset(), which sets high and
> > batch values that effectively disable pcplists.
> >
> > We can remove this wrapper if we just set these values for all pagesets in
> > pageset_init(). Non-boot pagesets then subsequently update them to the proper
> > values.
> >
> > No functional change.
> >
> > Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
> > ---
> >  mm/page_alloc.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 26069c8d1b19..76c2b4578723 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -5823,7 +5823,7 @@ static void build_zonelists(pg_data_t *pgdat)
> >   * not check if the processor is online before following the pageset pointer.
> >   * Other parts of the kernel may not check if the zone is available.
> >   */
> > -static void setup_pageset(struct per_cpu_pageset *p);
> > +static void pageset_init(struct per_cpu_pageset *p);
> >  static DEFINE_PER_CPU(struct per_cpu_pageset, boot_pageset);
> >  static DEFINE_PER_CPU(struct per_cpu_nodestat, boot_nodestats);
> >  
> > @@ -5891,7 +5891,7 @@ build_all_zonelists_init(void)
> >  	 * (a chicken-egg dilemma).
> >  	 */
> >  	for_each_possible_cpu(cpu)
> > -		setup_pageset(&per_cpu(boot_pageset, cpu));
> > +		pageset_init(&per_cpu(boot_pageset, cpu));
> >  
> >  	mminit_verify_zonelist();
> >  	cpuset_init_current_mems_allowed();
> > @@ -6210,12 +6210,15 @@ static void pageset_init(struct per_cpu_pageset *p)
> >  	pcp = &p->pcp;
> >  	for (migratetype = 0; migratetype < MIGRATE_PCPTYPES; migratetype++)
> >  		INIT_LIST_HEAD(&pcp->lists[migratetype]);
> > -}
> >  
> > -static void setup_pageset(struct per_cpu_pageset *p)
> > -{
> > -	pageset_init(p);
> > -	pageset_update(&p->pcp, 0, 1);
> > +	/*
> > +	 * Set batch and high values safe for a boot pageset. A true percpu
> > +	 * pageset's initialization will update them subsequently. Here we don't
> > +	 * need to be as careful as pageset_update() as nobody can access the
> > +	 * pageset yet.
>
> Isn't this slightly misleading? pageset_init is called from setup_zone_pageset
> which is called from the memory hotplug as well. Isn't this more about

It is.

> early zone initialization rather than boot pagesets? Or am I misreading
> the patch?

It's for boot pagesets (call from build_all_zonelists_init()) that we need these 
particular values (well, just high = 0). For early zone initialization during 
hotplug the values don't matter as the caller is setup_zone_pageset() who 
immediately replaces the values by zone_set_pageset_high_and_batch(). Which is 
what the comment tries to say.

> > +	 */
> > +	pcp->high = 0;
> > +	pcp->batch = 1;
> >  }
> >  
> >  /*
> > --
> > 2.28.0