[Qemu-devel] [PATCH 0/3] numa: code consolidation and test fixup

[Qemu-devel] [PATCH 0/3] numa: code consolidation and test fixup

[Igor Mammedov]

git repo for testing:
   https://github.com/imammedo/qemu.git cphp_numa_cfg_follow_up_v3_cleanups_v1

CC: qemu-arm@nongnu.org
CC: qemu-ppc@nongnu.org
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Andrew Jones <drjones@redhat.com>

Igor Mammedov (3):
  numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr
  numa: move default mapping init to machine
  numa: silence incomplete mapping warning under qtest

 include/sysemu/numa.h |  1 +
 hw/arm/virt.c         | 16 ++--------------
 hw/core/machine.c     | 35 +++++++++++++++++++++++------------
 hw/i386/pc.c          | 17 +----------------
 hw/ppc/spapr.c        | 17 +----------------
 numa.c                | 48 ++++++++++++++++++++++--------------------------
 6 files changed, 50 insertions(+), 84 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/sysemu/numa.h |  1 +
 hw/arm/virt.c         | 16 ++--------------
 hw/i386/pc.c          | 17 +----------------
 hw/ppc/spapr.c        | 17 +----------------
 numa.c                | 22 ++++++++++++++++++++++
 5 files changed, 27 insertions(+), 46 deletions(-)

diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index 7ffde5b..610eece 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
                                  int nb_nodes, ram_addr_t size);
 void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
                                   int nb_nodes, ram_addr_t size);
+void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);
 #endif
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c7c8159..ce676df 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1351,7 +1351,6 @@ static void machvirt_init(MachineState *machine)
     for (n = 0; n < possible_cpus->len; n++) {
         Object *cpuobj;
         CPUState *cs;
-        int node_id;
 
         if (n >= smp_cpus) {
             break;
@@ -1364,19 +1363,8 @@ static void machvirt_init(MachineState *machine)
         cs = CPU(cpuobj);
         cs->cpu_index = n;
 
-        node_id = possible_cpus->cpus[cs->cpu_index].props.node_id;
-        if (!possible_cpus->cpus[cs->cpu_index].props.has_node_id) {
-            /* by default CPUState::numa_node was 0 if it's not set via CLI
-             * keep it this way for now but in future we probably should
-             * refuse to start up with incomplete numa mapping */
-             node_id = 0;
-        }
-        if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
-            cs->numa_node = node_id;
-        } else {
-            /* CPU isn't device_add compatible yet, this shouldn't happen */
-            error_setg(&error_abort, "user set node-id not implemented");
-        }
+        numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
+                          &error_fatal);
 
         if (!vms->secure) {
             object_property_set_bool(cpuobj, false, "has_el3", NULL);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e36a375..d83c158 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1895,7 +1895,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
                             DeviceState *dev, Error **errp)
 {
     int idx;
-    int node_id;
     CPUState *cs;
     CPUArchId *cpu_slot;
     X86CPUTopoInfo topo;
@@ -1986,21 +1985,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
     cs = CPU(cpu);
     cs->cpu_index = idx;
 
-    node_id = cpu_slot->props.node_id;
-    if (!cpu_slot->props.has_node_id) {
-        /* by default CPUState::numa_node was 0 if it's not set via CLI
-         * keep it this way for now but in future we probably should
-         * refuse to start up with incomplete numa mapping */
-        node_id = 0;
-    }
-    if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
-        cs->numa_node = node_id;
-    } else if (cs->numa_node != node_id) {
-            error_setg(errp, "node-id %d must match numa node specified"
-                "with -numa option for cpu-index %d",
-                cs->numa_node, cs->cpu_index);
-            return;
-    }
+    numa_cpu_pre_plug(cpu_slot, dev, errp);
 }
 
 static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0980d73..c7fee8b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2831,11 +2831,9 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
     Error *local_err = NULL;
     CPUCore *cc = CPU_CORE(dev);
-    sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
     char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
     const char *type = object_get_typename(OBJECT(dev));
     CPUArchId *core_slot;
-    int node_id;
     int index;
 
     if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
@@ -2870,20 +2868,7 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out;
     }
 
-    node_id = core_slot->props.node_id;
-    if (!core_slot->props.has_node_id) {
-        /* by default CPUState::numa_node was 0 if it's not set via CLI
-         * keep it this way for now but in future we probably should
-         * refuse to start up with incomplete numa mapping */
-        node_id = 0;
-    }
-    if (sc->node_id == CPU_UNSET_NUMA_NODE_ID) {
-        sc->node_id = node_id;
-    } else if (sc->node_id != node_id) {
-        error_setg(&local_err, "node-id %d must match numa node specified"
-            "with -numa option for cpu-index %d", sc->node_id, cc->core_id);
-        goto out;
-    }
+    numa_cpu_pre_plug(core_slot, dev, &local_err);
 
 out:
     g_free(base_core_type);
diff --git a/numa.c b/numa.c
index ca73145..0115bfd 100644
--- a/numa.c
+++ b/numa.c
@@ -534,6 +534,28 @@ void parse_numa_opts(MachineState *ms)
     }
 }
 
+void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp)
+{
+    int node_id = object_property_get_int(OBJECT(dev), "node-id", errp);
+
+    if (node_id == CPU_UNSET_NUMA_NODE_ID) {
+        /* by default CPUState::numa_node was 0 if it wasn't set explicitly
+         * TODO: make it error when incomplete numa mapping support is removed
+         */
+        node_id = 0;
+
+        /* due to bug in libvirt, it doesn't pass node-id from props on
+         * device_add as expected, so we have to fix it up here */
+        if (slot->props.has_node_id) {
+            node_id = slot->props.node_id;
+        }
+        object_property_set_int(OBJECT(dev), node_id, "node-id", errp);
+    } else if (node_id != slot->props.node_id) {
+        error_setg(errp, "node-id=%d must match numa node specified "
+                   "with -numa option", node_id);
+    }
+}
+
 static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
                                            const char *name,
                                            uint64_t ram_size)
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr

[Eduardo Habkost]

On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/sysemu/numa.h |  1 +
>  hw/arm/virt.c         | 16 ++--------------
>  hw/i386/pc.c          | 17 +----------------
>  hw/ppc/spapr.c        | 17 +----------------
>  numa.c                | 22 ++++++++++++++++++++++
>  5 files changed, 27 insertions(+), 46 deletions(-)
> 
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 7ffde5b..610eece 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>                                   int nb_nodes, ram_addr_t size);
>  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>                                    int nb_nodes, ram_addr_t size);
> +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);

I understand an explicitly call to numa_cpu_pre_plug() is needed
on spapr_core_pre_plug() because it is not handling a TYPE_CPU
object. But why not adding a numa_cpu_pre_plug() call to
cpu_common_realizefn(), so the explicit calls in machvirt_init()
and pc_cpu_pre_plug() are not necessary?

