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

[Eduardo Habkost]

On Wed, Jan 18, 2017 at 06:08:24PM +0100, Igor Mammedov wrote:
> On Wed, 18 Jan 2017 13:19:45 -0200
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Wed, Jan 18, 2017 at 03:48:45PM +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.  
> > 
> > I agree to allow the node-id assignment logic to be defined by
> > arch-specific code, but:
> > 
> > Considering that we still have the generic CPU<->node mapping in
> > numa_info[].node_cpu, I don't see the benefit of moving the
> > numa_info[].node_cpu => node-id translation from common generic
> > code to duplicated arch-specific code.
> it's duplicated for a time being until all targets that use
> node_cpu are internally converted to socket/core/thread-id interface.
> This patch is a part if re-factoring RFC which I'm about to post.
> 
> 

Maybe the duplication will be OK if it's temporary, and going to
be removed immediately by the same series. But if it is a
standalone patch, I don't want to make the existing duplication
worse.

> > What about adding this to cpu_generic_realize():
> > 
> >   node = (numa_get_node_for_cpu(cpu)
> >   if (node >= 0) {
> >      int cur_node = object_property_find(cpu, "node-id") ?
> >                     object_property_get_int(cpu, "node-id") : -1;
> >      if (cur_node >= 0 && cur_node != node) {
> >          error_setg(&err, "Conflict between -numa option and CPU node-id");
> >          goto out;
> >      }
> >      object_property_set_int(cpu, "node-id", node, &err);
> >   }
> What I don't like here is that putting above snippet into
> cpu_generic_realize() is just doing the same implicit init
> that numa_post_machine_init() has been doing before just
> a bit later. It looks like a quick fix for 'info numa' issues.

Yes, it is. But I still don't see what's the problem with it. If
numa_info[].node_cpu still exists in generic code, I don't see
why not handle it in generic code.

> 
> However it would add stumble block on getting rid of 
> numa_get_node_for_cpu() { numa_info[].node_cpu } and keeping
> node-mapping along with the rest of topology in Machine object
> instead of globals.

Removing duplication would make it easier: then you'll have only
one user of numa_info[].node_cpu/numa_get_node_for_cpu() to get
rid of, instead of 4.

> 
> I'm in process of getting rid of numa_info[].node_cpu/numa_get_node_for_cpu()
> altogether. And incrementally done it for PC, so I'd be forced
> to reduplicate above snippet from cpu_generic_realize() in concrete
> users anyway to do conversion without breaking other users.

If you manage to get rid of numa_info[].node_cpu somehow, then we
could get rid of numa_post_machine_init()/numa_get_node_for_cpu()
at the same time. But I don't see the benefit of duplicating
existing generic code into arch-specific code while keeping the
existing generic numa_info[].node_cpu data structure untouched.

>  
> [...]
> > > --- 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;
> > > +    }
> this is the only place where I add above mentioned duplication,
> which sort of compensated by removed numa_post_machine_init().

Is it? I see the same logic duplicated in 3 places (see below).
Not all cases were added by you, but that doesn't mean it is OK
to make it worse.

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;
+    }
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index c18632b..7f6661b 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -71,7 +71,7 @@ void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU 
*cpu, Error **errp)
     /* Set NUMA node for the added CPUs  */
     i = numa_get_node_for_cpu(cs->cpu_index);
     if (i < nb_numa_nodes) {
-            cs->numa_node = i;
+        cpu->numa_nid = i;
     }
 
     xics_cpu_setup(spapr->xics, cpu);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7a03f84..b86b5fd 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1333,6 +1332,11 @@ static void machvirt_init(MachineState *machine)
                                      "secure-memory", &error_abort);
         }
 
+        if (nb_numa_nodes) {
+            object_property_set_int(cpuobj, numa_get_node_for_cpu(n),
+                                    "node-id", NULL);
+        }
+
         object_property_set_bool(cpuobj, true, "realized", NULL);
     }
     fdt_add_timer_nodes(vms);


> And it's here only to keep working+fix 'info numa'.
> The other targets currently already do numa_get_node_for_cpu(),
> lookup so I'm not changing there anything.
> Later(not in this patch) I'm removing numa_get_node_for_cpu() from
> pc_cpu_pre_plug() and from PC code altogether and replacing it
> with node-id from possible_cpus.

I will look at the whole series to re-evaluate this patch, then.

> 
> PS:
> since travis-ci seems to be broken, I'll post re-factoring
> RFC without building there and it includes this patch as well.

No problem to me, if it's a RFC.

-- 
Eduardo