lkml.org 
[lkml]   [2010]   [Jan]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.


\
 
 \ /
  Last update: 2010-01-17 22:55    [from the cache]
©2003-2011 Jasper Spaans