Re: [PATCH] mm, hotplug: protect nr_zones with pgdat_resize_lock()

[Michal Hocko <mhocko@suse.com>]

On Tue 20-11-18 09:48:22, Wei Yang wrote:
> After memory hot-added, users could online pages through sysfs, and this
> could be done in parallel.
> 
> In case two threads online pages in two different empty zones at the
> same time, there would be a contention to update the nr_zones.

No, this shouldn't be the case as I've explained in the original thread.
We use memory hotplug lock over the online phase. So there shouldn't be
any race possible.

On the other hand I would like to see the global lock to go away because
it causes scalability issues and I would like to change it to a range
lock. This would make this race possible.

That being said this is more of a preparatory work than a fix. One could
argue that pgdat resize lock is abused here but I am not convinced a
dedicated lock is much better. We do take this lock already and spanning
its scope seems reasonable. An update to the documentation is due.

> The patch use pgdat_resize_lock() to protect this critical section.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

After the changelog is updated to reflect the above, feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/page_alloc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index e13987c2e1c4..525a5344a13b 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5796,9 +5796,12 @@ void __meminit init_currently_empty_zone(struct zone *zone,
>  {
>  	struct pglist_data *pgdat = zone->zone_pgdat;
>  	int zone_idx = zone_idx(zone) + 1;
> +	unsigned long flags;
>  
> +	pgdat_resize_lock(pgdat, &flags);
>  	if (zone_idx > pgdat->nr_zones)
>  		pgdat->nr_zones = zone_idx;
> +	pgdat_resize_unlock(pgdat, &flags);
>  
>  	zone->zone_start_pfn = zone_start_pfn;
>  
> -- 
> 2.15.1

-- 
Michal Hocko
SUSE Labs