Adding the code to cpu_common_realizefn() would also ensure
CPUState::node_id is set consistently, even if hotplug was not
done at thread level.


>  #endif
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index c7c8159..ce676df 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1351,7 +1351,6 @@ static void machvirt_init(MachineState *machine)
>      for (n = 0; n < possible_cpus->len; n++) {
>          Object *cpuobj;
>          CPUState *cs;
> -        int node_id;
>  
>          if (n >= smp_cpus) {
>              break;
> @@ -1364,19 +1363,8 @@ static void machvirt_init(MachineState *machine)
>          cs = CPU(cpuobj);
>          cs->cpu_index = n;
>  
> -        node_id = possible_cpus->cpus[cs->cpu_index].props.node_id;
> -        if (!possible_cpus->cpus[cs->cpu_index].props.has_node_id) {
> -            /* by default CPUState::numa_node was 0 if it's not set via CLI
> -             * keep it this way for now but in future we probably should
> -             * refuse to start up with incomplete numa mapping */
> -             node_id = 0;
> -        }
> -        if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> -            cs->numa_node = node_id;
> -        } else {
> -            /* CPU isn't device_add compatible yet, this shouldn't happen */
> -            error_setg(&error_abort, "user set node-id not implemented");
> -        }
> +        numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
> +                          &error_fatal);
>  
>          if (!vms->secure) {
>              object_property_set_bool(cpuobj, false, "has_el3", NULL);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e36a375..d83c158 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1895,7 +1895,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                              DeviceState *dev, Error **errp)
>  {
>      int idx;
> -    int node_id;
>      CPUState *cs;
>      CPUArchId *cpu_slot;
>      X86CPUTopoInfo topo;
> @@ -1986,21 +1985,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>      cs = CPU(cpu);
>      cs->cpu_index = idx;
>  
> -    node_id = cpu_slot->props.node_id;
> -    if (!cpu_slot->props.has_node_id) {
> -        /* by default CPUState::numa_node was 0 if it's not set via CLI
> -         * keep it this way for now but in future we probably should
> -         * refuse to start up with incomplete numa mapping */
> -        node_id = 0;
> -    }
> -    if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> -        cs->numa_node = node_id;
> -    } else if (cs->numa_node != node_id) {
> -            error_setg(errp, "node-id %d must match numa node specified"
> -                "with -numa option for cpu-index %d",
> -                cs->numa_node, cs->cpu_index);
> -            return;
> -    }
> +    numa_cpu_pre_plug(cpu_slot, dev, errp);
>  }
>  
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0980d73..c7fee8b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2831,11 +2831,9 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>      Error *local_err = NULL;
>      CPUCore *cc = CPU_CORE(dev);
> -    sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
>      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
>      const char *type = object_get_typename(OBJECT(dev));
>      CPUArchId *core_slot;
> -    int node_id;
>      int index;
>  
>      if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
> @@ -2870,20 +2868,7 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          goto out;
>      }
>  
> -    node_id = core_slot->props.node_id;
> -    if (!core_slot->props.has_node_id) {
> -        /* by default CPUState::numa_node was 0 if it's not set via CLI
> -         * keep it this way for now but in future we probably should
> -         * refuse to start up with incomplete numa mapping */
> -        node_id = 0;
> -    }
> -    if (sc->node_id == CPU_UNSET_NUMA_NODE_ID) {
> -        sc->node_id = node_id;
> -    } else if (sc->node_id != node_id) {
> -        error_setg(&local_err, "node-id %d must match numa node specified"
> -            "with -numa option for cpu-index %d", sc->node_id, cc->core_id);
> -        goto out;
> -    }
> +    numa_cpu_pre_plug(core_slot, dev, &local_err);
>  
>  out:
>      g_free(base_core_type);
> diff --git a/numa.c b/numa.c
> index ca73145..0115bfd 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -534,6 +534,28 @@ void parse_numa_opts(MachineState *ms)
>      }
>  }
>  
> +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp)
> +{
> +    int node_id = object_property_get_int(OBJECT(dev), "node-id", errp);

You don't check for errors here. If they will never happen,
should we use &error_abort instead?

> +
> +    if (node_id == CPU_UNSET_NUMA_NODE_ID) {
> +        /* by default CPUState::numa_node was 0 if it wasn't set explicitly
> +         * TODO: make it error when incomplete numa mapping support is removed
> +         */
> +        node_id = 0;
> +
> +        /* due to bug in libvirt, it doesn't pass node-id from props on
> +         * device_add as expected, so we have to fix it up here */
> +        if (slot->props.has_node_id) {
> +            node_id = slot->props.node_id;
> +        }
> +        object_property_set_int(OBJECT(dev), node_id, "node-id", errp);
> +    } else if (node_id != slot->props.node_id) {
> +        error_setg(errp, "node-id=%d must match numa node specified "
> +                   "with -numa option", node_id);

There's less detail on the error message, now. Probably harmless,
but I would like to understand when exactly this can be
triggered: is device_add the only way to trigger this error
message?

> +    }
> +}
> +
>  static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
>                                             const char *name,
>                                             uint64_t ram_size)
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr

[Igor Mammedov]

On Thu, 18 May 2017 15:19:13 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/sysemu/numa.h |  1 +
> >  hw/arm/virt.c         | 16 ++--------------
> >  hw/i386/pc.c          | 17 +----------------
> >  hw/ppc/spapr.c        | 17 +----------------
> >  numa.c                | 22 ++++++++++++++++++++++
> >  5 files changed, 27 insertions(+), 46 deletions(-)
> > 
> > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > index 7ffde5b..610eece 100644
> > --- a/include/sysemu/numa.h
> > +++ b/include/sysemu/numa.h
> > @@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> >                                   int nb_nodes, ram_addr_t size);
> >  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> >                                    int nb_nodes, ram_addr_t size);
> > +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);  
> 
> I understand an explicitly call to numa_cpu_pre_plug() is needed
> on spapr_core_pre_plug() because it is not handling a TYPE_CPU
> object. But why not adding a numa_cpu_pre_plug() call to
> cpu_common_realizefn(), so the explicit calls in machvirt_init()
> and pc_cpu_pre_plug() are not necessary?
1. of the reasons is not to pollute all cpus with numa code

> Adding the code to cpu_common_realizefn() would also ensure
> CPUState::node_id is set consistently, even if hotplug was not
> done at thread level.
2. not all CPUs have node-id property
3. call site of thread_realize() in encapsulating object (spapr_core)
   might be somewhere in the middle of parent's realize and likely
   failure would need proper parent state rollback/cleanup.
4. and finely it's not cpu's responsibility to assign/check
   node-id property. It's machine's job that owns/manages topology
   layout. It' the same like with socket/core/thread properties.
   So for the sake of small optimization, I'm not really willing
   to violate that.
 
