Re: [PATCH v1] mm/memory_hotplug: Don't take the cpu_hotplug_lock

[David Hildenbrand]

On 24.09.19 16:48, Michal Hocko wrote:
> On Tue 24-09-19 16:36:15, David Hildenbrand wrote:
>> Since commit 3f906ba23689 ("mm/memory-hotplug: switch locking to a percpu
>> rwsem") we do a cpus_read_lock() in mem_hotplug_begin(). This was
>> introduced to fix a potential deadlock between get_online_mems() and
>> get_online_cpus() - the memory and cpu hotplug lock. The root issue was
>> that build_all_zonelists() -> stop_machine() required the cpu hotplug lock:
>>     The reason is that memory hotplug takes the memory hotplug lock and
>>     then calls stop_machine() which calls get_online_cpus().  That's the
>>     reverse lock order to get_online_cpus(); get_online_mems(); in
>>     mm/slub_common.c
>>
>> So memory hotplug never really required any cpu lock itself, only
>> stop_machine() and lru_add_drain_all() required it. Back then,
>> stop_machine_cpuslocked() and lru_add_drain_all_cpuslocked() were used
>> as the cpu hotplug lock was now obtained in the caller.
>>
>> Since commit 11cd8638c37f ("mm, page_alloc: remove stop_machine from build
>> all_zonelists"), the stop_machine_cpuslocked() call is gone.
>> build_all_zonelists() does no longer require the cpu lock and does no
>> longer make use of stop_machine().
>>
>> Since commit 9852a7212324 ("mm: drop hotplug lock from
>> lru_add_drain_all()"), lru_add_drain_all() "Doesn't need any cpu hotplug
>> locking because we do rely on per-cpu kworkers being shut down before our
>> page_alloc_cpu_dead callback is executed on the offlined cpu.". The
>> lru_add_drain_all_cpuslocked() variant was removed.
>>
>> So there is nothing left that requires the cpu hotplug lock. The memory
>> hotplug lock and the device hotplug lock are sufficient.
> 
> I would love to see this happen. The biggest offenders should be gone. 
> I really hated how those two locks have been conflated which likely
> resulted in some undocumented/unintended dependencies. So for now, I
> cannot really tell you whether the patch is correct. It would really
> require a lot of testing. I am not sure this is reasonably reviewable.

At least judging from the obvious history, this should be fine. But the
bad parts are undocumented side effects that sneaked in in the meantime.
I agree that reviewing this is hard - too much hidden and undocumented
complexity.

> 
> So please add some testing results (ideally cpu hotplug racing a lot
> with the memory hotplug). Then I would be willing to give this a try
> and see. First by keeping it in linux-next for a release or two and then
> eyes closed, fingers crossed and go to the wild. Do we have a tag for
> that Dared-by maybe?

I'll do some more testing to try to find the obvious issues. But I
suspect if there are still some leftovers, they will be hard to trigger.
So I agree that keeping this in linux-next for a longer period sounds
like a good idea.

Tentatively-Acked-by ? :D

> 
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Oscar Salvador <osalvador@xxxxxxx>
>> Cc: Michal Hocko <mhocko@xxxxxxxx>
>> Cc: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>> ---
>>
>> RFC -> v1:
>> - Reword and add more details why the cpu hotplug lock was needed here
>>   in the first place, and why we no longer require it.
>>
>> ---
>>  mm/memory_hotplug.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index c3e9aed6023f..5fa30f3010e1 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -88,14 +88,12 @@ __setup("memhp_default_state=", setup_memhp_default_state);
>>  
>>  void mem_hotplug_begin(void)
>>  {
>> -	cpus_read_lock();
>>  	percpu_down_write(&mem_hotplug_lock);
>>  }
>>  
>>  void mem_hotplug_done(void)
>>  {
>>  	percpu_up_write(&mem_hotplug_lock);
>> -	cpus_read_unlock();
>>  }
>>  
>>  u64 max_mem_size = U64_MAX;
>> -- 
>> 2.21.0
> 


-- 

Thanks,

David / dhildenb