Re: [PATCH] x86, acpi: map hotadded cpu to correct node.

[Thomas Renninger <trenn@suse.de>]

On Monday 08 February 2010 03:48:06 Haicheng Li wrote:
> hello,
> 
> any comments on this patch? in fact, it's a straightforward bug fix: with existing
> CPU hotadd code, new added CPUs won't be mapped to its own node. especially for hotadding
> a new node with CPU and MEM, new added memories can be mapped to this new node, but new added
> CPUs are always mapped to old nodes. This patch is to fix this obvious bug. thanks.
> 
I can confirm that this patch works as expected:
Tested-by: Thomas Renninger <trenn@suse.de>

While the cores previously showed up on the wrong, already existing
node, they are now added to the correct one.

Be aware that there seem to be other issues (Andi posted some
slab memory hot plug fixes recently).

Find one "nit pick" below:

> -haicheng
> 
> Haicheng Li wrote:
> > x86: map hotadded cpu to correct node.
> > 
> > When hotadd new cpu to system, if its affinitive node is online, should 
> > map the cpu to its own node. otherwise, let kernel select one online 
> > node for the new cpu later.
> > 
> > Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com>
> > ---
> >  arch/x86/kernel/acpi/boot.c |   21 +++++++++++++++++++++
> >  1 files changed, 21 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
> > index 67e929b..92a4861 100644
> > --- a/arch/x86/kernel/acpi/boot.c
> > +++ b/arch/x86/kernel/acpi/boot.c
> > @@ -49,6 +49,7 @@ EXPORT_SYMBOL(acpi_disabled);
> > 
> >  #ifdef    CONFIG_X86_64
> >  # include <asm/proto.h>
> > +# include <asm/numa_64.h>
> >  #endif                /* X86 */
> > 
> >  #define BAD_MADT_ENTRY(entry, end) (                        \
> > @@ -482,6 +483,25 @@ int acpi_register_gsi(struct device *dev, u32 gsi, 
> > int trigger, int polarity)
> >   */
> >  #ifdef CONFIG_ACPI_HOTPLUG_CPU
> > 
> > +static void acpi_map_cpu2node(acpi_handle handle, int cpu, int physid)
> > +{
> > +#ifdef CONFIG_ACPI_NUMA
> > +    int nid;
> > +
> > +    nid = acpi_get_node(handle);
> > +    if (!node_online(nid))
if (nid == -1 || !node_online(nid))
would avoid passing an invalid param to node_online(..)
node_online() probably already can handle this...
A maintainer eventually could fiddle this into the patch/line
without the need of re-posting.

I am not that familiar with numa node handling, but went the code and
spec up and down a bit. Also assigning a node no. is rather straight
forward, thus I can give this a:
Reviewed-by: Thomas Renninger <trenn@suse.de>

Thanks,

   Thomas