[PATCH] sparsemem interaction with memory add bug fixes

[PATCH] sparsemem interaction with memory add bug fixes

[Mike Kravetz]

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;
 }

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

[jschopp]

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>
> 

These fix memory adding spu addresses on cell, among other things.

Signed-off-by: Joel Schopp <jschopp@austin.ibm.com>

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

[Andy Whitcroft]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

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


[-- Attachment #2: fix-leak-in-sparse_add_one_section --]
[-- Type: text/plain, Size: 479 bytes --]

 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;
 }

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

[jschopp]

Andy Whitcroft wrote:
> 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;
>  }

Whatever, I don't really care.  It doesn't matter functionally if we free under the 
pgdat_resize lock or not.  This looks fine, as does the original patch.  If resizing 
pgdats was a hot path we might prefer this way outside the lock.

-Joel

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

[Dave Hansen]

On Tue, 2006-04-11 at 19:33 -0700, 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>.

At first glance, these seem fine to me.

Just out of curiosity.  Can these issues be revealed with current code,
or do they only show up with the cell changes?

-- Dave

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

[Mike Kravetz]

On Wed, Apr 12, 2006 at 07:46:04AM -0700, Dave Hansen wrote:
> Just out of curiosity.  Can these issues be revealed with current code,
> or do they only show up with the cell changes?

Yes, you could hit either of these doing memory adds with the current
code.  The follow up issue of adding sub-section amounts of memory is
cell specific.

-- 
Mike