Re: [PATCH] x86, mm: clean up setup_node_bootmem

From: David Rientjes
Date: Thu Mar 03 2011 - 17:04:33 EST


On Thu, 3 Mar 2011, Yinghai Lu wrote:

> >> Index: linux-2.6/arch/x86/mm/numa_64.c
> >> ===================================================================
> >> --- linux-2.6.orig/arch/x86/mm/numa_64.c
> >> +++ linux-2.6/arch/x86/mm/numa_64.c
> >> @@ -237,21 +237,18 @@ int __init numa_add_memblk(int nid, u64
> >> }
> >>
> >> /* Initialize bootmem allocator for a node */
> >> -void __init
> >> +static void __init
> >> setup_node_bootmem(int nodeid, unsigned long start, unsigned long end)
> >> {
> >> unsigned long start_pfn, last_pfn, nodedata_phys;
> >> const int pgdat_size = roundup(sizeof(pg_data_t), PAGE_SIZE);
> >> int nid;
> >>
> >> - if (!end)
> >> - return;
> >> -
> >> /*
> >> * Don't confuse VM with a node that doesn't have the
> >> * minimum amount of memory:
> >> */
> >> - if (end && (end - start) < NODE_MIN_SIZE)
> >> + if (end < (start + NODE_MIN_SIZE))
> >> return;
> >>
> >> start = roundup(start, ZONE_ALIGN);
> >> @@ -557,8 +554,7 @@ static int __init numa_register_memblks(
> >> end = max(mi->blk[i].end, end);
> >> }
> >>
> >> - if (start < end)
> >> - setup_node_bootmem(nid, start, end);
> >> + setup_node_bootmem(nid, start, end);
> >> }
> >>
> >> return 0;
> >>
> >
> > Good catch on finding only the one caller of setup_node_bootmem().
> >
> > I'd actually rather see the validity checking being done in
> > numa_register_memblks(), though, because it's a bug for a node to be set
> > in node_possible_map without having a valid
> > [mi->blk[i].start, mi->blk[i].end) range.
> >
> > So could this be
> >
> > BUG_ON(end < start);
>
> no, it could cause crash here
>
> /* Finally register nodes. */
> for_each_node_mask(nid, node_possible_map) {
> u64 start = (u64)max_pfn << PAGE_SHIFT;
> u64 end = 0;
>
> for (i = 0; i < mi->nr_blks; i++) {
> if (nid != mi->blk[i].nid)
> continue;
> start = min(mi->blk[i].start, start);
> end = max(mi->blk[i].end, end);
> }
>
> could have some node without memory. and it could be set in node_possible_map, and it will have end = 0, and start max_pfn<<page_shift.
>

Ok, I didn't realize this was being used for memoryless nodes.

> > /*
> > * Don't confuse the VM with a node that doesn't have the minimum
> > * amount of memory.
> > */
> > if (end < start + NODE_MIN_SIZE) {
> > node_clear(nid, node_possible_map);
>
> why touch that possible_map ?
> online_map is for that purpose that state node have ram.
>

Hmm, that's the purpose of N_NORMAL_MEMORY, not node_online_map. I
understand why we don't want to register bootmem for a node that has such
little memory that we disregard it, but shouldn't we then consider it to
no longer be possible?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/