On Wed, 18 Jan 2017 16:18:20 -0200
Eduardo Habkost <ehabk...@redhat.com> wrote:
> 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 <imamm...@redhat.com>
>
> 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.
ok, I'll split this in 3 patches
>
> 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.
rationale for (1) and (2) is the first 2 paragraphs of this commit message
