lkml.org 
[lkml]   [2006]   [Apr]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] sparsemem interaction with memory add bug fixes
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-04-12 15:47    [W:0.076 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site