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

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

[Wei Yang]

pgdat_resize_lock is used to protect pgdat's memory region information
like: node_start_pfn, node_present_pages, etc.

In function sparse_add/remove_one_section(), those data is not touched.
This means it is not necessary to acquire pgdat_resize_lock to protect
this area.

Since the information needed in sparse_add_one_section() is node id to
allocate proper memory. This patch also changes the prototype of
sparse_add_one_section() to pass node id directly. This is intended to
reduce misleading that sparse_add_one_section() would touch pgdat.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 include/linux/memory_hotplug.h |  2 +-
 mm/memory_hotplug.c            |  2 +-
 mm/sparse.c                    | 17 +++++------------
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 45a5affcab8a..3787d4e913e6 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
 		unsigned long nr_pages, struct vmem_altmap *altmap);
 extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
 extern bool is_memblock_offlined(struct memory_block *mem);
-extern int sparse_add_one_section(struct pglist_data *pgdat,
+extern int sparse_add_one_section(int nid,
 		unsigned long start_pfn, struct vmem_altmap *altmap);
 extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		unsigned long map_offset, struct vmem_altmap *altmap);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index f626e7e5f57b..5b3a3d7b4466 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
 	if (pfn_valid(phys_start_pfn))
 		return -EEXIST;
 
-	ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap);
+	ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
 	if (ret < 0)
 		return ret;
 
diff --git a/mm/sparse.c b/mm/sparse.c
index 33307fc05c4d..a4fdbcb21514 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -662,25 +662,24 @@ static void free_map_bootmem(struct page *memmap)
  * set.  If this is <=0, then that means that the passed-in
  * map was not consumed and must be freed.
  */
-int __meminit sparse_add_one_section(struct pglist_data *pgdat,
-		unsigned long start_pfn, struct vmem_altmap *altmap)
+int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
+				     struct vmem_altmap *altmap)
 {
 	unsigned long section_nr = pfn_to_section_nr(start_pfn);
 	struct mem_section *ms;
 	struct page *memmap;
 	unsigned long *usemap;
-	unsigned long flags;
 	int ret;
 
 	/*
 	 * no locking for this, because it does its own
 	 * plus, it does a kmalloc
 	 */
-	ret = sparse_index_init(section_nr, pgdat->node_id);
+	ret = sparse_index_init(section_nr, nid);
 	if (ret < 0 && ret != -EEXIST)
 		return ret;
 	ret = 0;
-	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
+	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
 	if (!memmap)
 		return -ENOMEM;
 	usemap = __kmalloc_section_usemap();
@@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 		return -ENOMEM;
 	}
 
-	pgdat_resize_lock(pgdat, &flags);
-
 	ms = __pfn_to_section(start_pfn);
 	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
 		ret = -EEXIST;
@@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 	sparse_init_one_section(ms, section_nr, memmap, usemap);
 
 out:
-	pgdat_resize_unlock(pgdat, &flags);
 	if (ret < 0) {
 		kfree(usemap);
 		__kfree_section_memmap(memmap, altmap);
@@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		unsigned long map_offset, struct vmem_altmap *altmap)
 {
 	struct page *memmap = NULL;
-	unsigned long *usemap = NULL, flags;
-	struct pglist_data *pgdat = zone->zone_pgdat;
+	unsigned long *usemap = NULL;
 
-	pgdat_resize_lock(pgdat, &flags);
 	if (ms->section_mem_map) {
 		usemap = ms->pageblock_flags;
 		memmap = sparse_decode_mem_map(ms->section_mem_map,
@@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		ms->section_mem_map = 0;
 		ms->pageblock_flags = NULL;
 	}
-	pgdat_resize_unlock(pgdat, &flags);
 
 	clear_hwpoisoned_pages(memmap + map_offset,
 			PAGES_PER_SECTION - map_offset);
-- 
2.15.1

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

[Michal Hocko]

[Cc Dave who has added the lock into this path. Maybe he remembers why]

On Tue 27-11-18 10:36:30, Wei Yang wrote:
> pgdat_resize_lock is used to protect pgdat's memory region information
> like: node_start_pfn, node_present_pages, etc.
> 
> In function sparse_add/remove_one_section(), those data is not touched.
> This means it is not necessary to acquire pgdat_resize_lock to protect
> this area.
> 
> Since the information needed in sparse_add_one_section() is node id to
> allocate proper memory. This patch also changes the prototype of
> sparse_add_one_section() to pass node id directly. This is intended to
> reduce misleading that sparse_add_one_section() would touch pgdat.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  include/linux/memory_hotplug.h |  2 +-
>  mm/memory_hotplug.c            |  2 +-
>  mm/sparse.c                    | 17 +++++------------
>  3 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 45a5affcab8a..3787d4e913e6 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
>  		unsigned long nr_pages, struct vmem_altmap *altmap);
>  extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
>  extern bool is_memblock_offlined(struct memory_block *mem);
> -extern int sparse_add_one_section(struct pglist_data *pgdat,
> +extern int sparse_add_one_section(int nid,
>  		unsigned long start_pfn, struct vmem_altmap *altmap);
>  extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		unsigned long map_offset, struct vmem_altmap *altmap);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index f626e7e5f57b..5b3a3d7b4466 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>  	if (pfn_valid(phys_start_pfn))
>  		return -EEXIST;
>  
> -	ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap);
> +	ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 33307fc05c4d..a4fdbcb21514 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -662,25 +662,24 @@ static void free_map_bootmem(struct page *memmap)
>   * set.  If this is <=0, then that means that the passed-in
>   * map was not consumed and must be freed.
>   */
> -int __meminit sparse_add_one_section(struct pglist_data *pgdat,
> -		unsigned long start_pfn, struct vmem_altmap *altmap)
> +int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> +				     struct vmem_altmap *altmap)
>  {
>  	unsigned long section_nr = pfn_to_section_nr(start_pfn);
>  	struct mem_section *ms;
>  	struct page *memmap;
>  	unsigned long *usemap;
> -	unsigned long flags;
>  	int ret;
>  
>  	/*
>  	 * no locking for this, because it does its own
>  	 * plus, it does a kmalloc
>  	 */
> -	ret = sparse_index_init(section_nr, pgdat->node_id);
> +	ret = sparse_index_init(section_nr, nid);
>  	if (ret < 0 && ret != -EEXIST)
>  		return ret;
>  	ret = 0;
> -	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
> +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
>  	if (!memmap)
>  		return -ENOMEM;
>  	usemap = __kmalloc_section_usemap();
> @@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  		return -ENOMEM;
>  	}
>  
> -	pgdat_resize_lock(pgdat, &flags);
> -
>  	ms = __pfn_to_section(start_pfn);
>  	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
>  		ret = -EEXIST;
> @@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  	sparse_init_one_section(ms, section_nr, memmap, usemap);
>  
>  out:
> -	pgdat_resize_unlock(pgdat, &flags);
>  	if (ret < 0) {
>  		kfree(usemap);
>  		__kfree_section_memmap(memmap, altmap);
> @@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		unsigned long map_offset, struct vmem_altmap *altmap)
>  {
>  	struct page *memmap = NULL;
> -	unsigned long *usemap = NULL, flags;
> -	struct pglist_data *pgdat = zone->zone_pgdat;
> +	unsigned long *usemap = NULL;
>  
> -	pgdat_resize_lock(pgdat, &flags);
>  	if (ms->section_mem_map) {
>  		usemap = ms->pageblock_flags;
>  		memmap = sparse_decode_mem_map(ms->section_mem_map,
> @@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		ms->section_mem_map = 0;
>  		ms->pageblock_flags = NULL;
>  	}
> -	pgdat_resize_unlock(pgdat, &flags);
>  
>  	clear_hwpoisoned_pages(memmap + map_offset,
>  			PAGES_PER_SECTION - map_offset);
> -- 
> 2.15.1
> 

-- 
Michal Hocko
SUSE Labs

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

[Dave Hansen]

On 11/26/18 10:25 PM, Michal Hocko wrote:
> [Cc Dave who has added the lock into this path. Maybe he remembers why]

I don't remember specifically.  But, the pattern of:

	allocate
	lock
	set
	unlock

is _usually_ so we don't have two "sets" racing with each other.  In
this case, that would have been to ensure that two
sparse_init_one_section()'s didn't race and leak one of the two
allocated memmaps or worse.

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.

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.

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

[Michal Hocko]

On Mon 26-11-18 23:17:40, Dave Hansen wrote:
> On 11/26/18 10:25 PM, Michal Hocko wrote:
> > [Cc Dave who has added the lock into this path. Maybe he remembers why]
> 
> I don't remember specifically.  But, the pattern of:
> 
> 	allocate
> 	lock
> 	set
> 	unlock
> 
> is _usually_ so we don't have two "sets" racing with each other.  In
> this case, that would have been to ensure that two
> sparse_init_one_section()'s didn't race and leak one of the two
> allocated memmaps or worse.
> 
> 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.
> 
> 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.

Agreed. It really seems like the lock is not needed anymore but a more
trhougout analysis and explanation is definitely due.

Thanks!

-- 
Michal Hocko
SUSE Labs

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

[osalvador]

> 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

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.

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

[Michal Hocko]

[I am mostly offline and will be so tomorrow as well]

On Tue 27-11-18 08:52:14, osalvador@suse.de wrote:
[...]
> 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.

But it doesn't get us further either, right? This patch shouldn't make
any plan for range locking any worse. Both adding and removing a sparse
section is pfn range defined unless I am missing something.
-- 
Michal Hocko
SUSE Labs

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

[osalvador]

On 2018-11-27 09:00, Michal Hocko wrote:
> [I am mostly offline and will be so tomorrow as well]
> 
> On Tue 27-11-18 08:52:14, osalvador@suse.de wrote:
> [...]
>> 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.
> 
> But it doesn't get us further either, right? This patch shouldn't make
> any plan for range locking any worse. Both adding and removing a sparse
> section is pfn range defined unless I am missing something.

Yes, you are right, it should not have any impact.
It just felt like "we do already have the global lock, so let us stick 
with that".
But the less unneeded locks we have in our way, the better.

Sorry for the confusion.

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

[Wei Yang]

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

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

[Wei Yang]

On Mon, Nov 26, 2018 at 11:17:40PM -0800, Dave Hansen wrote:
>On 11/26/18 10:25 PM, Michal Hocko wrote:
>> [Cc Dave who has added the lock into this path. Maybe he remembers why]
>
>I don't remember specifically.  But, the pattern of:
>
>	allocate
>	lock
>	set
>	unlock
>
>is _usually_ so we don't have two "sets" racing with each other.  In
>this case, that would have been to ensure that two
>sparse_init_one_section()'s didn't race and leak one of the two
>allocated memmaps or worse.
>
>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.
>
>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.

Dave,

Thanks for your comment :-)

I should put more words to the reason for removing the lock.

Here is a simplified call trace for sparse_add_one_section() during
physical add/remove phase.

    __add_memory()
        add_memory_resource()
    	mem_hotplug_begin()
    
    	arch_add_memory()
    	    add_pages()
    	        __add_pages()
    	            __add_section()
    	                sparse_add_one_section(pfn)
    
    	mem_hotplug_done()

When we just look at the sparse section initialization, we can see the
contention happens when __add_memory() try to add a same range or range
overlapped in SECTIONS_PER_ROOT number of sections. Otherwise, they
won't access the same memory. 

If this happens, we may face two contentions:

    * reallocation of mem_section[root]
    * reallocation of memmap and usemap

While neither of them could be protected by the pgdat_resize_lock from
my understanding. Grab pgdat_resize_lock just slow down the process,
while finally they will replace the mem_section[root] and
ms->section_mem_map with their own new allocated data.

Last bu not the least, to be honest, even the global mem_hotplug_lock
doesn't help in this situation. In case __add_memory() try to add the
same range twice, the sparse section would be initialized twice. Which
means it will be overwritten with the new allocated memmap/usermap.

But maybe we have the assumption this reentrance will not happen.

This is all what I understand, in case there is some misunderstanding,
please let me know.

-- 
Wei Yang
Help you, Help me

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

[Oscar Salvador]

> My current idea is :

I do not want to hold you back.
I think that if you send a V2 detailing why we should be safe removing
the pgdat lock it is fine (memhotplug lock protects us).

We can later on think about the range locking, but that is another
discussion.
Sorry for having brought in that topic here, out of scope.

-- 
Oscar Salvador
SUSE L3

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

[Wei Yang]

On Wed, Nov 28, 2018 at 09:19:27AM +0100, Oscar Salvador wrote:
>> My current idea is :
>
>I do not want to hold you back.
>I think that if you send a V2 detailing why we should be safe removing
>the pgdat lock it is fine (memhotplug lock protects us).

Fine.

>
>We can later on think about the range locking, but that is another
>discussion.
>Sorry for having brought in that topic here, out of scope.
>
>-- 
>Oscar Salvador
>SUSE L3

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Wed, Nov 28, 2018 at 01:01:12AM +0000, Wei Yang wrote:
>On Mon, Nov 26, 2018 at 11:17:40PM -0800, Dave Hansen wrote:
>>On 11/26/18 10:25 PM, Michal Hocko wrote:
>>> [Cc Dave who has added the lock into this path. Maybe he remembers why]
>>
>>I don't remember specifically.  But, the pattern of:
>>
>>	allocate
>>	lock
>>	set
>>	unlock
>>
>>is _usually_ so we don't have two "sets" racing with each other.  In
>>this case, that would have been to ensure that two
>>sparse_init_one_section()'s didn't race and leak one of the two
>>allocated memmaps or worse.
>>
>>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.
>>
>>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.
>
>Dave,
>
>Thanks for your comment :-)
>
>I should put more words to the reason for removing the lock.
>
>Here is a simplified call trace for sparse_add_one_section() during
>physical add/remove phase.
>
>    __add_memory()
>        add_memory_resource()
>    	mem_hotplug_begin()
>    
>    	arch_add_memory()
>    	    add_pages()
>    	        __add_pages()
>    	            __add_section()
>    	                sparse_add_one_section(pfn)
>    
>    	mem_hotplug_done()
>
>When we just look at the sparse section initialization, we can see the
>contention happens when __add_memory() try to add a same range or range
>overlapped in SECTIONS_PER_ROOT number of sections. Otherwise, they
>won't access the same memory. 
>
>If this happens, we may face two contentions:
>
>    * reallocation of mem_section[root]
>    * reallocation of memmap and usemap
>
>While neither of them could be protected by the pgdat_resize_lock from
>my understanding. Grab pgdat_resize_lock just slow down the process,
>while finally they will replace the mem_section[root] and
>ms->section_mem_map with their own new allocated data.
>

