Messages in this thread | | | Date | Sun, 17 Jan 2010 13:53:18 -0800 (PST) | From | David Rientjes <> | Subject | Re: [PATCH] x86/mm/srat_64.c: nodes_parsed should include all nodes detected by ACPI. |
| |
On Fri, 15 Jan 2010, Haicheng Li wrote:
> This is to fix the bug discussed in email thread: > http://patchwork.kernel.org/patch/69499/. >
It would be better to include a brief synopsis of the problem in the changelog instead of a citation, otherwise it obfuscates the issue it solves.
> Currently node_possible_map won't include the offlined node that has neither > CPU onlined nor MEM onlined at booting time. As a result, nr_node_ids won't be > equal to possible nodes. > > CC: Thomas Gleixner <tglx@linutronix.de> > CC: Ingo Molnar <mingo@redhat.com> > CC: H. Peter Anvin <hpa@zytor.com> > CC: Andi Kleen <andi@firstfloor.org> > Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> > --- > arch/x86/mm/srat_64.c | 10 ++-------- > 1 files changed, 2 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/mm/srat_64.c b/arch/x86/mm/srat_64.c > index a271241..a5bc297 100644 > --- a/arch/x86/mm/srat_64.c > +++ b/arch/x86/mm/srat_64.c > @@ -238,7 +238,7 @@ update_nodes_add(int node, unsigned long start, unsigned > long end) > void __init > acpi_numa_memory_affinity_init(struct acpi_srat_mem_affinity *ma) > { > - struct bootnode *nd, oldnode; > + struct bootnode *nd; > unsigned long start, end; > int node, pxm; > int i; > @@ -277,7 +277,6 @@ acpi_numa_memory_affinity_init(struct > acpi_srat_mem_affinity *ma) > return; > } > nd = &nodes[node]; > - oldnode = *nd; > if (!node_test_and_set(node, nodes_parsed)) { > nd->start = start; > nd->end = end; > @@ -291,13 +290,8 @@ acpi_numa_memory_affinity_init(struct > acpi_srat_mem_affinity *ma) > printk(KERN_INFO "SRAT: Node %u PXM %u %lx-%lx\n", node, pxm, > start, end); > > - if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) { > + if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) > update_nodes_add(node, start, end); > - /* restore nodes[node] */ > - *nd = oldnode; > - if ((nd->start | nd->end) == 0) > - node_clear(node, nodes_parsed); > - } > > node_memblk_range[num_node_memblks].start = start; > node_memblk_range[num_node_memblks].end = end;
There're a couple of issues with this patch:
- it breaks CONFIG_MEMORY_HOTPLUG_SPARSE kernels when the SRAT reports more than one entry for a single node id and the later entry does not specify an address range, and
- carrying the bit for the node over in nodes_parsed is inappropriate since other x86 code depends on that map including only nodes with memory such as e820_register_active_regions() and nodes_cover_memory().
[ The existing naming is admittedly convoluted since nodes_parsed represents nodes with memory and cpu_nodes_parsed represents nodes with only cpus; in actuality, they should probably be mem_nodes_parsed and no_mem_nodes_parsed, respectively, since the latter will now include hotpluggable nodes. ]
To fix both of these issues simultaneously, I think it would be better to set the bit in no_mem_nodes_parsed so that it gets unioned in acpi_scan_nodes() when nodes_possible_map is actually formed.
| |