Re: [PATCH 4/6] mm, memory_hotplug: reorganize new pgdat initialization

[Oscar Salvador]

On 2022-01-27 09:53, Michal Hocko wrote:
> From: Michal Hocko <mhocko@xxxxxxxx>
>
> When a !node_online node is brought up it needs a hotplug specific
> initialization because the node could be either uninitialized yet or it
> could have been recycled after previous hotremove. hotadd_init_pgdat is
> responsible for that.
>
> Internal pgdat state is initialized at two places currently
> 	- hotadd_init_pgdat
> 	- free_area_init_core_hotplug
> There is no real clear cut what should go where but this patch's chosen 
> to
> move the whole internal state initialization into 
> free_area_init_core_hotplug.
> hotadd_init_pgdat is still responsible to pull all the parts together -
> most notably to initialize zonelists because those depend on the
> overall topology.
>
> This patch doesn't introduce any functional change.
>
> Acked-by: Rafael Aquini <raquini@xxxxxxxxxx>
> Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>

I have some comments below:

Reviewed-by: Oscar Salvador <osalvador@xxxxxxx>

>  	/*
>  	 * The node we allocated has no zone fallback lists. For avoiding
> @@ -1210,6 +1187,7 @@ static pg_data_t __ref *hotadd_init_pgdat(int 
> nid)
>  	 * When memory is hot-added, all the memory is in offline state. So
>  	 * clear all zones' present_pages because they will be updated in
>  	 * online_pages() and offline_pages().
> +	 * TODO: should be in free_area_init_core_hotplug?
>  	 */
>  	reset_node_managed_pages(pgdat);
>  	reset_node_present_pages(pgdat);

Most likely yes, but more below.

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 1a05669044d3..32d0189de4c5 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7506,12 +7506,33 @@ static void __meminit
> zone_init_internals(struct zone *zone, enum zone_type idx,
>   * NOTE: this function is only called during memory hotplug
>   */
>  #ifdef CONFIG_MEMORY_HOTPLUG
> -void __ref free_area_init_core_hotplug(int nid)
> +void __ref free_area_init_core_hotplug(struct pglist_data *pgdat)
>  {
> +	int nid = pgdat->node_id;
>  	enum zone_type z;
> -	pg_data_t *pgdat = NODE_DATA(nid);
> +	int cpu;
>
>  	pgdat_init_internals(pgdat);
> +
> +	if (pgdat->per_cpu_nodestats == &boot_nodestats)
> +		pgdat->per_cpu_nodestats = alloc_percpu(struct per_cpu_nodestat);
> +
> +	/*
> +	 * Reset the nr_zones, order and highest_zoneidx before reuse.
> +	 * Note that kswapd will init kswapd_highest_zoneidx properly
> +	 * when it starts in the near future.
> +	 */
> +	pgdat->nr_zones = 0;
> +	pgdat->kswapd_order = 0;
> +	pgdat->kswapd_highest_zoneidx = 0;
> +	pgdat->node_start_pfn = 0;
> +	for_each_online_cpu(cpu) {
> +		struct per_cpu_nodestat *p;
> +
> +		p = per_cpu_ptr(pgdat->per_cpu_nodestats, cpu);
> +		memset(p, 0, sizeof(*p));
> +	}
> +

I am with David that the cleanest way would be to initialize all of this
(pgdat internals) only once at boot time now that we are initializing 
the
node anyway, and memory-offlining would have to wipe it out once last 
pgdat's
chunk of memory goes away.

I guess free_area_init_core_hotplug() would still need to allocate
alloc_percpu(struct per_cpu_nodestat) for the "new" node since doing it 
at
boot time is not desirable because of the memory waste on unused nodes.

As you said, probably worth looking at but not really something to cover 
on
this patchset at all.

--
Oscar Salvador
SUSE Labs