Re: [PATCH] mm, sparse: drop pgdat_resize_lock in sparse_add/remove_one_section()

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

On Tue, Nov 27, 2018 at 08:52:14AM +0100, osalvador@suse.de 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