On Tue, Nov 27, 2018 at 08:52:14AM +0100, osalvador@xxxxxxx wrote:
>> I think mem_hotplug_lock protects this case these days, though. I don't
>> think we had it in the early days and were just slumming it with the
>> pgdat locks.
>
>Yes, it does.
>
>>
>> I really don't like the idea of removing the lock by just saying it
>> doesn't protect anything without doing some homework first, though. It
>> would actually be really nice to comment the entire call chain from the
>> mem_hotplug_lock acquisition to here. There is precious little
>> commenting in there and it could use some love.
>
>[hot-add operation]
>add_memory_resource : acquire mem_hotplug lock
> arch_add_memory
> add_pages
> __add_pages
> __add_section
> sparse_add_one_section
> sparse_init_one_section
>
>[hot-remove operation]
>__remove_memory : acquire mem_hotplug lock
> arch_remove_memory
> __remove_pages
> __remove_section
> sparse_remove_one_section
>
Thanks for this detailed analysis.
>Both operations are serialized by the mem_hotplug lock, so they cannot step
>on each other's feet.
>
>Now, there seems to be an agreement/thought to remove the global mem_hotplug
>lock, in favor of a range locking for hot-add/remove and online/offline
>stage.
>So, although removing the lock here is pretty straightforward, it does not
>really get us closer to that goal IMHO, if that is what we want to do in the
>end.
>
My current idea is :
we can try to get rid of global mem_hotplug_lock in logical
online/offline phase first, and leave the physical add/remove phase
serialized by mem_hotplug_lock for now.
There are two phase in memory hotplug:
* physical add/remove phase
* logical online/offline phase
Currently, both of them are protected by the global mem_hotplug_lock.
While get rid of the this in logical online/offline phase is a little
easier to me, since this procedure is protected by memory_block_dev's lock.
This ensures there is no pfn over lap during parallel processing.
The physical add/remove phase is a little harder, because it will touch
* memblock
* kernel page table
* node online
* sparse mem
And I don't see a similar lock as memory_block_dev's lock.
Below is the call trace for these two phase and I list some suspicious
point which is not safe without mem_hotplug_lock.
1. physical phase
__add_memory()
register_memory_resource() <- protected by resource_lock
add_memory_resource()
mem_hotplug_begin()
memblock_add_node() <- add to memblock.memory, not safe
__try_online_node() <- not safe, related to node_set_online()
arch_add_memory()
init_memory_mapping() <- not safe
add_pages()
__add_pages()
__add_section()
sparse_add_one_section()
update_end_of_memory_vars() <- not safe
node_set_online(nid) <- need to hold mem_hotplug
__register_one_node(nid)
link_mem_sections()
firmware_map_add_hotplug()
mem_hotplug_done()
2. logical phase
device_lock(memory_block_dev)
online_pages()
mem_hotplug_begin()
mem = find_memory_block() <- not
zone = move_pfn_range()
zone_for_pfn_range();
move_pfn_range_to_zone()
!populated_zone()
setup_zone_pageset(zone)
online_pages_range() <- looks safe
build_all_zonelists() <- not
init_per_zone_wmark_min() <- not
kswapd_run() <- may not
vm_total_pages = nr_free_pagecache_pages()
mem_hotplug_done()
device_unlock(memory_block_dev)
--
Wei Yang
Help you, Help me