Hmm... sorry, I am not correct here.

The pgdat_resize_lock do protect the second case.

But not the first one.

>Last bu not the least, to be honest, even the global mem_hotplug_lock
>doesn't help in this situation. In case __add_memory() try to add the
>same range twice, the sparse section would be initialized twice. Which
>means it will be overwritten with the new allocated memmap/usermap.
>

The mem_section[root] still has a chance to face the contention here.

>But maybe we have the assumption this reentrance will not happen.
>
>This is all what I understand, in case there is some misunderstanding,
>please let me know.

I will rewrite the changelog to emphasize this process is protected by
the global mem_hotplug_lock.

>
>-- 
>Wei Yang
>Help you, Help me

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

In function sparse_add/remove_one_section(), pgdat_resize_lock is used
to protect initialization/release of one mem_section. This looks not
necessary for current implementation.

Following is the current call trace of sparse_add/remove_one_section()

    mem_hotplug_begin()
    arch_add_memory()
       add_pages()
           __add_pages()
               __add_section()
                   sparse_add_one_section()
    mem_hotplug_done()

    mem_hotplug_begin()
    arch_remove_memory()
        __remove_pages()
            __remove_section()
                sparse_remove_one_section()
    mem_hotplug_done()

which shows these functions is protected by the global mem_hotplug_lock.
It won't face contention when accessing the mem_section.

Since the information needed in sparse_add_one_section() is node id to
allocate proper memory. This patch also changes the prototype of
sparse_add_one_section() to pass node id directly. This is intended to
reduce misleading that sparse_add_one_section() would touch pgdat.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

---
v2:
   * adjust changelog to show this procedure is serialized by global
     mem_hotplug_lock
---
 include/linux/memory_hotplug.h |  2 +-
 mm/memory_hotplug.c            |  2 +-
 mm/sparse.c                    | 17 +++++------------
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 45a5affcab8a..3787d4e913e6 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
 		unsigned long nr_pages, struct vmem_altmap *altmap);
 extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
 extern bool is_memblock_offlined(struct memory_block *mem);
-extern int sparse_add_one_section(struct pglist_data *pgdat,
+extern int sparse_add_one_section(int nid,
 		unsigned long start_pfn, struct vmem_altmap *altmap);
 extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		unsigned long map_offset, struct vmem_altmap *altmap);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index f626e7e5f57b..5b3a3d7b4466 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
 	if (pfn_valid(phys_start_pfn))
 		return -EEXIST;
 
-	ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap);
+	ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
 	if (ret < 0)
 		return ret;
 
diff --git a/mm/sparse.c b/mm/sparse.c
index 33307fc05c4d..a4fdbcb21514 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -662,25 +662,24 @@ static void free_map_bootmem(struct page *memmap)
  * set.  If this is <=0, then that means that the passed-in
  * map was not consumed and must be freed.
  */
-int __meminit sparse_add_one_section(struct pglist_data *pgdat,
-		unsigned long start_pfn, struct vmem_altmap *altmap)
+int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
+				     struct vmem_altmap *altmap)
 {
 	unsigned long section_nr = pfn_to_section_nr(start_pfn);
 	struct mem_section *ms;
 	struct page *memmap;
 	unsigned long *usemap;
-	unsigned long flags;
 	int ret;
 
 	/*
 	 * no locking for this, because it does its own
 	 * plus, it does a kmalloc
 	 */
-	ret = sparse_index_init(section_nr, pgdat->node_id);
+	ret = sparse_index_init(section_nr, nid);
 	if (ret < 0 && ret != -EEXIST)
 		return ret;
 	ret = 0;
-	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
+	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
 	if (!memmap)
 		return -ENOMEM;
 	usemap = __kmalloc_section_usemap();
@@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 		return -ENOMEM;
 	}
 
-	pgdat_resize_lock(pgdat, &flags);
-
 	ms = __pfn_to_section(start_pfn);
 	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
 		ret = -EEXIST;
@@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 	sparse_init_one_section(ms, section_nr, memmap, usemap);
 
 out:
