Re: [Qemu-devel] [RFC 01/13] numa: access CPU's node id via property in hmp_info_numa()

[Eduardo Habkost]

On Wed, Jan 18, 2017 at 06:13:17PM +0100, Igor Mammedov wrote:
> Move vcpu's assocciated numa_node field out of generic CPUState
> into inherited classes that actually care about cpu<->numa mapping
> and make monitor 'info numa' get vcpu's assocciated node id via
> node-id property.
> It allows to drop implicit node id intialization in
> numa_post_machine_init() and would allow switching to mapping
> defined by target's CpuInstanceProperties instead of global
> numa_info[i].node_cpu bitmaps.
> 
> As side effect it fixes 'info numa' displaying wrong mapping
> for CPUs specified with -device/device_add.
> Before patch following CLI would produce:
> QEMU -smp 1,sockets=3,maxcpus=3 \
>        -device qemu64-x86_64-cpu,socket-id=1,core-id=0,thread-id=0 \
>        -numa node,nodeid=0,cpus=0 \
>        -numa node,nodeid=1,cpus=1 \
>        -numa node,nodeid=2,cpus=2
> (qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
> (qemu) info numa
> 3 nodes
> node 0 cpus: 0 1 2
> node 0 size: 40 MB
> node 1 cpus:
> node 1 size: 40 MB
> node 2 cpus:
> node 2 size: 48 MB
> 
> after patch all CPUs are on nodes they are supposed to be:
> (qemu) device_add qemu64-x86_64-cpu,socket-id=2,core-id=0,thread-id=0
> (qemu) info numa
> 3 nodes
> node 0 cpus: 0
> node 0 size: 40 MB
> node 1 cpus: 1
> node 1 size: 40 MB
> node 2 cpus: 2
> node 2 size: 48 MB
> 
> Signed-off-by: Igor Mammedov <address@hidden>

So, in addition to the other comments I had, it looks like this
patch is doing 3 things at the same time:

1) Adding a "node-id" property to CPU objects.
2) Moving CPUState::numa_node to arch-specific CPU structs.
3) Fixing the bug where the NUMA node info isn't initialized
   for PC on CPUs created by -device/device_add.

All of them are independent from each other. For example, all you
need to fix the bug you describe above is (3), which is contained
in a single hunk from this patch, that is:

> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f721fde..9d2b265 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1978,6 +1978,11 @@ static void pc_cpu_pre_plug(HotplugHandler 
> *hotplug_dev,
>  
>      cs = CPU(cpu);
>      cs->cpu_index = idx;
> +
> +    idx = numa_get_node_for_cpu(cs->cpu_index);
> +    if (idx < nb_numa_nodes) {
> +        cpu->numa_nid = idx;
> +    }
>  }

All the rest seems irrelevant to fix the bug. (1) and (2) may be
useful, but they need separate justification.

-- 
Eduardo