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

[Wei Yang <richard.weiyang@gmail.com>]

On Tue, Nov 20, 2018 at 08:31:41AM +0100, Michal Hocko wrote:
>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.

Sorry for misunderstanding your point.

>
>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.

The global lock you want to remove is mem_hotplug_begin() ?

Hmm... my understanding may not correct. While mem_hotplug_begin() use
percpu lock, which means if there are two threads running on two
different cpus to online pages at the same time, they could get their
own lock?

If this is the case, will we face the race condition here?

>
>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.

Agree, I will try to update the documentation in next verstion. 

>
>> 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

-- 
Wei Yang
Help you, Help me