> >  #endif
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index c7c8159..ce676df 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1351,7 +1351,6 @@ static void machvirt_init(MachineState *machine)
> >      for (n = 0; n < possible_cpus->len; n++) {
> >          Object *cpuobj;
> >          CPUState *cs;
> > -        int node_id;
> >  
> >          if (n >= smp_cpus) {
> >              break;
> > @@ -1364,19 +1363,8 @@ static void machvirt_init(MachineState *machine)
> >          cs = CPU(cpuobj);
> >          cs->cpu_index = n;
> >  
> > -        node_id = possible_cpus->cpus[cs->cpu_index].props.node_id;
> > -        if (!possible_cpus->cpus[cs->cpu_index].props.has_node_id) {
> > -            /* by default CPUState::numa_node was 0 if it's not set via CLI
> > -             * keep it this way for now but in future we probably should
> > -             * refuse to start up with incomplete numa mapping */
> > -             node_id = 0;
> > -        }
> > -        if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> > -            cs->numa_node = node_id;
> > -        } else {
> > -            /* CPU isn't device_add compatible yet, this shouldn't happen */
> > -            error_setg(&error_abort, "user set node-id not implemented");
> > -        }
> > +        numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
> > +                          &error_fatal);
> >  
> >          if (!vms->secure) {
> >              object_property_set_bool(cpuobj, false, "has_el3", NULL);
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index e36a375..d83c158 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1895,7 +1895,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >                              DeviceState *dev, Error **errp)
> >  {
> >      int idx;
> > -    int node_id;
> >      CPUState *cs;
> >      CPUArchId *cpu_slot;
> >      X86CPUTopoInfo topo;
> > @@ -1986,21 +1985,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
> >      cs = CPU(cpu);
> >      cs->cpu_index = idx;
> >  
> > -    node_id = cpu_slot->props.node_id;
> > -    if (!cpu_slot->props.has_node_id) {
> > -        /* by default CPUState::numa_node was 0 if it's not set via CLI
> > -         * keep it this way for now but in future we probably should
> > -         * refuse to start up with incomplete numa mapping */
> > -        node_id = 0;
> > -    }
> > -    if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> > -        cs->numa_node = node_id;
> > -    } else if (cs->numa_node != node_id) {
> > -            error_setg(errp, "node-id %d must match numa node specified"
> > -                "with -numa option for cpu-index %d",
> > -                cs->numa_node, cs->cpu_index);
> > -            return;
> > -    }
> > +    numa_cpu_pre_plug(cpu_slot, dev, errp);
> >  }
> >  
> >  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 0980d73..c7fee8b 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2831,11 +2831,9 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
> >      Error *local_err = NULL;
> >      CPUCore *cc = CPU_CORE(dev);
> > -    sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
> >      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> >      const char *type = object_get_typename(OBJECT(dev));
> >      CPUArchId *core_slot;
> > -    int node_id;
> >      int index;
> >  
> >      if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
> > @@ -2870,20 +2868,7 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          goto out;
> >      }
> >  
> > -    node_id = core_slot->props.node_id;
> > -    if (!core_slot->props.has_node_id) {
> > -        /* by default CPUState::numa_node was 0 if it's not set via CLI
> > -         * keep it this way for now but in future we probably should
> > -         * refuse to start up with incomplete numa mapping */
> > -        node_id = 0;
> > -    }
> > -    if (sc->node_id == CPU_UNSET_NUMA_NODE_ID) {
> > -        sc->node_id = node_id;
> > -    } else if (sc->node_id != node_id) {
> > -        error_setg(&local_err, "node-id %d must match numa node specified"
> > -            "with -numa option for cpu-index %d", sc->node_id, cc->core_id);
> > -        goto out;
> > -    }
> > +    numa_cpu_pre_plug(core_slot, dev, &local_err);
> >  
> >  out:
> >      g_free(base_core_type);
> > diff --git a/numa.c b/numa.c
> > index ca73145..0115bfd 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -534,6 +534,28 @@ void parse_numa_opts(MachineState *ms)
> >      }
> >  }
> >  
> > +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp)
> > +{
> > +    int node_id = object_property_get_int(OBJECT(dev), "node-id", errp);  
> 
> You don't check for errors here. If they will never happen,
> should we use &error_abort instead?
sure, I'll use &error_abort in v2

> > +
> > +    if (node_id == CPU_UNSET_NUMA_NODE_ID) {
> > +        /* by default CPUState::numa_node was 0 if it wasn't set explicitly
> > +         * TODO: make it error when incomplete numa mapping support is removed
> > +         */
> > +        node_id = 0;
> > +
> > +        /* due to bug in libvirt, it doesn't pass node-id from props on
> > +         * device_add as expected, so we have to fix it up here */
> > +        if (slot->props.has_node_id) {
> > +            node_id = slot->props.node_id;
> > +        }
> > +        object_property_set_int(OBJECT(dev), node_id, "node-id", errp);
> > +    } else if (node_id != slot->props.node_id) {
> > +        error_setg(errp, "node-id=%d must match numa node specified "
> > +                   "with -numa option", node_id);  
> 
> There's less detail on the error message, now. Probably harmless,
> but I would like to understand when exactly this can be
> triggered: is device_add the only way to trigger this error
> message?
error is triggered only during -device/device_add so there were no
need in more verbose error as device_add will report its arguments
(affected cpu in this case)


> 
> > +    }
> > +}
> > +
> >  static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
> >                                             const char *name,
> >                                             uint64_t ram_size)
> > -- 
> > 2.7.4
> >   
> 

Re: [Qemu-devel] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr

[Eduardo Habkost]

On Mon, May 22, 2017 at 08:39:31AM +0200, Igor Mammedov wrote:
> On Thu, 18 May 2017 15:19:13 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  include/sysemu/numa.h |  1 +
> > >  hw/arm/virt.c         | 16 ++--------------
> > >  hw/i386/pc.c          | 17 +----------------
> > >  hw/ppc/spapr.c        | 17 +----------------
> > >  numa.c                | 22 ++++++++++++++++++++++
> > >  5 files changed, 27 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > > index 7ffde5b..610eece 100644
> > > --- a/include/sysemu/numa.h
> > > +++ b/include/sysemu/numa.h
> > > @@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> > >                                   int nb_nodes, ram_addr_t size);
> > >  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> > >                                    int nb_nodes, ram_addr_t size);
> > > +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);  
> > 
> > I understand an explicitly call to numa_cpu_pre_plug() is needed
> > on spapr_core_pre_plug() because it is not handling a TYPE_CPU
> > object. But why not adding a numa_cpu_pre_plug() call to
> > cpu_common_realizefn(), so the explicit calls in machvirt_init()
> > and pc_cpu_pre_plug() are not necessary?
> 1. of the reasons is not to pollute all cpus with numa code

I understand this goal...

> 
> > Adding the code to cpu_common_realizefn() would also ensure
> > CPUState::node_id is set consistently, even if hotplug was not
> > done at thread level.
> 2. not all CPUs have node-id property

...and this. But: we already have the CPUState::numa_node field.
If we don't handle it in common code, we risk leaving the field
uninitialized, which is a problem if other code tries to use the
field for something.

Maybe that's an argument for removing the CPUState::numa_node
field too.


> 3. call site of thread_realize() in encapsulating object (spapr_core)
>    might be somewhere in the middle of parent's realize and likely
>    failure would need proper parent state rollback/cleanup.

I don't see why this could be a problem, if the code setting
realized=true is already supposed to handle errors on the realize
method.

> 4. and finely it's not cpu's responsibility to assign/check
>    node-id property. It's machine's job that owns/manages topology
>    layout. It' the same like with socket/core/thread properties.
>    So for the sake of small optimization, I'm not really willing
>    to violate that.

I don't disagree with that, but in that case I would like to
remove the CPUState::numa_node field soon, if possible.

[...]
> > > +    } else if (node_id != slot->props.node_id) {
> > > +        error_setg(errp, "node-id=%d must match numa node specified "
> > > +                   "with -numa option", node_id);  
> > 
> > There's less detail on the error message, now. Probably harmless,
> > but I would like to understand when exactly this can be
> > triggered: is device_add the only way to trigger this error
> > message?
> error is triggered only during -device/device_add so there were no
> need in more verbose error as device_add will report its arguments
> (affected cpu in this case)

OK.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr

[Igor Mammedov]

On Mon, 22 May 2017 09:58:45 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, May 22, 2017 at 08:39:31AM +0200, Igor Mammedov wrote:
> > On Thu, 18 May 2017 15:19:13 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote:  
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >  include/sysemu/numa.h |  1 +
> > > >  hw/arm/virt.c         | 16 ++--------------
> > > >  hw/i386/pc.c          | 17 +----------------
> > > >  hw/ppc/spapr.c        | 17 +----------------
> > > >  numa.c                | 22 ++++++++++++++++++++++
> > > >  5 files changed, 27 insertions(+), 46 deletions(-)
> > > > 
> > > > diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> > > > index 7ffde5b..610eece 100644
> > > > --- a/include/sysemu/numa.h
> > > > +++ b/include/sysemu/numa.h
> > > > @@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> > > >                                   int nb_nodes, ram_addr_t size);
> > > >  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
> > > >                                    int nb_nodes, ram_addr_t size);
> > > > +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);    
> > > 
> > > I understand an explicitly call to numa_cpu_pre_plug() is needed
> > > on spapr_core_pre_plug() because it is not handling a TYPE_CPU
> > > object. But why not adding a numa_cpu_pre_plug() call to
> > > cpu_common_realizefn(), so the explicit calls in machvirt_init()
> > > and pc_cpu_pre_plug() are not necessary?  
> > 1. of the reasons is not to pollute all cpus with numa code  
> 
> I understand this goal...
> 
> >   
> > > Adding the code to cpu_common_realizefn() would also ensure
> > > CPUState::node_id is set consistently, even if hotplug was not
> > > done at thread level.  
> > 2. not all CPUs have node-id property  
> 
> ...and this. But: we already have the CPUState::numa_node field.
> If we don't handle it in common code, we risk leaving the field
> uninitialized, which is a problem if other code tries to use the
> field for something.
> 
> Maybe that's an argument for removing the CPUState::numa_node
> field too.
> 
> 
> > 3. call site of thread_realize() in encapsulating object (spapr_core)
> >    might be somewhere in the middle of parent's realize and likely
> >    failure would need proper parent state rollback/cleanup.  
> 
> I don't see why this could be a problem, if the code setting
> realized=true is already supposed to handle errors on the realize
> method.
> 
> > 4. and finely it's not cpu's responsibility to assign/check
> >    node-id property. It's machine's job that owns/manages topology
> >    layout. It' the same like with socket/core/thread properties.
> >    So for the sake of small optimization, I'm not really willing
> >    to violate that.  
> 
> I don't disagree with that, but in that case I would like to
> remove the CPUState::numa_node field soon, if possible.
ok, I'll try add a patch on respin to do it (I think that I've
tried this at some RFC time)

Re: [Qemu-devel] [PATCH 1/3] numa: consolidate cpu_preplug fixups/checks for pc/arm/spapr

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7007 bytes --]

On Thu, May 18, 2017 at 10:09:29AM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  include/sysemu/numa.h |  1 +
>  hw/arm/virt.c         | 16 ++--------------
>  hw/i386/pc.c          | 17 +----------------
>  hw/ppc/spapr.c        | 17 +----------------
>  numa.c                | 22 ++++++++++++++++++++++
>  5 files changed, 27 insertions(+), 46 deletions(-)
> 
> diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
> index 7ffde5b..610eece 100644
> --- a/include/sysemu/numa.h
> +++ b/include/sysemu/numa.h
> @@ -35,4 +35,5 @@ void numa_legacy_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>                                   int nb_nodes, ram_addr_t size);
>  void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>                                    int nb_nodes, ram_addr_t size);
> +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp);
>  #endif
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index c7c8159..ce676df 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1351,7 +1351,6 @@ static void machvirt_init(MachineState *machine)
>      for (n = 0; n < possible_cpus->len; n++) {
>          Object *cpuobj;
>          CPUState *cs;
> -        int node_id;
>  
>          if (n >= smp_cpus) {
>              break;
> @@ -1364,19 +1363,8 @@ static void machvirt_init(MachineState *machine)
>          cs = CPU(cpuobj);
>          cs->cpu_index = n;
>  
> -        node_id = possible_cpus->cpus[cs->cpu_index].props.node_id;
> -        if (!possible_cpus->cpus[cs->cpu_index].props.has_node_id) {
> -            /* by default CPUState::numa_node was 0 if it's not set via CLI
> -             * keep it this way for now but in future we probably should
> -             * refuse to start up with incomplete numa mapping */
> -             node_id = 0;
> -        }
> -        if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> -            cs->numa_node = node_id;
> -        } else {
> -            /* CPU isn't device_add compatible yet, this shouldn't happen */
> -            error_setg(&error_abort, "user set node-id not implemented");
> -        }
> +        numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
> +                          &error_fatal);
>  
>          if (!vms->secure) {
>              object_property_set_bool(cpuobj, false, "has_el3", NULL);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index e36a375..d83c158 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1895,7 +1895,6 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>                              DeviceState *dev, Error **errp)
>  {
>      int idx;
> -    int node_id;
>      CPUState *cs;
>      CPUArchId *cpu_slot;
>      X86CPUTopoInfo topo;
> @@ -1986,21 +1985,7 @@ static void pc_cpu_pre_plug(HotplugHandler *hotplug_dev,
>      cs = CPU(cpu);
>      cs->cpu_index = idx;
>  
> -    node_id = cpu_slot->props.node_id;
> -    if (!cpu_slot->props.has_node_id) {
> -        /* by default CPUState::numa_node was 0 if it's not set via CLI
> -         * keep it this way for now but in future we probably should
> -         * refuse to start up with incomplete numa mapping */
> -        node_id = 0;
> -    }
> -    if (cs->numa_node == CPU_UNSET_NUMA_NODE_ID) {
> -        cs->numa_node = node_id;
> -    } else if (cs->numa_node != node_id) {
> -            error_setg(errp, "node-id %d must match numa node specified"
> -                "with -numa option for cpu-index %d",
> -                cs->numa_node, cs->cpu_index);
> -            return;
> -    }
> +    numa_cpu_pre_plug(cpu_slot, dev, errp);
>  }
>  
>  static void pc_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0980d73..c7fee8b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2831,11 +2831,9 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      MachineClass *mc = MACHINE_GET_CLASS(hotplug_dev);
>      Error *local_err = NULL;
>      CPUCore *cc = CPU_CORE(dev);
> -    sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
>      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
>      const char *type = object_get_typename(OBJECT(dev));
>      CPUArchId *core_slot;
> -    int node_id;
>      int index;
>  
>      if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
> @@ -2870,20 +2868,7 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          goto out;
>      }
>  
> -    node_id = core_slot->props.node_id;
> -    if (!core_slot->props.has_node_id) {
> -        /* by default CPUState::numa_node was 0 if it's not set via CLI
> -         * keep it this way for now but in future we probably should
> -         * refuse to start up with incomplete numa mapping */
> -        node_id = 0;
> -    }
> -    if (sc->node_id == CPU_UNSET_NUMA_NODE_ID) {
> -        sc->node_id = node_id;
> -    } else if (sc->node_id != node_id) {
> -        error_setg(&local_err, "node-id %d must match numa node specified"
> -            "with -numa option for cpu-index %d", sc->node_id, cc->core_id);
> -        goto out;
> -    }
> +    numa_cpu_pre_plug(core_slot, dev, &local_err);
>  
>  out:
>      g_free(base_core_type);
> diff --git a/numa.c b/numa.c
> index ca73145..0115bfd 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -534,6 +534,28 @@ void parse_numa_opts(MachineState *ms)
>      }
>  }
>  
> +void numa_cpu_pre_plug(const CPUArchId *slot, DeviceState *dev, Error **errp)
> +{
> +    int node_id = object_property_get_int(OBJECT(dev), "node-id", errp);
> +
> +    if (node_id == CPU_UNSET_NUMA_NODE_ID) {
> +        /* by default CPUState::numa_node was 0 if it wasn't set explicitly
> +         * TODO: make it error when incomplete numa mapping support is removed
> +         */
> +        node_id = 0;
> +
> +        /* due to bug in libvirt, it doesn't pass node-id from props on
> +         * device_add as expected, so we have to fix it up here */
> +        if (slot->props.has_node_id) {
> +            node_id = slot->props.node_id;
> +        }
> +        object_property_set_int(OBJECT(dev), node_id, "node-id", errp);
> +    } else if (node_id != slot->props.node_id) {
> +        error_setg(errp, "node-id=%d must match numa node specified "
> +                   "with -numa option", node_id);
> +    }
> +}
> +
>  static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
>                                             const char *name,
>                                             uint64_t ram_size)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH 2/3] numa: move default mapping init to machine

[Igor Mammedov]

there is no need use cpu_index_to_instance_props() for setting
default cpu -> node mapping. Generic machine code can do it
without cpu_index by just enabling already preset defaults
in possible_cpus.

PS:
as bonus it makes one less user of cpu_index_to_instance_props()

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/machine.c | 32 +++++++++++++++++++++-----------
 numa.c            | 26 --------------------------
 2 files changed, 21 insertions(+), 37 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index fd6a436..2e91aa9 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -700,26 +700,36 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
     return g_string_free(s, false);
 }
 
