Re: [PATCH] sparsemem interaction with memory add bug fixes

[Andy Whitcroft <apw@shadowen.org>]

Mike Kravetz wrote:
> This patch fixes two bugs with the way sparsemem interacts with memory
> add.  They are:
> - memory leak if memmap for section already exists
> - calling alloc_bootmem_node() after boot
> These bugs were discovered and a first cut at the fixes were provided by
> Arnd Bergmann <arnd@arndb.de> and Joel Schopp <jschopp@us.ibm.com>.
> 
> Signed-off-by: Mike Kravetz <kravetz@us.ibm.com>
> 
> diff -Naupr linux-2.6.17-rc1-mm2/mm/sparse.c linux-2.6.17-rc1-mm2.work/mm/sparse.c
> --- linux-2.6.17-rc1-mm2/mm/sparse.c	2006-04-03 03:22:10.000000000 +0000
> +++ linux-2.6.17-rc1-mm2.work/mm/sparse.c	2006-04-11 23:32:10.000000000 +0000
> @@ -32,7 +32,10 @@ static struct mem_section *sparse_index_
>  	unsigned long array_size = SECTIONS_PER_ROOT *
>  				   sizeof(struct mem_section);
>  
> -	section = alloc_bootmem_node(NODE_DATA(nid), array_size);
> +	if (system_state == SYSTEM_RUNNING)
> +		section = kmalloc_node(array_size, GFP_KERNEL, nid);
> +	else
> +		section = alloc_bootmem_node(NODE_DATA(nid), array_size);
>  
>  	if (section)
>  		memset(section, 0, array_size);
> @@ -281,9 +284,9 @@ int sparse_add_one_section(struct zone *
>  
>  	ret = sparse_init_one_section(ms, section_nr, memmap);
>  
> +out:
>  	if (ret <= 0)
>  		__kfree_section_memmap(memmap, nr_pages);
> -out:
>  	pgdat_resize_unlock(pgdat, &flags);
>  	return ret;
>  }

First change looks sane.  For the second it makes it obvious that we are
freeing the alloc'd section within the pgdat resize lock.  Doesn't seem
to make any sense to do that to me?  Perhaps it should be more like the
attached.

-apw

 sparse.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
diff -upN reference/mm/sparse.c current/mm/sparse.c
--- reference/mm/sparse.c
+++ current/mm/sparse.c
@@ -281,9 +281,9 @@ int sparse_add_one_section(struct zone *
 
 	ret = sparse_init_one_section(ms, section_nr, memmap);
 
-	if (ret <= 0)
-		__kfree_section_memmap(memmap, nr_pages);
 out:
 	pgdat_resize_unlock(pgdat, &flags);
+	if (ret <= 0)
+		__kfree_section_memmap(memmap, nr_pages);
 	return ret;
 }