-	pgdat_resize_unlock(pgdat, &flags);
 	if (ret < 0) {
 		kfree(usemap);
 		__kfree_section_memmap(memmap, altmap);
@@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		unsigned long map_offset, struct vmem_altmap *altmap)
 {
 	struct page *memmap = NULL;
-	unsigned long *usemap = NULL, flags;
-	struct pglist_data *pgdat = zone->zone_pgdat;
+	unsigned long *usemap = NULL;
 
-	pgdat_resize_lock(pgdat, &flags);
 	if (ms->section_mem_map) {
 		usemap = ms->pageblock_flags;
 		memmap = sparse_decode_mem_map(ms->section_mem_map,
@@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		ms->section_mem_map = 0;
 		ms->pageblock_flags = NULL;
 	}
-	pgdat_resize_unlock(pgdat, &flags);
 
 	clear_hwpoisoned_pages(memmap + map_offset,
 			PAGES_PER_SECTION - map_offset);
-- 
2.15.1

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

[Wei Yang]

On Wed, Nov 28, 2018 at 08:47:29AM +0000, Wei Yang wrote:
>>
>>Dave,
>>
>>Thanks for your comment :-)
>>
>>I should put more words to the reason for removing the lock.
>>
>>Here is a simplified call trace for sparse_add_one_section() during
>>physical add/remove phase.
>>
>>    __add_memory()
>>        add_memory_resource()
>>    	mem_hotplug_begin()
>>    
>>    	arch_add_memory()
>>    	    add_pages()
>>    	        __add_pages()
>>    	            __add_section()
>>    	                sparse_add_one_section(pfn)
>>    
>>    	mem_hotplug_done()
>>
>>When we just look at the sparse section initialization, we can see the
>>contention happens when __add_memory() try to add a same range or range
>>overlapped in SECTIONS_PER_ROOT number of sections. Otherwise, they
>>won't access the same memory. 
>>
>>If this happens, we may face two contentions:
>>
>>    * reallocation of mem_section[root]
>>    * reallocation of memmap and usemap
>>
>>While neither of them could be protected by the pgdat_resize_lock from
>>my understanding. Grab pgdat_resize_lock just slow down the process,
>>while finally they will replace the mem_section[root] and
>>ms->section_mem_map with their own new allocated data.
>>
>
>Hmm... sorry, I am not correct here.
>
>The pgdat_resize_lock do protect the second case.
>
>But not the first one.
>

One more thing, (hope I am not too talkative)

Expand the pgdat_resize_lock to include sparse_index_init() may not
work. Because SECTIONS_PER_ROOT number of section may span two nodes.


-- 
Wei Yang
Help you, Help me

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

[David Hildenbrand]

On 28.11.18 10:12, Wei Yang wrote:
> In function sparse_add/remove_one_section(), pgdat_resize_lock is used
> to protect initialization/release of one mem_section. This looks not
> necessary for current implementation.
> 
> Following is the current call trace of sparse_add/remove_one_section()
> 
>     mem_hotplug_begin()
>     arch_add_memory()
>        add_pages()
>            __add_pages()
>                __add_section()
>                    sparse_add_one_section()
>     mem_hotplug_done()
> 
>     mem_hotplug_begin()
>     arch_remove_memory()
>         __remove_pages()
>             __remove_section()
>                 sparse_remove_one_section()
>     mem_hotplug_done()
> 
> which shows these functions is protected by the global mem_hotplug_lock.
> It won't face contention when accessing the mem_section.
> 
> Since the information needed in sparse_add_one_section() is node id to
> allocate proper memory. This patch also changes the prototype of
> sparse_add_one_section() to pass node id directly. This is intended to
> reduce misleading that sparse_add_one_section() would touch pgdat.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

Reviewed-by: David Hildenbrand <david@redhat.com>

> 
> ---
> v2:
>    * adjust changelog to show this procedure is serialized by global
>      mem_hotplug_lock
> ---
>  include/linux/memory_hotplug.h |  2 +-
>  mm/memory_hotplug.c            |  2 +-
>  mm/sparse.c                    | 17 +++++------------
>  3 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 45a5affcab8a..3787d4e913e6 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
>  		unsigned long nr_pages, struct vmem_altmap *altmap);
>  extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
>  extern bool is_memblock_offlined(struct memory_block *mem);
> -extern int sparse_add_one_section(struct pglist_data *pgdat,
> +extern int sparse_add_one_section(int nid,
>  		unsigned long start_pfn, struct vmem_altmap *altmap);
>  extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		unsigned long map_offset, struct vmem_altmap *altmap);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index f626e7e5f57b..5b3a3d7b4466 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>  	if (pfn_valid(phys_start_pfn))
>  		return -EEXIST;
>  
> -	ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap);
> +	ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 33307fc05c4d..a4fdbcb21514 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -662,25 +662,24 @@ static void free_map_bootmem(struct page *memmap)
>   * set.  If this is <=0, then that means that the passed-in
>   * map was not consumed and must be freed.
>   */
> -int __meminit sparse_add_one_section(struct pglist_data *pgdat,
> -		unsigned long start_pfn, struct vmem_altmap *altmap)
> +int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> +				     struct vmem_altmap *altmap)
>  {
>  	unsigned long section_nr = pfn_to_section_nr(start_pfn);
>  	struct mem_section *ms;
>  	struct page *memmap;
>  	unsigned long *usemap;
> -	unsigned long flags;
>  	int ret;
>  
>  	/*
>  	 * no locking for this, because it does its own
>  	 * plus, it does a kmalloc
>  	 */
> -	ret = sparse_index_init(section_nr, pgdat->node_id);
> +	ret = sparse_index_init(section_nr, nid);
>  	if (ret < 0 && ret != -EEXIST)
>  		return ret;
>  	ret = 0;
> -	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
> +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
>  	if (!memmap)
>  		return -ENOMEM;
>  	usemap = __kmalloc_section_usemap();
> @@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  		return -ENOMEM;
>  	}
>  
> -	pgdat_resize_lock(pgdat, &flags);
> -
>  	ms = __pfn_to_section(start_pfn);
>  	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
>  		ret = -EEXIST;
> @@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  	sparse_init_one_section(ms, section_nr, memmap, usemap);
>  
>  out:
> -	pgdat_resize_unlock(pgdat, &flags);
>  	if (ret < 0) {
>  		kfree(usemap);
>  		__kfree_section_memmap(memmap, altmap);
> @@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		unsigned long map_offset, struct vmem_altmap *altmap)
>  {
>  	struct page *memmap = NULL;
> -	unsigned long *usemap = NULL, flags;
> -	struct pglist_data *pgdat = zone->zone_pgdat;
> +	unsigned long *usemap = NULL;
>  
> -	pgdat_resize_lock(pgdat, &flags);
>  	if (ms->section_mem_map) {
>  		usemap = ms->pageblock_flags;
>  		memmap = sparse_decode_mem_map(ms->section_mem_map,
> @@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		ms->section_mem_map = 0;
>  		ms->pageblock_flags = NULL;
>  	}
> -	pgdat_resize_unlock(pgdat, &flags);
>  
>  	clear_hwpoisoned_pages(memmap + map_offset,
>  			PAGES_PER_SECTION - map_offset);
> 


-- 

Thanks,

David / dhildenb

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

[Michal Hocko]

On Wed 28-11-18 08:47:29, Wei Yang wrote:
[...]
> The mem_section[root] still has a chance to face the contention here.

_If_ that is really the case then we need a dedicated lock rather than
rely on pgdat which doesn't even make sense for sparsemem internal.

-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Wed 28-11-18 17:12:43, Wei Yang wrote:
> In function sparse_add/remove_one_section(), pgdat_resize_lock is used
> to protect initialization/release of one mem_section. This looks not
> necessary for current implementation.
> 
> Following is the current call trace of sparse_add/remove_one_section()
> 
>     mem_hotplug_begin()
>     arch_add_memory()
>        add_pages()
>            __add_pages()
>                __add_section()
>                    sparse_add_one_section()
>     mem_hotplug_done()
> 
>     mem_hotplug_begin()
>     arch_remove_memory()
>         __remove_pages()
>             __remove_section()
>                 sparse_remove_one_section()
>     mem_hotplug_done()
> 
> which shows these functions is protected by the global mem_hotplug_lock.
> It won't face contention when accessing the mem_section.

Again there is no explanation _why_ we want this patch. The reason is
that the lock doesn't really protect what the size of the pgdat. The
comment above the lock also mentiones 
"Holding this will also guarantee that any pfn_valid() stays that way."
which is true with the current implementation and false after this patch
but I fail to see how this is helpful. I do not see any pfn walkers to
take the lock so this looks like a relict from the past.

The comment should go away in this patch.

> 
> Since the information needed in sparse_add_one_section() is node id to
> allocate proper memory. This patch also changes the prototype of
> sparse_add_one_section() to pass node id directly. This is intended to
> reduce misleading that sparse_add_one_section() would touch pgdat.

I would do that in the separate patch because review would be slightly
easier.

> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

With the comment removed
Acked-by: Michal Hocko <mhocko@suse.com>

> 
> ---
> v2:
>    * adjust changelog to show this procedure is serialized by global
>      mem_hotplug_lock
> ---
>  include/linux/memory_hotplug.h |  2 +-
>  mm/memory_hotplug.c            |  2 +-
>  mm/sparse.c                    | 17 +++++------------
>  3 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 45a5affcab8a..3787d4e913e6 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
>  		unsigned long nr_pages, struct vmem_altmap *altmap);
>  extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
>  extern bool is_memblock_offlined(struct memory_block *mem);
> -extern int sparse_add_one_section(struct pglist_data *pgdat,
> +extern int sparse_add_one_section(int nid,
>  		unsigned long start_pfn, struct vmem_altmap *altmap);
>  extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		unsigned long map_offset, struct vmem_altmap *altmap);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index f626e7e5f57b..5b3a3d7b4466 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>  	if (pfn_valid(phys_start_pfn))
>  		return -EEXIST;
>  
> -	ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap);
> +	ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 33307fc05c4d..a4fdbcb21514 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -662,25 +662,24 @@ static void free_map_bootmem(struct page *memmap)
>   * set.  If this is <=0, then that means that the passed-in
>   * map was not consumed and must be freed.
>   */
> -int __meminit sparse_add_one_section(struct pglist_data *pgdat,
> -		unsigned long start_pfn, struct vmem_altmap *altmap)
> +int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> +				     struct vmem_altmap *altmap)
>  {
>  	unsigned long section_nr = pfn_to_section_nr(start_pfn);
>  	struct mem_section *ms;
>  	struct page *memmap;
>  	unsigned long *usemap;
> -	unsigned long flags;
>  	int ret;
>  
>  	/*
>  	 * no locking for this, because it does its own
>  	 * plus, it does a kmalloc
>  	 */
> -	ret = sparse_index_init(section_nr, pgdat->node_id);
> +	ret = sparse_index_init(section_nr, nid);
>  	if (ret < 0 && ret != -EEXIST)
>  		return ret;
>  	ret = 0;
> -	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
> +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
>  	if (!memmap)
>  		return -ENOMEM;
>  	usemap = __kmalloc_section_usemap();
> @@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  		return -ENOMEM;
>  	}
>  
> -	pgdat_resize_lock(pgdat, &flags);
> -
>  	ms = __pfn_to_section(start_pfn);
>  	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
>  		ret = -EEXIST;
> @@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  	sparse_init_one_section(ms, section_nr, memmap, usemap);
>  
>  out:
> -	pgdat_resize_unlock(pgdat, &flags);
>  	if (ret < 0) {
>  		kfree(usemap);
>  		__kfree_section_memmap(memmap, altmap);
> @@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		unsigned long map_offset, struct vmem_altmap *altmap)
>  {
>  	struct page *memmap = NULL;
> -	unsigned long *usemap = NULL, flags;
> -	struct pglist_data *pgdat = zone->zone_pgdat;
> +	unsigned long *usemap = NULL;
>  
> -	pgdat_resize_lock(pgdat, &flags);
>  	if (ms->section_mem_map) {
>  		usemap = ms->pageblock_flags;
>  		memmap = sparse_decode_mem_map(ms->section_mem_map,
> @@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		ms->section_mem_map = 0;
>  		ms->pageblock_flags = NULL;
>  	}
> -	pgdat_resize_unlock(pgdat, &flags);
>  
>  	clear_hwpoisoned_pages(memmap + map_offset,
>  			PAGES_PER_SECTION - map_offset);
> -- 
> 2.15.1
> 

-- 
Michal Hocko
SUSE Labs

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

[Wei Yang]

On Thu, Nov 29, 2018 at 09:54:22AM +0100, Michal Hocko wrote:
>On Wed 28-11-18 17:12:43, Wei Yang wrote:
>> In function sparse_add/remove_one_section(), pgdat_resize_lock is used
>> to protect initialization/release of one mem_section. This looks not
>> necessary for current implementation.
>> 
>> Following is the current call trace of sparse_add/remove_one_section()
>> 
>>     mem_hotplug_begin()
>>     arch_add_memory()
>>        add_pages()
>>            __add_pages()
>>                __add_section()
>>                    sparse_add_one_section()
>>     mem_hotplug_done()
>> 
>>     mem_hotplug_begin()
>>     arch_remove_memory()
>>         __remove_pages()
>>             __remove_section()
>>                 sparse_remove_one_section()
>>     mem_hotplug_done()
>> 
>> which shows these functions is protected by the global mem_hotplug_lock.
>> It won't face contention when accessing the mem_section.
>
>Again there is no explanation _why_ we want this patch. The reason is
>that the lock doesn't really protect what the size of the pgdat. The
>comment above the lock also mentiones 
>"Holding this will also guarantee that any pfn_valid() stays that way."
>which is true with the current implementation and false after this patch
>but I fail to see how this is helpful. I do not see any pfn walkers to
>take the lock so this looks like a relict from the past.
>
>The comment should go away in this patch.
>

Ok, let me try to address this.

>> 
>> Since the information needed in sparse_add_one_section() is node id to
>> allocate proper memory. This patch also changes the prototype of
>> sparse_add_one_section() to pass node id directly. This is intended to
>> reduce misleading that sparse_add_one_section() would touch pgdat.
>
>I would do that in the separate patch because review would be slightly
>easier.

Oops, I thought the merged version is preferred.

Hmm... I would prepare v3 to separate them.

>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>
>With the comment removed
>Acked-by: Michal Hocko <mhocko@suse.com>
>
>> 
>> ---
>> v2:
>>    * adjust changelog to show this procedure is serialized by global
>>      mem_hotplug_lock
>> ---
>>  include/linux/memory_hotplug.h |  2 +-
>>  mm/memory_hotplug.c            |  2 +-
>>  mm/sparse.c                    | 17 +++++------------
>>  3 files changed, 7 insertions(+), 14 deletions(-)
>> 
>> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>> index 45a5affcab8a..3787d4e913e6 100644
>> --- a/include/linux/memory_hotplug.h
>> +++ b/include/linux/memory_hotplug.h
>> @@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
>>  		unsigned long nr_pages, struct vmem_altmap *altmap);
>>  extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
>>  extern bool is_memblock_offlined(struct memory_block *mem);
>> -extern int sparse_add_one_section(struct pglist_data *pgdat,
>> +extern int sparse_add_one_section(int nid,
>>  		unsigned long start_pfn, struct vmem_altmap *altmap);
>>  extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>>  		unsigned long map_offset, struct vmem_altmap *altmap);
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index f626e7e5f57b..5b3a3d7b4466 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>>  	if (pfn_valid(phys_start_pfn))
>>  		return -EEXIST;
>>  
>> -	ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap);
>> +	ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
>>  	if (ret < 0)
>>  		return ret;
>>  
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index 33307fc05c4d..a4fdbcb21514 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -662,25 +662,24 @@ static void free_map_bootmem(struct page *memmap)
>>   * set.  If this is <=0, then that means that the passed-in
>>   * map was not consumed and must be freed.
>>   */
>> -int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>> -		unsigned long start_pfn, struct vmem_altmap *altmap)
>> +int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>> +				     struct vmem_altmap *altmap)
>>  {
>>  	unsigned long section_nr = pfn_to_section_nr(start_pfn);
>>  	struct mem_section *ms;
>>  	struct page *memmap;
>>  	unsigned long *usemap;
>> -	unsigned long flags;
>>  	int ret;
>>  
>>  	/*
>>  	 * no locking for this, because it does its own
>>  	 * plus, it does a kmalloc
>>  	 */
>> -	ret = sparse_index_init(section_nr, pgdat->node_id);
>> +	ret = sparse_index_init(section_nr, nid);
>>  	if (ret < 0 && ret != -EEXIST)
>>  		return ret;
>>  	ret = 0;
>> -	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
>> +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
>>  	if (!memmap)
>>  		return -ENOMEM;
>>  	usemap = __kmalloc_section_usemap();
>> @@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>>  		return -ENOMEM;
>>  	}
>>  
>> -	pgdat_resize_lock(pgdat, &flags);
>> -
>>  	ms = __pfn_to_section(start_pfn);
>>  	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
>>  		ret = -EEXIST;
>> @@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>>  	sparse_init_one_section(ms, section_nr, memmap, usemap);
>>  
>>  out:
>> -	pgdat_resize_unlock(pgdat, &flags);
>>  	if (ret < 0) {
>>  		kfree(usemap);
>>  		__kfree_section_memmap(memmap, altmap);
>> @@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>>  		unsigned long map_offset, struct vmem_altmap *altmap)
>>  {
>>  	struct page *memmap = NULL;
>> -	unsigned long *usemap = NULL, flags;
>> -	struct pglist_data *pgdat = zone->zone_pgdat;
>> +	unsigned long *usemap = NULL;
>>  
>> -	pgdat_resize_lock(pgdat, &flags);
>>  	if (ms->section_mem_map) {
>>  		usemap = ms->pageblock_flags;
>>  		memmap = sparse_decode_mem_map(ms->section_mem_map,
>> @@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>>  		ms->section_mem_map = 0;
>>  		ms->pageblock_flags = NULL;
>>  	}
>> -	pgdat_resize_unlock(pgdat, &flags);
>>  
>>  	clear_hwpoisoned_pages(memmap + map_offset,
>>  			PAGES_PER_SECTION - map_offset);
>> -- 
>> 2.15.1
>> 
>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

[PATCH v3 1/2] mm, sparse: drop pgdat_resize_lock in sparse_add/remove_one_section()

[Wei Yang]

pgdat_resize_lock is used to protect pgdat's memory region information
like: node_start_pfn, node_present_pages, etc. While in function
sparse_add/remove_one_section(), pgdat_resize_lock is used to protect
initialization/release of one mem_section. This looks not proper.

Based on current implementation, even remove this lock, mem_section
is still away from contention, because it is protected by global
mem_hotpulg_lock.

Following is the current call trace of sparse_add/remove_one_section()

    mem_hotplug_begin()
    arch_add_memory()
       add_pages()
           __add_pages()
               __add_section()
                   sparse_add_one_section()
    mem_hotplug_done()

    mem_hotplug_begin()
    arch_remove_memory()
        __remove_pages()
            __remove_section()
                sparse_remove_one_section()
    mem_hotplug_done()

The comment above the pgdat_resize_lock also mentions "Holding this will
also guarantee that any pfn_valid() stays that way.", which is true with
the current implementation and false after this patch. But current
implementation doesn't meet this comment. There isn't any pfn walkers
to take the lock so this looks like a relict from the past. This patch
also removes this comment.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

---
v3:
   * adjust the changelog with the reason for this change
   * remove a comment for pgdat_resize_lock
   * separate the prototype change of sparse_add_one_section() to
     another one
v2:
   * adjust changelog to show this procedure is serialized by global
     mem_hotplug_lock
---
 include/linux/mmzone.h | 2 --
 mm/sparse.c            | 9 +--------
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 1bb749bee284..0a66085d7ced 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -638,8 +638,6 @@ typedef struct pglist_data {
 	/*
 	 * Must be held any time you expect node_start_pfn,
 	 * node_present_pages, node_spanned_pages or nr_zones stay constant.
-	 * Holding this will also guarantee that any pfn_valid() stays that
-	 * way.
 	 *
 	 * pgdat_resize_lock() and pgdat_resize_unlock() are provided to
 	 * manipulate node_size_lock without checking for CONFIG_MEMORY_HOTPLUG
diff --git a/mm/sparse.c b/mm/sparse.c
index 33307fc05c4d..5825f276485f 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -669,7 +669,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 	struct mem_section *ms;
 	struct page *memmap;
 	unsigned long *usemap;
-	unsigned long flags;
 	int ret;
 
 	/*
@@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 		return -ENOMEM;
 	}
 
-	pgdat_resize_lock(pgdat, &flags);
-
 	ms = __pfn_to_section(start_pfn);
 	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
 		ret = -EEXIST;
@@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 	sparse_init_one_section(ms, section_nr, memmap, usemap);
 
 out:
-	pgdat_resize_unlock(pgdat, &flags);
 	if (ret < 0) {
 		kfree(usemap);
 		__kfree_section_memmap(memmap, altmap);
@@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		unsigned long map_offset, struct vmem_altmap *altmap)
 {
 	struct page *memmap = NULL;
-	unsigned long *usemap = NULL, flags;
-	struct pglist_data *pgdat = zone->zone_pgdat;
+	unsigned long *usemap = NULL;
 
-	pgdat_resize_lock(pgdat, &flags);
 	if (ms->section_mem_map) {
 		usemap = ms->pageblock_flags;
 		memmap = sparse_decode_mem_map(ms->section_mem_map,
@@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		ms->section_mem_map = 0;
 		ms->pageblock_flags = NULL;
 	}
-	pgdat_resize_unlock(pgdat, &flags);
 
 	clear_hwpoisoned_pages(memmap + map_offset,
 			PAGES_PER_SECTION - map_offset);
-- 
2.15.1

[PATCH v3 2/2] mm, sparse: pass nid instead of pgdat to sparse_add_one_section()

[Wei Yang]

Since the information needed in sparse_add_one_section() is node id to
allocate proper memory, it is not necessary to pass its pgdat.

This patch changes the prototype of sparse_add_one_section() to pass
node id directly. This is intended to reduce misleading that
sparse_add_one_section() would touch pgdat.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 include/linux/memory_hotplug.h | 2 +-
 mm/memory_hotplug.c            | 2 +-
 mm/sparse.c                    | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 45a5affcab8a..3787d4e913e6 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
 		unsigned long nr_pages, struct vmem_altmap *altmap);
 extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
 extern bool is_memblock_offlined(struct memory_block *mem);
-extern int sparse_add_one_section(struct pglist_data *pgdat,
+extern int sparse_add_one_section(int nid,
 		unsigned long start_pfn, struct vmem_altmap *altmap);
 extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		unsigned long map_offset, struct vmem_altmap *altmap);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index f626e7e5f57b..5b3a3d7b4466 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
 	if (pfn_valid(phys_start_pfn))
 		return -EEXIST;
 
-	ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap);
+	ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
 	if (ret < 0)
 		return ret;
 
diff --git a/mm/sparse.c b/mm/sparse.c
index 5825f276485f..2472bf23278a 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -662,7 +662,7 @@ static void free_map_bootmem(struct page *memmap)
  * set.  If this is <=0, then that means that the passed-in
  * map was not consumed and must be freed.
  */
-int __meminit sparse_add_one_section(struct pglist_data *pgdat,
+int __meminit sparse_add_one_section(int nid,
 		unsigned long start_pfn, struct vmem_altmap *altmap)
 {
 	unsigned long section_nr = pfn_to_section_nr(start_pfn);
@@ -675,11 +675,11 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 	 * no locking for this, because it does its own
 	 * plus, it does a kmalloc
 	 */
-	ret = sparse_index_init(section_nr, pgdat->node_id);
+	ret = sparse_index_init(section_nr, nid);
 	if (ret < 0 && ret != -EEXIST)
 		return ret;
 	ret = 0;
-	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
+	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
 	if (!memmap)
 		return -ENOMEM;
 	usemap = __kmalloc_section_usemap();
-- 
2.15.1

Re: [PATCH v3 2/2] mm, sparse: pass nid instead of pgdat to sparse_add_one_section()

[David Hildenbrand]

On 29.11.18 16:53, Wei Yang wrote:
> Since the information needed in sparse_add_one_section() is node id to
> allocate proper memory, it is not necessary to pass its pgdat.
> 
> This patch changes the prototype of sparse_add_one_section() to pass
> node id directly. This is intended to reduce misleading that
> sparse_add_one_section() would touch pgdat.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  include/linux/memory_hotplug.h | 2 +-
>  mm/memory_hotplug.c            | 2 +-
>  mm/sparse.c                    | 6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 45a5affcab8a..3787d4e913e6 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
>  		unsigned long nr_pages, struct vmem_altmap *altmap);
>  extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
>  extern bool is_memblock_offlined(struct memory_block *mem);
> -extern int sparse_add_one_section(struct pglist_data *pgdat,
> +extern int sparse_add_one_section(int nid,
>  		unsigned long start_pfn, struct vmem_altmap *altmap);

While you touch that, can you fixup the alignment of the other parameters?

Apart from that

Reviewed-by: David Hildenbrand <david@redhat.com>

>  extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		unsigned long map_offset, struct vmem_altmap *altmap);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index f626e7e5f57b..5b3a3d7b4466 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>  	if (pfn_valid(phys_start_pfn))
>  		return -EEXIST;
>  
> -	ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap);
> +	ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 5825f276485f..2472bf23278a 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -662,7 +662,7 @@ static void free_map_bootmem(struct page *memmap)
>   * set.  If this is <=0, then that means that the passed-in
>   * map was not consumed and must be freed.
>   */
> -int __meminit sparse_add_one_section(struct pglist_data *pgdat,
> +int __meminit sparse_add_one_section(int nid,
>  		unsigned long start_pfn, struct vmem_altmap *altmap)
>  {
>  	unsigned long section_nr = pfn_to_section_nr(start_pfn);
> @@ -675,11 +675,11 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  	 * no locking for this, because it does its own
>  	 * plus, it does a kmalloc
>  	 */
> -	ret = sparse_index_init(section_nr, pgdat->node_id);
> +	ret = sparse_index_init(section_nr, nid);
>  	if (ret < 0 && ret != -EEXIST)
>  		return ret;
>  	ret = 0;
> -	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
> +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
>  	if (!memmap)
>  		return -ENOMEM;
>  	usemap = __kmalloc_section_usemap();
> 


-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 29.11.18 16:53, Wei Yang wrote:
> pgdat_resize_lock is used to protect pgdat's memory region information
> like: node_start_pfn, node_present_pages, etc. While in function
> sparse_add/remove_one_section(), pgdat_resize_lock is used to protect
> initialization/release of one mem_section. This looks not proper.
> 
> Based on current implementation, even remove this lock, mem_section
> is still away from contention, because it is protected by global
> mem_hotpulg_lock.

s/mem_hotpulg_lock/mem_hotplug_lock/

> 
> Following is the current call trace of sparse_add/remove_one_section()
> 
>     mem_hotplug_begin()
>     arch_add_memory()
>        add_pages()
>            __add_pages()
>                __add_section()
>                    sparse_add_one_section()
>     mem_hotplug_done()
> 
>     mem_hotplug_begin()
>     arch_remove_memory()
>         __remove_pages()
>             __remove_section()
>                 sparse_remove_one_section()
>     mem_hotplug_done()
> 
> The comment above the pgdat_resize_lock also mentions "Holding this will
> also guarantee that any pfn_valid() stays that way.", which is true with
> the current implementation and false after this patch. But current
> implementation doesn't meet this comment. There isn't any pfn walkers
> to take the lock so this looks like a relict from the past. This patch
> also removes this comment.

Should we start to document which lock is expected to protect what?

I suggest adding what you just found out to
Documentation/admin-guide/mm/memory-hotplug.rst "Locking Internals".
Maybe a new subsection for mem_hotplug_lock. And eventually also
pgdat_resize_lock.

> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> 
> ---
> v3:
>    * adjust the changelog with the reason for this change
>    * remove a comment for pgdat_resize_lock
>    * separate the prototype change of sparse_add_one_section() to
>      another one
> v2:
>    * adjust changelog to show this procedure is serialized by global
>      mem_hotplug_lock
> ---
>  include/linux/mmzone.h | 2 --
>  mm/sparse.c            | 9 +--------
>  2 files changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 1bb749bee284..0a66085d7ced 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -638,8 +638,6 @@ typedef struct pglist_data {
>  	/*
>  	 * Must be held any time you expect node_start_pfn,
>  	 * node_present_pages, node_spanned_pages or nr_zones stay constant.
> -	 * Holding this will also guarantee that any pfn_valid() stays that
> -	 * way.
>  	 *
>  	 * pgdat_resize_lock() and pgdat_resize_unlock() are provided to
>  	 * manipulate node_size_lock without checking for CONFIG_MEMORY_HOTPLUG
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 33307fc05c4d..5825f276485f 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -669,7 +669,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  	struct mem_section *ms;
>  	struct page *memmap;
>  	unsigned long *usemap;
> -	unsigned long flags;
>  	int ret;
>  
>  	/*
> @@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  		return -ENOMEM;
>  	}
>  
> -	pgdat_resize_lock(pgdat, &flags);
> -
>  	ms = __pfn_to_section(start_pfn);
>  	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
>  		ret = -EEXIST;
> @@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  	sparse_init_one_section(ms, section_nr, memmap, usemap);
>  
>  out:
> -	pgdat_resize_unlock(pgdat, &flags);
>  	if (ret < 0) {
>  		kfree(usemap);
>  		__kfree_section_memmap(memmap, altmap);
> @@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		unsigned long map_offset, struct vmem_altmap *altmap)
>  {
>  	struct page *memmap = NULL;
> -	unsigned long *usemap = NULL, flags;
> -	struct pglist_data *pgdat = zone->zone_pgdat;
> +	unsigned long *usemap = NULL;
>  
> -	pgdat_resize_lock(pgdat, &flags);
>  	if (ms->section_mem_map) {
>  		usemap = ms->pageblock_flags;
>  		memmap = sparse_decode_mem_map(ms->section_mem_map,
> @@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		ms->section_mem_map = 0;
>  		ms->pageblock_flags = NULL;
>  	}
> -	pgdat_resize_unlock(pgdat, &flags);
>  
>  	clear_hwpoisoned_pages(memmap + map_offset,
>  			PAGES_PER_SECTION - map_offset);
> 


-- 

Thanks,

David / dhildenb

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

[Michal Hocko]

On Thu 29-11-18 23:53:15, Wei Yang wrote:
> pgdat_resize_lock is used to protect pgdat's memory region information
> like: node_start_pfn, node_present_pages, etc. While in function
> sparse_add/remove_one_section(), pgdat_resize_lock is used to protect
> initialization/release of one mem_section. This looks not proper.
> 
> Based on current implementation, even remove this lock, mem_section
> is still away from contention, because it is protected by global
> mem_hotpulg_lock.

I guess you wanted to say.
These code paths are currently protected by mem_hotpulg_lock currently
but should there ever be any reason for locking at the sparse layer a
dedicated lock should be introduced.

> 
> Following is the current call trace of sparse_add/remove_one_section()
> 
>     mem_hotplug_begin()
>     arch_add_memory()
>        add_pages()
>            __add_pages()
>                __add_section()
>                    sparse_add_one_section()
>     mem_hotplug_done()
> 
>     mem_hotplug_begin()
>     arch_remove_memory()
>         __remove_pages()
>             __remove_section()
>                 sparse_remove_one_section()
>     mem_hotplug_done()
> 
> The comment above the pgdat_resize_lock also mentions "Holding this will
> also guarantee that any pfn_valid() stays that way.", which is true with
> the current implementation and false after this patch. But current
> implementation doesn't meet this comment. There isn't any pfn walkers
> to take the lock so this looks like a relict from the past. This patch
> also removes this comment.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

Other than that
Acked-by: Michal Hocko <mhocko@suse.com>

> 
> ---
> v3:
>    * adjust the changelog with the reason for this change
>    * remove a comment for pgdat_resize_lock
>    * separate the prototype change of sparse_add_one_section() to
>      another one
> v2:
>    * adjust changelog to show this procedure is serialized by global
>      mem_hotplug_lock
> ---
>  include/linux/mmzone.h | 2 --
>  mm/sparse.c            | 9 +--------
>  2 files changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 1bb749bee284..0a66085d7ced 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -638,8 +638,6 @@ typedef struct pglist_data {
>  	/*
>  	 * Must be held any time you expect node_start_pfn,
>  	 * node_present_pages, node_spanned_pages or nr_zones stay constant.
> -	 * Holding this will also guarantee that any pfn_valid() stays that
> -	 * way.
>  	 *
>  	 * pgdat_resize_lock() and pgdat_resize_unlock() are provided to
>  	 * manipulate node_size_lock without checking for CONFIG_MEMORY_HOTPLUG
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 33307fc05c4d..5825f276485f 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -669,7 +669,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  	struct mem_section *ms;
>  	struct page *memmap;
>  	unsigned long *usemap;
> -	unsigned long flags;
>  	int ret;
>  
>  	/*
> @@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  		return -ENOMEM;
>  	}
>  
> -	pgdat_resize_lock(pgdat, &flags);
> -
>  	ms = __pfn_to_section(start_pfn);
>  	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
>  		ret = -EEXIST;
> @@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  	sparse_init_one_section(ms, section_nr, memmap, usemap);
>  
>  out:
> -	pgdat_resize_unlock(pgdat, &flags);
>  	if (ret < 0) {
>  		kfree(usemap);
>  		__kfree_section_memmap(memmap, altmap);
> @@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		unsigned long map_offset, struct vmem_altmap *altmap)
>  {
>  	struct page *memmap = NULL;
> -	unsigned long *usemap = NULL, flags;
> -	struct pglist_data *pgdat = zone->zone_pgdat;
> +	unsigned long *usemap = NULL;
>  
> -	pgdat_resize_lock(pgdat, &flags);
>  	if (ms->section_mem_map) {
>  		usemap = ms->pageblock_flags;
>  		memmap = sparse_decode_mem_map(ms->section_mem_map,
> @@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		ms->section_mem_map = 0;
>  		ms->pageblock_flags = NULL;
>  	}
> -	pgdat_resize_unlock(pgdat, &flags);
>  
>  	clear_hwpoisoned_pages(memmap + map_offset,
>  			PAGES_PER_SECTION - map_offset);
> -- 
> 2.15.1
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v3 2/2] mm, sparse: pass nid instead of pgdat to sparse_add_one_section()

[Michal Hocko]

On Thu 29-11-18 23:53:16, Wei Yang wrote:
> Since the information needed in sparse_add_one_section() is node id to
> allocate proper memory, it is not necessary to pass its pgdat.
> 
> This patch changes the prototype of sparse_add_one_section() to pass
> node id directly. This is intended to reduce misleading that
> sparse_add_one_section() would touch pgdat.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

>From a quick look, this looks ok.

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks for splitting up it from the original patch.

> ---
>  include/linux/memory_hotplug.h | 2 +-
>  mm/memory_hotplug.c            | 2 +-
>  mm/sparse.c                    | 6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 45a5affcab8a..3787d4e913e6 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
>  		unsigned long nr_pages, struct vmem_altmap *altmap);
>  extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
>  extern bool is_memblock_offlined(struct memory_block *mem);
> -extern int sparse_add_one_section(struct pglist_data *pgdat,
> +extern int sparse_add_one_section(int nid,
>  		unsigned long start_pfn, struct vmem_altmap *altmap);
>  extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		unsigned long map_offset, struct vmem_altmap *altmap);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index f626e7e5f57b..5b3a3d7b4466 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>  	if (pfn_valid(phys_start_pfn))
>  		return -EEXIST;
>  
> -	ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap);
> +	ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 5825f276485f..2472bf23278a 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -662,7 +662,7 @@ static void free_map_bootmem(struct page *memmap)
>   * set.  If this is <=0, then that means that the passed-in
>   * map was not consumed and must be freed.
>   */
> -int __meminit sparse_add_one_section(struct pglist_data *pgdat,
> +int __meminit sparse_add_one_section(int nid,
>  		unsigned long start_pfn, struct vmem_altmap *altmap)
>  {
>  	unsigned long section_nr = pfn_to_section_nr(start_pfn);
> @@ -675,11 +675,11 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  	 * no locking for this, because it does its own
>  	 * plus, it does a kmalloc
>  	 */
> -	ret = sparse_index_init(section_nr, pgdat->node_id);
> +	ret = sparse_index_init(section_nr, nid);
>  	if (ret < 0 && ret != -EEXIST)
>  		return ret;
>  	ret = 0;
> -	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
> +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
>  	if (!memmap)
>  		return -ENOMEM;
>  	usemap = __kmalloc_section_usemap();
> -- 
> 2.15.1
> 

-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Thu 29-11-18 17:06:15, David Hildenbrand wrote:
> I suggest adding what you just found out to
[...]
> Documentation/admin-guide/mm/memory-hotplug.rst "Locking Internals".
> Maybe a new subsection for mem_hotplug_lock. And eventually also
> pgdat_resize_lock.

That would be really great! I guess I have suggested something like that
to Oscar already and he provided a highlevel overview.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v3 2/2] mm, sparse: pass nid instead of pgdat to sparse_add_one_section()

[Wei Yang]

On Thu, Nov 29, 2018 at 06:15:42PM +0100, Michal Hocko wrote:
>On Thu 29-11-18 23:53:16, Wei Yang wrote:
>> Since the information needed in sparse_add_one_section() is node id to
>> allocate proper memory, it is not necessary to pass its pgdat.
>> 
>> This patch changes the prototype of sparse_add_one_section() to pass
>> node id directly. This is intended to reduce misleading that
>> sparse_add_one_section() would touch pgdat.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>
>>>From a quick look, this looks ok.
>
>Acked-by: Michal Hocko <mhocko@suse.com>
>
>Thanks for splitting up it from the original patch.
>

Thanks for your suggestion.

>> ---
>>  include/linux/memory_hotplug.h | 2 +-
>>  mm/memory_hotplug.c            | 2 +-
>>  mm/sparse.c                    | 6 +++---
>>  3 files changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>> index 45a5affcab8a..3787d4e913e6 100644
>> --- a/include/linux/memory_hotplug.h
>> +++ b/include/linux/memory_hotplug.h
>> @@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
>>  		unsigned long nr_pages, struct vmem_altmap *altmap);
>>  extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
>>  extern bool is_memblock_offlined(struct memory_block *mem);
>> -extern int sparse_add_one_section(struct pglist_data *pgdat,
>> +extern int sparse_add_one_section(int nid,
>>  		unsigned long start_pfn, struct vmem_altmap *altmap);
>>  extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>>  		unsigned long map_offset, struct vmem_altmap *altmap);
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index f626e7e5f57b..5b3a3d7b4466 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
>>  	if (pfn_valid(phys_start_pfn))
>>  		return -EEXIST;
>>  
>> -	ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap);
>> +	ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
>>  	if (ret < 0)
>>  		return ret;
>>  
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index 5825f276485f..2472bf23278a 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -662,7 +662,7 @@ static void free_map_bootmem(struct page *memmap)
>>   * set.  If this is <=0, then that means that the passed-in
>>   * map was not consumed and must be freed.
>>   */
>> -int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>> +int __meminit sparse_add_one_section(int nid,
>>  		unsigned long start_pfn, struct vmem_altmap *altmap)
>>  {
>>  	unsigned long section_nr = pfn_to_section_nr(start_pfn);
>> @@ -675,11 +675,11 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>>  	 * no locking for this, because it does its own
>>  	 * plus, it does a kmalloc
>>  	 */
>> -	ret = sparse_index_init(section_nr, pgdat->node_id);
>> +	ret = sparse_index_init(section_nr, nid);
>>  	if (ret < 0 && ret != -EEXIST)
>>  		return ret;
>>  	ret = 0;
>> -	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
>> +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
>>  	if (!memmap)
>>  		return -ENOMEM;
>>  	usemap = __kmalloc_section_usemap();
>> -- 
>> 2.15.1
>> 
>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Re: [PATCH v3 2/2] mm, sparse: pass nid instead of pgdat to sparse_add_one_section()

[Wei Yang]

On Thu, Nov 29, 2018 at 05:01:51PM +0100, David Hildenbrand wrote:
>On 29.11.18 16:53, Wei Yang wrote:
>> Since the information needed in sparse_add_one_section() is node id to
>> allocate proper memory, it is not necessary to pass its pgdat.
>> 
>> This patch changes the prototype of sparse_add_one_section() to pass
>> node id directly. This is intended to reduce misleading that
>> sparse_add_one_section() would touch pgdat.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>> ---
>>  include/linux/memory_hotplug.h | 2 +-
>>  mm/memory_hotplug.c            | 2 +-
>>  mm/sparse.c                    | 6 +++---
>>  3 files changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>> index 45a5affcab8a..3787d4e913e6 100644
>> --- a/include/linux/memory_hotplug.h
>> +++ b/include/linux/memory_hotplug.h
>> @@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
>>  		unsigned long nr_pages, struct vmem_altmap *altmap);
>>  extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
>>  extern bool is_memblock_offlined(struct memory_block *mem);
>> -extern int sparse_add_one_section(struct pglist_data *pgdat,
>> +extern int sparse_add_one_section(int nid,
>>  		unsigned long start_pfn, struct vmem_altmap *altmap);
>
>While you touch that, can you fixup the alignment of the other parameters?
>

If I am correct, the code style of alignment is like this?

extern int sparse_add_one_section(int nid, unsigned long start_pfn,
				  struct vmem_altmap *altmap);

>Apart from that
>
>Reviewed-by: David Hildenbrand <david@redhat.com>
>

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Thu, Nov 29, 2018 at 05:06:15PM +0100, David Hildenbrand wrote:
>On 29.11.18 16:53, Wei Yang wrote:
>> pgdat_resize_lock is used to protect pgdat's memory region information
>> like: node_start_pfn, node_present_pages, etc. While in function
>> sparse_add/remove_one_section(), pgdat_resize_lock is used to protect
>> initialization/release of one mem_section. This looks not proper.
>> 
>> Based on current implementation, even remove this lock, mem_section
>> is still away from contention, because it is protected by global
>> mem_hotpulg_lock.
>
>s/mem_hotpulg_lock/mem_hotplug_lock/
>
>> 
>> Following is the current call trace of sparse_add/remove_one_section()
>> 
>>     mem_hotplug_begin()
>>     arch_add_memory()
>>        add_pages()
>>            __add_pages()
>>                __add_section()
>>                    sparse_add_one_section()
>>     mem_hotplug_done()
>> 
>>     mem_hotplug_begin()
>>     arch_remove_memory()
>>         __remove_pages()
>>             __remove_section()
>>                 sparse_remove_one_section()
>>     mem_hotplug_done()
>> 
>> The comment above the pgdat_resize_lock also mentions "Holding this will
>> also guarantee that any pfn_valid() stays that way.", which is true with
>> the current implementation and false after this patch. But current
>> implementation doesn't meet this comment. There isn't any pfn walkers
>> to take the lock so this looks like a relict from the past. This patch
>> also removes this comment.
>
>Should we start to document which lock is expected to protect what?
>
>I suggest adding what you just found out to
>Documentation/admin-guide/mm/memory-hotplug.rst "Locking Internals".
>Maybe a new subsection for mem_hotplug_lock. And eventually also
>pgdat_resize_lock.

Well, I am not good at document writting. Below is my first trial.  Look
forward your comments.

BTW, in case I would send a new version with this, would I put this into
a separate one or merge this into current one?

diff --git a/Documentation/admin-guide/mm/memory-hotplug.rst b/Documentation/admin-guide/mm/memory-hotplug.rst
index 5c4432c96c4b..1548820a0762 100644
--- a/Documentation/admin-guide/mm/memory-hotplug.rst
+++ b/Documentation/admin-guide/mm/memory-hotplug.rst
@@ -396,6 +396,20 @@ Need more implementation yet....
 Locking Internals
 =================
 
+There are three locks involved in memory-hotplug, two global lock and one local
+lock:
+
+- device_hotplug_lock
+- mem_hotplug_lock
+- device_lock
+
+Currently, they are twisted together for all kinds of reasons. The following
+part is divded into device_hotplug_lock and mem_hotplug_lock parts
+respectively to describe those tricky situations.
+
+device_hotplug_lock
+---------------------
+
 When adding/removing memory that uses memory block devices (i.e. ordinary RAM),
 the device_hotplug_lock should be held to:
 
@@ -417,14 +431,21 @@ memory faster than expected:
 As the device is visible to user space before taking the device_lock(), this
 can result in a lock inversion.
 
+mem_hotplug_lock
+---------------------
+
 onlining/offlining of memory should be done via device_online()/
-device_offline() - to make sure it is properly synchronized to actions
-via sysfs. Holding device_hotplug_lock is advised (to e.g. protect online_type)
+device_offline() - to make sure it is properly synchronized to actions via
+sysfs. Even mem_hotplug_lock is used to protect the process, because of the
+lock inversion described above, holding device_hotplug_lock is still advised
+(to e.g. protect online_type)
 
 When adding/removing/onlining/offlining memory or adding/removing
 heterogeneous/device memory, we should always hold the mem_hotplug_lock in
 write mode to serialise memory hotplug (e.g. access to global/zone
-variables).
+variables). Currently, we take advantage of this to serialise sparsemem's
+mem_section handling in sparse_add_one_section() and
+sparse_remove_one_section().
 
 In addition, mem_hotplug_lock (in contrast to device_hotplug_lock) in read
 mode allows for a quite efficient get_online_mems/put_online_mems

>
>
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

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

[David Hildenbrand]

>> I suggest adding what you just found out to
>> Documentation/admin-guide/mm/memory-hotplug.rst "Locking Internals".
>> Maybe a new subsection for mem_hotplug_lock. And eventually also
>> pgdat_resize_lock.
> 
> Well, I am not good at document writting. Below is my first trial.  Look
> forward your comments.

I'll have a look, maybe also Oscar and Michal can have a look. I guess
we don't have to cover all now, we can add more details as we discover them.

> 
> BTW, in case I would send a new version with this, would I put this into
> a separate one or merge this into current one?

I would put this into a separate patch.

> 
> diff --git a/Documentation/admin-guide/mm/memory-hotplug.rst b/Documentation/admin-guide/mm/memory-hotplug.rst
> index 5c4432c96c4b..1548820a0762 100644
> --- a/Documentation/admin-guide/mm/memory-hotplug.rst
> +++ b/Documentation/admin-guide/mm/memory-hotplug.rst
> @@ -396,6 +396,20 @@ Need more implementation yet....
>  Locking Internals
>  =================
>  
> +There are three locks involved in memory-hotplug, two global lock and one local
> +lock:
> +
> +- device_hotplug_lock
> +- mem_hotplug_lock
> +- device_lock
> +
> +Currently, they are twisted together for all kinds of reasons. The following
> +part is divded into device_hotplug_lock and mem_hotplug_lock parts

s/divded/divided/

> +respectively to describe those tricky situations.
> +
> +device_hotplug_lock
> +---------------------
> +
>  When adding/removing memory that uses memory block devices (i.e. ordinary RAM),
>  the device_hotplug_lock should be held to:
>  
> @@ -417,14 +431,21 @@ memory faster than expected:
>  As the device is visible to user space before taking the device_lock(), this
>  can result in a lock inversion.
>  
> +mem_hotplug_lock
> +---------------------
> +

I would this section start after the following paragraph, as most of
that paragraph belongs to the device_hotplug_lock.


>  onlining/offlining of memory should be done via device_online()/
> -device_offline() - to make sure it is properly synchronized to actions
> -via sysfs. Holding device_hotplug_lock is advised (to e.g. protect online_type)
> +device_offline() - to make sure it is properly synchronized to actions via
> +sysfs. Even mem_hotplug_lock is used to protect the process, because of the
> +lock inversion described above, holding device_hotplug_lock is still advised
> +(to e.g. protect online_type)
>  
>  When adding/removing/onlining/offlining memory or adding/removing
>  heterogeneous/device memory, we should always hold the mem_hotplug_lock in
>  write mode to serialise memory hotplug (e.g. access to global/zone
> -variables).
> +variables). Currently, we take advantage of this to serialise sparsemem's
> +mem_section handling in sparse_add_one_section() and
> +sparse_remove_one_section().
>  
>  In addition, mem_hotplug_lock (in contrast to device_hotplug_lock) in read
>  mode allows for a quite efficient get_online_mems/put_online_mems
> 
>>
>>
>> Thanks,
>>
>> David / dhildenb
> 

Apart from that looks good to me, thanks!


-- 

Thanks,

David / dhildenb

Re: [PATCH v3 2/2] mm, sparse: pass nid instead of pgdat to sparse_add_one_section()

[David Hildenbrand]

On 30.11.18 02:22, Wei Yang wrote:
> On Thu, Nov 29, 2018 at 05:01:51PM +0100, David Hildenbrand wrote:
>> On 29.11.18 16:53, Wei Yang wrote:
>>> Since the information needed in sparse_add_one_section() is node id to
>>> allocate proper memory, it is not necessary to pass its pgdat.
>>>
>>> This patch changes the prototype of sparse_add_one_section() to pass
>>> node id directly. This is intended to reduce misleading that
>>> sparse_add_one_section() would touch pgdat.
>>>
>>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>>> ---
>>>  include/linux/memory_hotplug.h | 2 +-
>>>  mm/memory_hotplug.c            | 2 +-
>>>  mm/sparse.c                    | 6 +++---
>>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>>> index 45a5affcab8a..3787d4e913e6 100644
>>> --- a/include/linux/memory_hotplug.h
>>> +++ b/include/linux/memory_hotplug.h
>>> @@ -333,7 +333,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
>>>  		unsigned long nr_pages, struct vmem_altmap *altmap);
>>>  extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
>>>  extern bool is_memblock_offlined(struct memory_block *mem);
>>> -extern int sparse_add_one_section(struct pglist_data *pgdat,
>>> +extern int sparse_add_one_section(int nid,
>>>  		unsigned long start_pfn, struct vmem_altmap *altmap);
>>
>> While you touch that, can you fixup the alignment of the other parameters?
>>
> 
> If I am correct, the code style of alignment is like this?
> 
> extern int sparse_add_one_section(int nid, unsigned long start_pfn,
> 				  struct vmem_altmap *altmap);

Yes, all parameters should start at the same indentation. (some people
don't care and produce this "mess", I tend to care :) )

> 
>> Apart from that
>>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>
> 


-- 

Thanks,

David / dhildenb

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

[Michal Hocko]

On Fri 30-11-18 10:19:13, David Hildenbrand wrote:
> >> I suggest adding what you just found out to
> >> Documentation/admin-guide/mm/memory-hotplug.rst "Locking Internals".
> >> Maybe a new subsection for mem_hotplug_lock. And eventually also
> >> pgdat_resize_lock.
> > 
> > Well, I am not good at document writting. Below is my first trial.  Look
> > forward your comments.
> 
> I'll have a look, maybe also Oscar and Michal can have a look. I guess
> we don't have to cover all now, we can add more details as we discover them.

Oscar, didn't you have something already?
-- 
Michal Hocko
SUSE Labs

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

[Wei Yang]

On Fri, Nov 30, 2018 at 10:19:13AM +0100, David Hildenbrand wrote:
>>> I suggest adding what you just found out to
>>> Documentation/admin-guide/mm/memory-hotplug.rst "Locking Internals".
>>> Maybe a new subsection for mem_hotplug_lock. And eventually also
>>> pgdat_resize_lock.
>> 
>> Well, I am not good at document writting. Below is my first trial.  Look
>> forward your comments.
>
>I'll have a look, maybe also Oscar and Michal can have a look. I guess
>we don't have to cover all now, we can add more details as we discover them.
>
>> 
>> BTW, in case I would send a new version with this, would I put this into
>> a separate one or merge this into current one?
>
>I would put this into a separate patch.
>
>> 
>> diff --git a/Documentation/admin-guide/mm/memory-hotplug.rst b/Documentation/admin-guide/mm/memory-hotplug.rst
>> index 5c4432c96c4b..1548820a0762 100644
>> --- a/Documentation/admin-guide/mm/memory-hotplug.rst
>> +++ b/Documentation/admin-guide/mm/memory-hotplug.rst
>> @@ -396,6 +396,20 @@ Need more implementation yet....
>>  Locking Internals
>>  =================
>>  
>> +There are three locks involved in memory-hotplug, two global lock and one local
>> +lock:
>> +
>> +- device_hotplug_lock
>> +- mem_hotplug_lock
>> +- device_lock
>> +
>> +Currently, they are twisted together for all kinds of reasons. The following
>> +part is divded into device_hotplug_lock and mem_hotplug_lock parts
>
>s/divded/divided/
>
>> +respectively to describe those tricky situations.
>> +
>> +device_hotplug_lock
>> +---------------------
>> +
>>  When adding/removing memory that uses memory block devices (i.e. ordinary RAM),
>>  the device_hotplug_lock should be held to:
>>  
>> @@ -417,14 +431,21 @@ memory faster than expected:
>>  As the device is visible to user space before taking the device_lock(), this
>>  can result in a lock inversion.
>>  
>> +mem_hotplug_lock
>> +---------------------
>> +
>
>I would this section start after the following paragraph, as most of
>that paragraph belongs to the device_hotplug_lock.
>
>
>>  onlining/offlining of memory should be done via device_online()/
>> -device_offline() - to make sure it is properly synchronized to actions
>> -via sysfs. Holding device_hotplug_lock is advised (to e.g. protect online_type)
>> +device_offline() - to make sure it is properly synchronized to actions via
>> +sysfs. Even mem_hotplug_lock is used to protect the process, because of the
>> +lock inversion described above, holding device_hotplug_lock is still advised
>> +(to e.g. protect online_type)
>>  
>>  When adding/removing/onlining/offlining memory or adding/removing
>>  heterogeneous/device memory, we should always hold the mem_hotplug_lock in
>>  write mode to serialise memory hotplug (e.g. access to global/zone
>> -variables).
>> +variables). Currently, we take advantage of this to serialise sparsemem's
>> +mem_section handling in sparse_add_one_section() and
>> +sparse_remove_one_section().
>>  
>>  In addition, mem_hotplug_lock (in contrast to device_hotplug_lock) in read
>>  mode allows for a quite efficient get_online_mems/put_online_mems
>> 
>>>
>>>
>>> Thanks,
>>>
>>> David / dhildenb
>> 
>
>Apart from that looks good to me, thanks!
>

Thanks :-)

>
>-- 
>
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

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

[David Hildenbrand]

On 30.11.18 05:28, Wei Yang wrote:
> On Thu, Nov 29, 2018 at 05:06:15PM +0100, David Hildenbrand wrote:
>> On 29.11.18 16:53, Wei Yang wrote:
>>> pgdat_resize_lock is used to protect pgdat's memory region information
>>> like: node_start_pfn, node_present_pages, etc. While in function
>>> sparse_add/remove_one_section(), pgdat_resize_lock is used to protect
>>> initialization/release of one mem_section. This looks not proper.
>>>
>>> Based on current implementation, even remove this lock, mem_section
>>> is still away from contention, because it is protected by global
>>> mem_hotpulg_lock.
>>
>> s/mem_hotpulg_lock/mem_hotplug_lock/
>>
>>>
>>> Following is the current call trace of sparse_add/remove_one_section()
>>>
>>>     mem_hotplug_begin()
>>>     arch_add_memory()
>>>        add_pages()
>>>            __add_pages()
>>>                __add_section()
>>>                    sparse_add_one_section()
>>>     mem_hotplug_done()
>>>
>>>     mem_hotplug_begin()
>>>     arch_remove_memory()
>>>         __remove_pages()
>>>             __remove_section()
>>>                 sparse_remove_one_section()
>>>     mem_hotplug_done()
>>>
>>> The comment above the pgdat_resize_lock also mentions "Holding this will
>>> also guarantee that any pfn_valid() stays that way.", which is true with
>>> the current implementation and false after this patch. But current
>>> implementation doesn't meet this comment. There isn't any pfn walkers
>>> to take the lock so this looks like a relict from the past. This patch
>>> also removes this comment.
>>
>> Should we start to document which lock is expected to protect what?
>>
>> I suggest adding what you just found out to
>> Documentation/admin-guide/mm/memory-hotplug.rst "Locking Internals".
>> Maybe a new subsection for mem_hotplug_lock. And eventually also
>> pgdat_resize_lock.
> 
> Well, I am not good at document writting. Below is my first trial.  Look
> forward your comments.
> 
> BTW, in case I would send a new version with this, would I put this into
> a separate one or merge this into current one?
> 
> diff --git a/Documentation/admin-guide/mm/memory-hotplug.rst b/Documentation/admin-guide/mm/memory-hotplug.rst
> index 5c4432c96c4b..1548820a0762 100644
> --- a/Documentation/admin-guide/mm/memory-hotplug.rst
> +++ b/Documentation/admin-guide/mm/memory-hotplug.rst

BTW, it really should go into

Documentation/core-api/memory-hotplug.rst

Something got wrong while merging this in linux-next, so now we have
duplicate documentation and the one in
Documentation/admin-guide/mm/memory-hotplug.rst about locking internals
has to go.


> @@ -396,6 +396,20 @@ Need more implementation yet....
>  Locking Internals
>  =================
>  
> +There are three locks involved in memory-hotplug, two global lock and one local
> +lock:
> +
> +- device_hotplug_lock
> +- mem_hotplug_lock
> +- device_lock
> +
> +Currently, they are twisted together for all kinds of reasons. The following
> +part is divded into device_hotplug_lock and mem_hotplug_lock parts
> +respectively to describe those tricky situations.
> +
> +device_hotplug_lock
> +---------------------
> +
>  When adding/removing memory that uses memory block devices (i.e. ordinary RAM),
>  the device_hotplug_lock should be held to:
>  
> @@ -417,14 +431,21 @@ memory faster than expected:
>  As the device is visible to user space before taking the device_lock(), this
>  can result in a lock inversion.
>  
> +mem_hotplug_lock
> +---------------------
> +
>  onlining/offlining of memory should be done via device_online()/
> -device_offline() - to make sure it is properly synchronized to actions
> -via sysfs. Holding device_hotplug_lock is advised (to e.g. protect online_type)
> +device_offline() - to make sure it is properly synchronized to actions via
> +sysfs. Even mem_hotplug_lock is used to protect the process, because of the
> +lock inversion described above, holding device_hotplug_lock is still advised
> +(to e.g. protect online_type)
>  
>  When adding/removing/onlining/offlining memory or adding/removing
>  heterogeneous/device memory, we should always hold the mem_hotplug_lock in
>  write mode to serialise memory hotplug (e.g. access to global/zone
> -variables).
> +variables). Currently, we take advantage of this to serialise sparsemem's
> +mem_section handling in sparse_add_one_section() and
> +sparse_remove_one_section().
>  
>  In addition, mem_hotplug_lock (in contrast to device_hotplug_lock) in read
>  mode allows for a quite efficient get_online_mems/put_online_mems
> 
>>
>>
>> Thanks,
>>
>> David / dhildenb
> 


-- 

Thanks,

David / dhildenb

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

[Wei Yang]

On Mon, Dec 03, 2018 at 12:25:20PM +0100, David Hildenbrand wrote:
>On 30.11.18 05:28, Wei Yang wrote:
>> On Thu, Nov 29, 2018 at 05:06:15PM +0100, David Hildenbrand wrote:
>>> On 29.11.18 16:53, Wei Yang wrote:
>>>> pgdat_resize_lock is used to protect pgdat's memory region information
>>>> like: node_start_pfn, node_present_pages, etc. While in function
>>>> sparse_add/remove_one_section(), pgdat_resize_lock is used to protect
>>>> initialization/release of one mem_section. This looks not proper.
>>>>
>>>> Based on current implementation, even remove this lock, mem_section
>>>> is still away from contention, because it is protected by global
>>>> mem_hotpulg_lock.
>>>
>>> s/mem_hotpulg_lock/mem_hotplug_lock/
>>>
>>>>
>>>> Following is the current call trace of sparse_add/remove_one_section()
>>>>
>>>>     mem_hotplug_begin()
>>>>     arch_add_memory()
>>>>        add_pages()
>>>>            __add_pages()
>>>>                __add_section()
>>>>                    sparse_add_one_section()
>>>>     mem_hotplug_done()
>>>>
>>>>     mem_hotplug_begin()
>>>>     arch_remove_memory()
>>>>         __remove_pages()
>>>>             __remove_section()
>>>>                 sparse_remove_one_section()
>>>>     mem_hotplug_done()
>>>>
>>>> The comment above the pgdat_resize_lock also mentions "Holding this will
>>>> also guarantee that any pfn_valid() stays that way.", which is true with
>>>> the current implementation and false after this patch. But current
>>>> implementation doesn't meet this comment. There isn't any pfn walkers
>>>> to take the lock so this looks like a relict from the past. This patch
>>>> also removes this comment.
>>>
>>> Should we start to document which lock is expected to protect what?
>>>
>>> I suggest adding what you just found out to
>>> Documentation/admin-guide/mm/memory-hotplug.rst "Locking Internals".
>>> Maybe a new subsection for mem_hotplug_lock. And eventually also
>>> pgdat_resize_lock.
>> 
>> Well, I am not good at document writting. Below is my first trial.  Look
>> forward your comments.
>> 
>> BTW, in case I would send a new version with this, would I put this into
>> a separate one or merge this into current one?
>> 
>> diff --git a/Documentation/admin-guide/mm/memory-hotplug.rst b/Documentation/admin-guide/mm/memory-hotplug.rst
>> index 5c4432c96c4b..1548820a0762 100644
>> --- a/Documentation/admin-guide/mm/memory-hotplug.rst
>> +++ b/Documentation/admin-guide/mm/memory-hotplug.rst
>
>BTW, it really should go into
>
>Documentation/core-api/memory-hotplug.rst
>
>Something got wrong while merging this in linux-next, so now we have
>duplicate documentation and the one in
>Documentation/admin-guide/mm/memory-hotplug.rst about locking internals
>has to go.
>

Sounds reasonable.

Admin may not necessary need to understand the internal locking.

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Fri, Nov 30, 2018 at 10:52:30AM +0100, Michal Hocko wrote:
>On Fri 30-11-18 10:19:13, David Hildenbrand wrote:
>> >> I suggest adding what you just found out to
>> >> Documentation/admin-guide/mm/memory-hotplug.rst "Locking Internals".
>> >> Maybe a new subsection for mem_hotplug_lock. And eventually also
>> >> pgdat_resize_lock.
>> > 
>> > Well, I am not good at document writting. Below is my first trial.  Look
>> > forward your comments.
>> 
>> I'll have a look, maybe also Oscar and Michal can have a look. I guess
>> we don't have to cover all now, we can add more details as we discover them.
>
>Oscar, didn't you have something already?

Since we prefer to address the document in a separate patch, I will send
out v4 with changes suggested from Michal and David first.

>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

[PATCH v4 1/2] mm, sparse: drop pgdat_resize_lock in sparse_add/remove_one_section()

[Wei Yang]

pgdat_resize_lock is used to protect pgdat's memory region information
like: node_start_pfn, node_present_pages, etc. While in function
sparse_add/remove_one_section(), pgdat_resize_lock is used to protect
initialization/release of one mem_section. This looks not proper.

These code paths are currently protected by mem_hotplug_lock currently
but should there ever be any reason for locking at the sparse layer a
dedicated lock should be introduced.

Following is the current call trace of sparse_add/remove_one_section()

    mem_hotplug_begin()
    arch_add_memory()
       add_pages()
           __add_pages()
               __add_section()
                   sparse_add_one_section()
    mem_hotplug_done()

    mem_hotplug_begin()
    arch_remove_memory()
        __remove_pages()
            __remove_section()
                sparse_remove_one_section()
    mem_hotplug_done()

The comment above the pgdat_resize_lock also mentions "Holding this will
also guarantee that any pfn_valid() stays that way.", which is true with
the current implementation and false after this patch. But current
implementation doesn't meet this comment. There isn't any pfn walkers
to take the lock so this looks like a relict from the past. This patch
also removes this comment.

[Michal: changelog suggestion]

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Acked-by: Michal Hocko <mhocko@suse.com>

---
v4:
   * fix typo in changelog
   * adjust second paragraph of changelog
v3:
   * adjust the changelog with the reason for this change
   * remove a comment for pgdat_resize_lock
   * separate the prototype change of sparse_add_one_section() to
     another one
v2:
   * adjust changelog to show this procedure is serialized by global
     mem_hotplug_lock
---
 include/linux/mmzone.h | 2 --
 mm/sparse.c            | 9 +--------
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index d76177cb8436..be126113b499 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -639,8 +639,6 @@ typedef struct pglist_data {
 	/*
 	 * Must be held any time you expect node_start_pfn,
 	 * node_present_pages, node_spanned_pages or nr_zones stay constant.
-	 * Holding this will also guarantee that any pfn_valid() stays that
-	 * way.
 	 *
 	 * pgdat_resize_lock() and pgdat_resize_unlock() are provided to
 	 * manipulate node_size_lock without checking for CONFIG_MEMORY_HOTPLUG
diff --git a/mm/sparse.c b/mm/sparse.c
index 33307fc05c4d..5825f276485f 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -669,7 +669,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 	struct mem_section *ms;
 	struct page *memmap;
 	unsigned long *usemap;
-	unsigned long flags;
 	int ret;
 
 	/*
@@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 		return -ENOMEM;
 	}
 
-	pgdat_resize_lock(pgdat, &flags);
-
 	ms = __pfn_to_section(start_pfn);
 	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
 		ret = -EEXIST;
@@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 	sparse_init_one_section(ms, section_nr, memmap, usemap);
 
 out:
-	pgdat_resize_unlock(pgdat, &flags);
 	if (ret < 0) {
 		kfree(usemap);
 		__kfree_section_memmap(memmap, altmap);
@@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		unsigned long map_offset, struct vmem_altmap *altmap)
 {
 	struct page *memmap = NULL;
-	unsigned long *usemap = NULL, flags;
-	struct pglist_data *pgdat = zone->zone_pgdat;
+	unsigned long *usemap = NULL;
 
-	pgdat_resize_lock(pgdat, &flags);
 	if (ms->section_mem_map) {
 		usemap = ms->pageblock_flags;
 		memmap = sparse_decode_mem_map(ms->section_mem_map,
@@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		ms->section_mem_map = 0;
 		ms->pageblock_flags = NULL;
 	}
-	pgdat_resize_unlock(pgdat, &flags);
 
 	clear_hwpoisoned_pages(memmap + map_offset,
 			PAGES_PER_SECTION - map_offset);
-- 
2.15.1

[PATCH v4 2/2] mm, sparse: pass nid instead of pgdat to sparse_add_one_section()

[Wei Yang]

Since the information needed in sparse_add_one_section() is node id to
allocate proper memory, it is not necessary to pass its pgdat.

This patch changes the prototype of sparse_add_one_section() to pass
node id directly. This is intended to reduce misleading that
sparse_add_one_section() would touch pgdat.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Acked-by: Michal Hocko <mhocko@suse.com>

---
* adjust parameter alignment
---
 include/linux/memory_hotplug.h | 4 ++--
 mm/memory_hotplug.c            | 2 +-
 mm/sparse.c                    | 8 ++++----
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 45a5affcab8a..b81cc29482d8 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -333,8 +333,8 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
 		unsigned long nr_pages, struct vmem_altmap *altmap);
 extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
 extern bool is_memblock_offlined(struct memory_block *mem);
-extern int sparse_add_one_section(struct pglist_data *pgdat,
-		unsigned long start_pfn, struct vmem_altmap *altmap);
+extern int sparse_add_one_section(int nid, unsigned long start_pfn,
+				  struct vmem_altmap *altmap);
 extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
 		unsigned long map_offset, struct vmem_altmap *altmap);
 extern struct page *sparse_decode_mem_map(unsigned long coded_mem_map,
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index f626e7e5f57b..5b3a3d7b4466 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -253,7 +253,7 @@ static int __meminit __add_section(int nid, unsigned long phys_start_pfn,
 	if (pfn_valid(phys_start_pfn))
 		return -EEXIST;
 
-	ret = sparse_add_one_section(NODE_DATA(nid), phys_start_pfn, altmap);
+	ret = sparse_add_one_section(nid, phys_start_pfn, altmap);
 	if (ret < 0)
 		return ret;
 
diff --git a/mm/sparse.c b/mm/sparse.c
index 5825f276485f..a4fdbcb21514 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -662,8 +662,8 @@ static void free_map_bootmem(struct page *memmap)
  * set.  If this is <=0, then that means that the passed-in
  * map was not consumed and must be freed.
  */
-int __meminit sparse_add_one_section(struct pglist_data *pgdat,
-		unsigned long start_pfn, struct vmem_altmap *altmap)
+int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
+				     struct vmem_altmap *altmap)
 {
 	unsigned long section_nr = pfn_to_section_nr(start_pfn);
 	struct mem_section *ms;
@@ -675,11 +675,11 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 	 * no locking for this, because it does its own
 	 * plus, it does a kmalloc
 	 */
-	ret = sparse_index_init(section_nr, pgdat->node_id);
+	ret = sparse_index_init(section_nr, nid);
 	if (ret < 0 && ret != -EEXIST)
 		return ret;
 	ret = 0;
-	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
+	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
 	if (!memmap)
 		return -ENOMEM;
 	usemap = __kmalloc_section_usemap();
-- 
2.15.1

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

[David Hildenbrand]

On 04.12.18 09:56, Wei Yang wrote:
> pgdat_resize_lock is used to protect pgdat's memory region information
> like: node_start_pfn, node_present_pages, etc. While in function
> sparse_add/remove_one_section(), pgdat_resize_lock is used to protect
> initialization/release of one mem_section. This looks not proper.
> 
> These code paths are currently protected by mem_hotplug_lock currently
> but should there ever be any reason for locking at the sparse layer a
> dedicated lock should be introduced.
> 
> Following is the current call trace of sparse_add/remove_one_section()
> 
>     mem_hotplug_begin()
>     arch_add_memory()
>        add_pages()
>            __add_pages()
>                __add_section()
>                    sparse_add_one_section()
>     mem_hotplug_done()
> 
>     mem_hotplug_begin()
>     arch_remove_memory()
>         __remove_pages()
>             __remove_section()
>                 sparse_remove_one_section()
>     mem_hotplug_done()
> 
> The comment above the pgdat_resize_lock also mentions "Holding this will
> also guarantee that any pfn_valid() stays that way.", which is true with
> the current implementation and false after this patch. But current
> implementation doesn't meet this comment. There isn't any pfn walkers
> to take the lock so this looks like a relict from the past. This patch
> also removes this comment.
> 
> [Michal: changelog suggestion]
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> ---
> v4:
>    * fix typo in changelog
>    * adjust second paragraph of changelog
> v3:
>    * adjust the changelog with the reason for this change
>    * remove a comment for pgdat_resize_lock
>    * separate the prototype change of sparse_add_one_section() to
>      another one
> v2:
>    * adjust changelog to show this procedure is serialized by global
>      mem_hotplug_lock
> ---
>  include/linux/mmzone.h | 2 --
>  mm/sparse.c            | 9 +--------
>  2 files changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index d76177cb8436..be126113b499 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -639,8 +639,6 @@ typedef struct pglist_data {
>  	/*
>  	 * Must be held any time you expect node_start_pfn,
>  	 * node_present_pages, node_spanned_pages or nr_zones stay constant.
> -	 * Holding this will also guarantee that any pfn_valid() stays that
> -	 * way.
>  	 *
>  	 * pgdat_resize_lock() and pgdat_resize_unlock() are provided to
>  	 * manipulate node_size_lock without checking for CONFIG_MEMORY_HOTPLUG
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 33307fc05c4d..5825f276485f 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -669,7 +669,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  	struct mem_section *ms;
>  	struct page *memmap;
>  	unsigned long *usemap;
> -	unsigned long flags;
>  	int ret;
>  
>  	/*
> @@ -689,8 +688,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  		return -ENOMEM;
>  	}
>  
> -	pgdat_resize_lock(pgdat, &flags);
> -
>  	ms = __pfn_to_section(start_pfn);
>  	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
>  		ret = -EEXIST;
> @@ -707,7 +704,6 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  	sparse_init_one_section(ms, section_nr, memmap, usemap);
>  
>  out:
> -	pgdat_resize_unlock(pgdat, &flags);
>  	if (ret < 0) {
>  		kfree(usemap);
>  		__kfree_section_memmap(memmap, altmap);
> @@ -769,10 +765,8 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		unsigned long map_offset, struct vmem_altmap *altmap)
>  {
>  	struct page *memmap = NULL;
> -	unsigned long *usemap = NULL, flags;
> -	struct pglist_data *pgdat = zone->zone_pgdat;
> +	unsigned long *usemap = NULL;
>  
> -	pgdat_resize_lock(pgdat, &flags);
>  	if (ms->section_mem_map) {
>  		usemap = ms->pageblock_flags;
>  		memmap = sparse_decode_mem_map(ms->section_mem_map,
> @@ -780,7 +774,6 @@ void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
>  		ms->section_mem_map = 0;
>  		ms->pageblock_flags = NULL;
>  	}
> -	pgdat_resize_unlock(pgdat, &flags);
>  
>  	clear_hwpoisoned_pages(memmap + map_offset,
>  			PAGES_PER_SECTION - map_offset);
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb