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 <[email protected]> and Joel Schopp <[email protected]>.
Signed-off-by: Mike Kravetz <[email protected]>
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]