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

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

On Tue, Nov 27, 2018 at 02:16:58PM +0100, Michal Hocko wrote:
>On Tue 27-11-18 03:12:00, Wei Yang wrote:
>> On Mon, Nov 26, 2018 at 09:16:08AM +0100, Michal Hocko wrote:
>> >On Mon 26-11-18 10:28:40, Wei Yang wrote:
>> >[...]
>> >> But I get some difficulty to understand this TODO. You want to get rid of
>> >> these lock? While these locks seem necessary to protect those data of
>> >> pgdat/zone. Would you mind sharing more on this statement?
>> >
>> >Why do we need this lock to be irqsave? Is there any caller that uses
>> >the lock from the IRQ context?
>> 
>> Went through the code, we have totally 9 place acquire
>> pgdat_resize_lock:
>> 
>>    lib/show_mem.c:         1    show_mem()
>>    mm/memory_hotplug.c:    4    online/offline_pages/__remove_zone()
>>    mm/page_alloc.c:        2    defer_init
>>    mm/sparse.c:            2    not necessary
>> 
>> Two places I am not sure:
>> 
>>    * show_mem() would be called from __alloc_pages_slowpath()
>
>This shouldn't really need the lock. It is a mostly debugging aid rather
>than something that cannot tolarate racing with hotplug. What is the
>worst case that can happen?
>

Agree.

The worst case is debug information is not exact in case defer init or
hotplug happens at the same time. While this is a rare case.

If you think it is ok, I would suggest to remove the lock here.

>>    * __remove_zone() is related to acpi_scan() on x86, may related to
>>      other method on different arch
>
>This one really needs a lock qwith a larger scope anyway.

Based on my understanding, __remove_zone() happens at physical memory
remove phase. While for currently logic, we adjust zone information at
logic memory online phase.

They looks not consistent?

If we could do this at logical memory offline phase, we are sure this is
not in IRQ context.

>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me