-static void machine_numa_validate(MachineState *machine)
+static void machine_numa_finish_init(MachineState *machine)
 {
-    int i;
+    int i, default_mapping;
     GString *s = g_string_new(NULL);
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(machine);
 
     assert(nb_numa_nodes);
+    for (i = possible_cpus->len;
+         i && !possible_cpus->cpus[i - 1].props.has_node_id;
+         i--)
+        ;;
+    default_mapping = !i; /* i == 0 : no explicit mapping provided by user */
+
     for (i = 0; i < possible_cpus->len; i++) {
         const CPUArchId *cpu_slot = &possible_cpus->cpus[i];
 
-        /* at this point numa mappings are initilized by CLI options
-         * or with default mappings so it's sufficient to list
-         * all not yet mapped CPUs here */
-        /* TODO: make it hard error in future */
         if (!cpu_slot->props.has_node_id) {
-            char *cpu_str = cpu_slot_to_string(cpu_slot);
-            g_string_append_printf(s, "%sCPU %d [%s]", s->len ? ", " : "", i,
-                                   cpu_str);
-            g_free(cpu_str);
+            if (default_mapping) {
+                /* fetch default mapping from board and enable it */
+                CpuInstanceProperties props = cpu_slot->props;
+                props.has_node_id = true;
+                machine_set_cpu_numa_node(machine, &props, &error_fatal);
+            } else {
+                /* record slots with not set mapping */
+                char *cpu_str = cpu_slot_to_string(cpu_slot);
+                g_string_append_printf(s, "%sCPU %d [%s]",
+                                       s->len ? ", " : "", i, cpu_str);
+                g_free(cpu_str);
+            }
         }
     }
     if (s->len) {
@@ -737,7 +747,7 @@ void machine_run_board_init(MachineState *machine)
     MachineClass *machine_class = MACHINE_GET_CLASS(machine);
 
     if (nb_numa_nodes) {
-        machine_numa_validate(machine);
+        machine_numa_finish_init(machine);
     }
     machine_class->init(machine);
 }
diff --git a/numa.c b/numa.c
index 0115bfd..796cd7d 100644
--- a/numa.c
+++ b/numa.c
@@ -427,7 +427,6 @@ void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
 void parse_numa_opts(MachineState *ms)
 {
     int i;
-    const CPUArchIdList *possible_cpus;
     MachineClass *mc = MACHINE_GET_CLASS(ms);
 
     if (qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, ms, NULL)) {
@@ -485,31 +484,6 @@ void parse_numa_opts(MachineState *ms)
 
         numa_set_mem_ranges();
 
-        /* assign CPUs to nodes using board provided default mapping */
-        if (!mc->cpu_index_to_instance_props || !mc->possible_cpu_arch_ids) {
-            error_report("default CPUs to NUMA node mapping isn't supported");
-            exit(1);
-        }
-
-        possible_cpus = mc->possible_cpu_arch_ids(ms);
-        for (i = 0; i < possible_cpus->len; i++) {
-            if (possible_cpus->cpus[i].props.has_node_id) {
-                break;
-            }
-        }
-
-        /* no CPUs are assigned to NUMA nodes */
-        if (i == possible_cpus->len) {
-            for (i = 0; i < max_cpus; i++) {
-                CpuInstanceProperties props;
-                /* fetch default mapping from board and enable it */
-                props = mc->cpu_index_to_instance_props(ms, i);
-                props.has_node_id = true;
-
-                machine_set_cpu_numa_node(ms, &props, &error_fatal);
-            }
-        }
-
         /* QEMU needs at least all unique node pair distances to build
          * the whole NUMA distance table. QEMU treats the distance table
          * as symmetric by default, i.e. distance A->B == distance B->A.
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] numa: move default mapping init to machine

[Eduardo Habkost]

On Thu, May 18, 2017 at 10:09:30AM +0200, Igor Mammedov wrote:
> there is no need use cpu_index_to_instance_props() for setting
> default cpu -> node mapping. Generic machine code can do it
> without cpu_index by just enabling already preset defaults
> in possible_cpus.
> 
> PS:
> as bonus it makes one less user of cpu_index_to_instance_props()
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/core/machine.c | 32 +++++++++++++++++++++-----------
>  numa.c            | 26 --------------------------
>  2 files changed, 21 insertions(+), 37 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index fd6a436..2e91aa9 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -700,26 +700,36 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
>      return g_string_free(s, false);
>  }
>  
> -static void machine_numa_validate(MachineState *machine)
> +static void machine_numa_finish_init(MachineState *machine)
>  {
> -    int i;
> +    int i, default_mapping;

I suggest bool instead of int.

>      GString *s = g_string_new(NULL);
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(machine);
>  
>      assert(nb_numa_nodes);
> +    for (i = possible_cpus->len;
> +         i && !possible_cpus->cpus[i - 1].props.has_node_id;
> +         i--)
> +        ;;

I believe the original code was more readable, and it had only 1
more line than this version. i.e.:

    for (i = 0; i < possible_cpus->len; i++) {
        if (possible_cpus->cpus[i].props.has_node_id) {
            break;
        }
    }
    default_mapping = (i == possible_cpus->len);

> +    default_mapping = !i; /* i == 0 : no explicit mapping provided by user */
> +
>      for (i = 0; i < possible_cpus->len; i++) {
>          const CPUArchId *cpu_slot = &possible_cpus->cpus[i];
>  
> -        /* at this point numa mappings are initilized by CLI options
> -         * or with default mappings so it's sufficient to list
> -         * all not yet mapped CPUs here */
> -        /* TODO: make it hard error in future */

Did we change our mind about making it a hard error in the
future?

>          if (!cpu_slot->props.has_node_id) {
> -            char *cpu_str = cpu_slot_to_string(cpu_slot);
> -            g_string_append_printf(s, "%sCPU %d [%s]", s->len ? ", " : "", i,
> -                                   cpu_str);
> -            g_free(cpu_str);
> +            if (default_mapping) {
> +                /* fetch default mapping from board and enable it */
> +                CpuInstanceProperties props = cpu_slot->props;
> +                props.has_node_id = true;
> +                machine_set_cpu_numa_node(machine, &props, &error_fatal);

Is a machine_set_cpu_numa_node() call really necessary here, if
we are already looking at cpu_slot->props directly?

> +            } else {
> +                /* record slots with not set mapping */
> +                char *cpu_str = cpu_slot_to_string(cpu_slot);
> +                g_string_append_printf(s, "%sCPU %d [%s]",
> +                                       s->len ? ", " : "", i, cpu_str);
> +                g_free(cpu_str);
> +            }
>          }

What about doing this instead:

        if (default_mapping) {
            /*
             * Default mapping was already set by board at
             * cpu_slot->props.node_id, just enable it
             */
            cpu_slot->props.has_node_id = true;
        } else if (!cpu_slot->props.has_node_id) {
            char *cpu_str = cpu_slot_to_string(cpu_slot);
            g_string_append_printf(s, "%sCPU %d [%s]", s->len ? ", " : "", i,
                                   cpu_str);
            g_free(cpu_str);
        }

>      }
>      if (s->len) {
> @@ -737,7 +747,7 @@ void machine_run_board_init(MachineState *machine)
>      MachineClass *machine_class = MACHINE_GET_CLASS(machine);
>  
>      if (nb_numa_nodes) {
> -        machine_numa_validate(machine);
> +        machine_numa_finish_init(machine);
>      }
>      machine_class->init(machine);
>  }
> diff --git a/numa.c b/numa.c
> index 0115bfd..796cd7d 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -427,7 +427,6 @@ void numa_default_auto_assign_ram(MachineClass *mc, NodeInfo *nodes,
>  void parse_numa_opts(MachineState *ms)
>  {
>      int i;
> -    const CPUArchIdList *possible_cpus;
>      MachineClass *mc = MACHINE_GET_CLASS(ms);
>  
>      if (qemu_opts_foreach(qemu_find_opts("numa"), parse_numa, ms, NULL)) {
> @@ -485,31 +484,6 @@ void parse_numa_opts(MachineState *ms)
>  
>          numa_set_mem_ranges();
>  
> -        /* assign CPUs to nodes using board provided default mapping */
> -        if (!mc->cpu_index_to_instance_props || !mc->possible_cpu_arch_ids) {
> -            error_report("default CPUs to NUMA node mapping isn't supported");
> -            exit(1);
> -        }
> -
> -        possible_cpus = mc->possible_cpu_arch_ids(ms);
> -        for (i = 0; i < possible_cpus->len; i++) {
> -            if (possible_cpus->cpus[i].props.has_node_id) {
> -                break;
> -            }
> -        }
> -
> -        /* no CPUs are assigned to NUMA nodes */
> -        if (i == possible_cpus->len) {
> -            for (i = 0; i < max_cpus; i++) {
> -                CpuInstanceProperties props;
> -                /* fetch default mapping from board and enable it */
> -                props = mc->cpu_index_to_instance_props(ms, i);
> -                props.has_node_id = true;
> -
> -                machine_set_cpu_numa_node(ms, &props, &error_fatal);
> -            }
> -        }
> -
>          /* QEMU needs at least all unique node pair distances to build
>           * the whole NUMA distance table. QEMU treats the distance table
>           * as symmetric by default, i.e. distance A->B == distance B->A.
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/3] numa: move default mapping init to machine

[Igor Mammedov]

On Thu, 18 May 2017 15:50:58 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, May 18, 2017 at 10:09:30AM +0200, Igor Mammedov wrote:
> > there is no need use cpu_index_to_instance_props() for setting
> > default cpu -> node mapping. Generic machine code can do it
> > without cpu_index by just enabling already preset defaults
> > in possible_cpus.
> > 
> > PS:
> > as bonus it makes one less user of cpu_index_to_instance_props()
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/core/machine.c | 32 +++++++++++++++++++++-----------
> >  numa.c            | 26 --------------------------
> >  2 files changed, 21 insertions(+), 37 deletions(-)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index fd6a436..2e91aa9 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -700,26 +700,36 @@ static char *cpu_slot_to_string(const CPUArchId *cpu)
> >      return g_string_free(s, false);
> >  }
> >  
> > -static void machine_numa_validate(MachineState *machine)
> > +static void machine_numa_finish_init(MachineState *machine)
> >  {
> > -    int i;
> > +    int i, default_mapping;  
> 
> I suggest bool instead of int.
will do it in v2

> 
> >      GString *s = g_string_new(NULL);
> >      MachineClass *mc = MACHINE_GET_CLASS(machine);
> >      const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(machine);
> >  
> >      assert(nb_numa_nodes);
> > +    for (i = possible_cpus->len;
> > +         i && !possible_cpus->cpus[i - 1].props.has_node_id;
> > +         i--)
> > +        ;;  
> 
> I believe the original code was more readable, and it had only 1
> more line than this version. i.e.:
> 
>     for (i = 0; i < possible_cpus->len; i++) {
>         if (possible_cpus->cpus[i].props.has_node_id) {
>             break;
>         }
>     }
>     default_mapping = (i == possible_cpus->len);
ok, I'll do it this way

> 
> > +    default_mapping = !i; /* i == 0 : no explicit mapping provided by user */
> > +
> >      for (i = 0; i < possible_cpus->len; i++) {
> >          const CPUArchId *cpu_slot = &possible_cpus->cpus[i];
> >  
> > -        /* at this point numa mappings are initilized by CLI options
> > -         * or with default mappings so it's sufficient to list
> > -         * all not yet mapped CPUs here */
> > -        /* TODO: make it hard error in future */  
> 
> Did we change our mind about making it a hard error in the
> future?
nope, error message at the end of function says that partial mapping
is obsoleted and will be removed. I've thought that's was sufficient
reminder for us of what needs to be removed.
I'll move TODO to:

+            } else {
+                /* record slots with not set mapping */
++               /* TODO: make it hard error in future */ 
+                char *cpu_str = cpu_slot_to_string(cpu_slot);
+                g_string_append_printf(s, "%sCPU %d [%s]",

> 
> >          if (!cpu_slot->props.has_node_id) {
> > -            char *cpu_str = cpu_slot_to_string(cpu_slot);
> > -            g_string_append_printf(s, "%sCPU %d [%s]", s->len ? ", " : "", i,
> > -                                   cpu_str);
> > -            g_free(cpu_str);
> > +            if (default_mapping) {
> > +                /* fetch default mapping from board and enable it */
> > +                CpuInstanceProperties props = cpu_slot->props;
> > +                props.has_node_id = true;
> > +                machine_set_cpu_numa_node(machine, &props, &error_fatal);  
> 
> Is a machine_set_cpu_numa_node() call really necessary here, if
> we are already looking at cpu_slot->props directly?
yes, it's necessary as cpu_slot comes from:
   const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(machine);
callback, which initializes machine::possible_cpus if necessary.

So cpu_slot is readonly and I'd prefer to keep mc->possible_cpu_arch_ids()
returning 'const' as it's used by external callers and they shouldn't
mess with possible_cpus content by accident.

usage of machine_set_cpu_numa_node() adds +2 more lines and it's
proper/validating setter for node_id and will catch
mistakes if we make them in the code.
So I wouldn't use shortcuts here to save 2 lines.

 
> > +            } else {
> > +                /* record slots with not set mapping */
> > +                char *cpu_str = cpu_slot_to_string(cpu_slot);
> > +                g_string_append_printf(s, "%sCPU %d [%s]",
> > +                                       s->len ? ", " : "", i, cpu_str);
> > +                g_free(cpu_str);
> > +            }
> >          }  
> 
> What about doing this instead:
> 
>         if (default_mapping) {
>             /*
>              * Default mapping was already set by board at
>              * cpu_slot->props.node_id, just enable it
>              */
>             cpu_slot->props.has_node_id = true;
>         } else if (!cpu_slot->props.has_node_id) {
>             char *cpu_str = cpu_slot_to_string(cpu_slot);
>             g_string_append_printf(s, "%sCPU %d [%s]", s->len ? ", " : "", i,
>                                    cpu_str);
>             g_free(cpu_str);
>         }

Re: [Qemu-devel] [PATCH 2/3] numa: move default mapping init to machine

[Eduardo Habkost]

On Mon, May 22, 2017 at 09:04:03AM +0200, Igor Mammedov wrote:
> On Thu, 18 May 2017 15:50:58 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Thu, May 18, 2017 at 10:09:30AM +0200, Igor Mammedov wrote:
[...]
> > 
> > > +    default_mapping = !i; /* i == 0 : no explicit mapping provided by user */
> > > +
> > >      for (i = 0; i < possible_cpus->len; i++) {
> > >          const CPUArchId *cpu_slot = &possible_cpus->cpus[i];
> > >  
> > > -        /* at this point numa mappings are initilized by CLI options
> > > -         * or with default mappings so it's sufficient to list
> > > -         * all not yet mapped CPUs here */
> > > -        /* TODO: make it hard error in future */  
> > 
> > Did we change our mind about making it a hard error in the
> > future?
> nope, error message at the end of function says that partial mapping
> is obsoleted and will be removed. I've thought that's was sufficient
> reminder for us of what needs to be removed.

Yes, it is sufficient. I just didn't notice the message. :)

> I'll move TODO to:
> 
> +            } else {
> +                /* record slots with not set mapping */
> ++               /* TODO: make it hard error in future */ 
> +                char *cpu_str = cpu_slot_to_string(cpu_slot);
> +                g_string_append_printf(s, "%sCPU %d [%s]",

I don't mind removing the TODO comment, so it's up to you.

> 
> > 
> > >          if (!cpu_slot->props.has_node_id) {
> > > -            char *cpu_str = cpu_slot_to_string(cpu_slot);
> > > -            g_string_append_printf(s, "%sCPU %d [%s]", s->len ? ", " : "", i,
> > > -                                   cpu_str);
> > > -            g_free(cpu_str);
> > > +            if (default_mapping) {
> > > +                /* fetch default mapping from board and enable it */
> > > +                CpuInstanceProperties props = cpu_slot->props;
> > > +                props.has_node_id = true;
> > > +                machine_set_cpu_numa_node(machine, &props, &error_fatal);  
> > 
> > Is a machine_set_cpu_numa_node() call really necessary here, if
> > we are already looking at cpu_slot->props directly?
> yes, it's necessary as cpu_slot comes from:
>    const CPUArchIdList *possible_cpus = mc->possible_cpu_arch_ids(machine);
> callback, which initializes machine::possible_cpus if necessary.
> 
> So cpu_slot is readonly and I'd prefer to keep mc->possible_cpu_arch_ids()
> returning 'const' as it's used by external callers and they shouldn't
> mess with possible_cpus content by accident.
> 
> usage of machine_set_cpu_numa_node() adds +2 more lines and it's
> proper/validating setter for node_id and will catch
> mistakes if we make them in the code.
> So I wouldn't use shortcuts here to save 2 lines.

Oh, I didn't notice cpu_slot was const. That's OK to me.

> 
>  
> > > +            } else {
> > > +                /* record slots with not set mapping */
> > > +                char *cpu_str = cpu_slot_to_string(cpu_slot);
> > > +                g_string_append_printf(s, "%sCPU %d [%s]",
> > > +                                       s->len ? ", " : "", i, cpu_str);
> > > +                g_free(cpu_str);
> > > +            }
> > >          }  
> > 
> > What about doing this instead:
> > 
> >         if (default_mapping) {
> >             /*
> >              * Default mapping was already set by board at
> >              * cpu_slot->props.node_id, just enable it
> >              */
> >             cpu_slot->props.has_node_id = true;
> >         } else if (!cpu_slot->props.has_node_id) {
> >             char *cpu_str = cpu_slot_to_string(cpu_slot);
> >             g_string_append_printf(s, "%sCPU %d [%s]", s->len ? ", " : "", i,
> >                                    cpu_str);
> >             g_free(cpu_str);
> >         }

-- 
Eduardo

[Qemu-devel] [PATCH 3/3] numa: silence incomplete mapping warning under qtest

[Igor Mammedov]

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
CC: Markus Armbruster <armbru@redhat.com>

---
 hw/core/machine.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2e91aa9..21ebef8 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -21,6 +21,7 @@
 #include "qemu/error-report.h"
 #include "qemu/cutils.h"
 #include "sysemu/numa.h"
+#include "sysemu/qtest.h"
 
 static char *machine_get_accel(Object *obj, Error **errp)
 {
@@ -732,7 +733,7 @@ static void machine_numa_finish_init(MachineState *machine)
             }
         }
     }
-    if (s->len) {
+    if (s->len && !qtest_enabled()) {
         error_report("warning: CPU(s) not present in any NUMA nodes: %s",
                      s->str);
         error_report("warning: All CPU(s) up to maxcpus should be described "
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/3] numa: silence incomplete mapping warning under qtest

[Eduardo Habkost]

On Thu, May 18, 2017 at 10:09:31AM +0200, Igor Mammedov wrote:
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Where exactly is the test code that triggers those messages and
requires this patch? I would like to document that in the commit
message.

> ---
> CC: Markus Armbruster <armbru@redhat.com>
> 
> ---
>  hw/core/machine.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2e91aa9..21ebef8 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -21,6 +21,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/cutils.h"
>  #include "sysemu/numa.h"
> +#include "sysemu/qtest.h"
>  
>  static char *machine_get_accel(Object *obj, Error **errp)
>  {
> @@ -732,7 +733,7 @@ static void machine_numa_finish_init(MachineState *machine)
>              }
>          }
>      }
> -    if (s->len) {
> +    if (s->len && !qtest_enabled()) {
>          error_report("warning: CPU(s) not present in any NUMA nodes: %s",
>                       s->str);
>          error_report("warning: All CPU(s) up to maxcpus should be described "
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/3] numa: silence incomplete mapping warning under qtest

[Stefan Hajnoczi]

On Thu, May 18, 2017 at 7:20 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, May 18, 2017 at 10:09:31AM +0200, Igor Mammedov wrote:
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>
> Where exactly is the test code that triggers those messages and
> requires this patch? I would like to document that in the commit
> message.

$ make V=1 check
TEST: tests/numa-test... (pid=30376)
  /x86_64/numa/mon/default:                                            OK
  /x86_64/numa/mon/cpus/explicit:                                      OK
  /x86_64/numa/mon/cpus/partial:
qemu-system-x86_64: warning: CPU(s) not present in any NUMA nodes: CPU
2 [socket-id: 2, core-id: 0, thread-id: 0], CPU 3 [socket-id: 3,
core-id: 0, thread-id: 0], CPU 6 [socket-id: 6, core-id: 0, thread-id:
0], CPU 7 [socket-id: 7, core-id: 0, thread-id: 0]
qemu-system-x86_64: warning: All CPU(s) up to maxcpus should be
described in NUMA config, ability to start up with partial NUMA
mappings is obsoleted and will be removed in future
OK

Re: [Qemu-devel] [PATCH 3/3] numa: silence incomplete mapping warning under qtest

[Igor Mammedov]

On Thu, 18 May 2017 15:20:39 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, May 18, 2017 at 10:09:31AM +0200, Igor Mammedov wrote:
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> 
> Where exactly is the test code that triggers those messages and
> requires this patch? I would like to document that in the commit
> message.

ok, I'll  mention test case in commit message on respin

> > ---
> > CC: Markus Armbruster <armbru@redhat.com>
> > 
> > ---
> >  hw/core/machine.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index 2e91aa9..21ebef8 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -21,6 +21,7 @@
> >  #include "qemu/error-report.h"
> >  #include "qemu/cutils.h"
> >  #include "sysemu/numa.h"
> > +#include "sysemu/qtest.h"
> >  
> >  static char *machine_get_accel(Object *obj, Error **errp)
> >  {
> > @@ -732,7 +733,7 @@ static void machine_numa_finish_init(MachineState *machine)
> >              }
> >          }
> >      }
> > -    if (s->len) {
> > +    if (s->len && !qtest_enabled()) {
> >          error_report("warning: CPU(s) not present in any NUMA nodes: %s",
> >                       s->str);
> >          error_report("warning: All CPU(s) up to maxcpus should be described "
> > -- 
> > 2.7.4
> >   
> 

Re: [Qemu-devel] [PATCH 3/3] numa: silence incomplete mapping warning under qtest

[Eduardo Habkost]

On Mon, May 22, 2017 at 09:58:36AM +0200, Igor Mammedov wrote:
> On Thu, 18 May 2017 15:20:39 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, May 18, 2017 at 10:09:31AM +0200, Igor Mammedov wrote:
> > > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>  
> > 
> > Where exactly is the test code that triggers those messages and
> > requires this patch? I would like to document that in the commit
> > message.
> 
> ok, I'll  mention test case in commit message on respin

I will apply this version, and add "Silence warnings triggered by
the /numa/mon/cpus/partial test case" to the commit
message.

I want to merge it as soon as possible, as the warnings are
confusing people.

-- 
Eduardo