1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH] spapr: ensure that all threads within core are on the same NUMA node

Igor Mammedov posted 1 patch 8 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1487928416-131310-1-git-send-email-imammedo@redhat.com
Test checkpatch passed
Test docker passed
Test s390x passed
hw/ppc/spapr_cpu_core.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
[Qemu-devel] [PATCH] spapr: ensure that all threads within core are on the same NUMA node
Posted by Igor Mammedov 8 years, 8 months ago 
Threads within a core shouldn't be on different
NUMA nodes, so if user has misconfgured command
line, fail QEMU at start up to force user fix it.

For now use the first thread on the core as source
of core's node-id. Later when cpu-numa refactoring
lands  it will be switched to core's node-id from
possible_cpus[].

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
Enforcement is necessary to ensure that it would be
possible to map legacy CPU index based CLI mapping
to core based one and replace numa_info[XXX].node_cpu
with possible_cpus[] as storage for mapping info.

CC: qemu-ppc@nongnu.org
CC: lvivier@redhat.com
CC: David Gibson <david@gibson.dropbear.id.au>
CC: thuth@redhat.com
CC: mdroth@linux.vnet.ibm.com
CC: agraf@suse.de
---
 hw/ppc/spapr_cpu_core.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 55cd045..1499a8b 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -50,8 +50,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
                            Error **errp)
 {
     CPUPPCState *env = &cpu->env;
-    CPUState *cs = CPU(cpu);
-    int i;
 
     /* Set time-base frequency to 512 MHz */
     cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
@@ -70,12 +68,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
         }
     }
 
-    /* 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;
-    }
-
     xics_cpu_setup(spapr->xics, cpu);
 
     qemu_register_reset(spapr_cpu_reset, cpu);
@@ -159,11 +151,13 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     const char *typename = object_class_get_name(scc->cpu_class);
     size_t size = object_type_get_instance_size(typename);
     Error *local_err = NULL;
+    int core_node_id = numa_get_node_for_cpu(cc->core_id);;
     void *obj;
     int i, j;
 
     sc->threads = g_malloc0(size * cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
+        int node_id;
         char id[32];
         CPUState *cs;
 
@@ -172,6 +166,19 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         object_initialize(obj, size, typename);
         cs = CPU(obj);
         cs->cpu_index = cc->core_id + i;
+
+        /* Set NUMA node for the added CPUs  */
+        node_id = numa_get_node_for_cpu(cs->cpu_index);
+        if (node_id != core_node_id) {
+            error_setg(&local_err, "Invalid node-id=%d of thread[cpu-index: %d]"
+                " on CPU[core-id: %d, node-id: %d], node-id must be the same",
+                 node_id, cs->cpu_index, cc->core_id, core_node_id);
+            goto err;
+        }
+        if (node_id < nb_numa_nodes) {
+            cs->numa_node = node_id;
+        }
+
         snprintf(id, sizeof(id), "thread[%d]", i);
         object_property_add_child(OBJECT(sc), id, obj, &local_err);
         if (local_err) {
-- 
2.7.4
Re: [Qemu-devel] [PATCH] spapr: ensure that all threads within core are on the same NUMA node
Posted by Igor Mammedov 8 years, 8 months ago 
On Fri, 24 Feb 2017 10:26:56 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> Threads within a core shouldn't be on different
> NUMA nodes, so if user has misconfgured command
> line, fail QEMU at start up to force user fix it.
> 
> For now use the first thread on the core as source
> of core's node-id. Later when cpu-numa refactoring
> lands  it will be switched to core's node-id from
> possible_cpus[].
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
ping,

David could you review/merge maybe adding to commit
message:

"
patch prevents the same issues as commit 20bb648d
fixes, only instead of default mapping it adds
check for manually configured NUMA node mapping
"

> ---
> Enforcement is necessary to ensure that it would be
> possible to map legacy CPU index based CLI mapping
> to core based one and replace numa_info[XXX].node_cpu
> with possible_cpus[] as storage for mapping info.
> 
> CC: qemu-ppc@nongnu.org
> CC: lvivier@redhat.com
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: thuth@redhat.com
> CC: mdroth@linux.vnet.ibm.com
> CC: agraf@suse.de
> ---
>  hw/ppc/spapr_cpu_core.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 55cd045..1499a8b 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -50,8 +50,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>                             Error **errp)
>  {
>      CPUPPCState *env = &cpu->env;
> -    CPUState *cs = CPU(cpu);
> -    int i;
>  
>      /* Set time-base frequency to 512 MHz */
>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> @@ -70,12 +68,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>          }
>      }
>  
> -    /* 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;
> -    }
> -
>      xics_cpu_setup(spapr->xics, cpu);
>  
>      qemu_register_reset(spapr_cpu_reset, cpu);
> @@ -159,11 +151,13 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      const char *typename = object_class_get_name(scc->cpu_class);
>      size_t size = object_type_get_instance_size(typename);
>      Error *local_err = NULL;
> +    int core_node_id = numa_get_node_for_cpu(cc->core_id);;
>      void *obj;
>      int i, j;
>  
>      sc->threads = g_malloc0(size * cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> +        int node_id;
>          char id[32];
>          CPUState *cs;
>  
> @@ -172,6 +166,19 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>          object_initialize(obj, size, typename);
>          cs = CPU(obj);
>          cs->cpu_index = cc->core_id + i;
> +
> +        /* Set NUMA node for the added CPUs  */
> +        node_id = numa_get_node_for_cpu(cs->cpu_index);
> +        if (node_id != core_node_id) {
> +            error_setg(&local_err, "Invalid node-id=%d of thread[cpu-index: %d]"
> +                " on CPU[core-id: %d, node-id: %d], node-id must be the same",
> +                 node_id, cs->cpu_index, cc->core_id, core_node_id);
> +            goto err;
> +        }
> +        if (node_id < nb_numa_nodes) {
> +            cs->numa_node = node_id;
> +        }
> +
>          snprintf(id, sizeof(id), "thread[%d]", i);
>          object_property_add_child(OBJECT(sc), id, obj, &local_err);
>          if (local_err) {
Re: [Qemu-devel] [PATCH] spapr: ensure that all threads within core are on the same NUMA node
Posted by David Gibson 8 years, 8 months ago 
On Fri, Mar 03, 2017 at 06:41:04PM +0100, Igor Mammedov wrote:
> On Fri, 24 Feb 2017 10:26:56 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > Threads within a core shouldn't be on different
> > NUMA nodes, so if user has misconfgured command
> > line, fail QEMU at start up to force user fix it.
> > 
> > For now use the first thread on the core as source
> > of core's node-id. Later when cpu-numa refactoring
> > lands  it will be switched to core's node-id from
> > possible_cpus[].
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ping,
> 
> David could you review/merge maybe adding to commit
> message:
> 
> "
> patch prevents the same issues as commit 20bb648d
> fixes, only instead of default mapping it adds
> check for manually configured NUMA node mapping
> "

Ok, merged.  I'll try to send that up today, just in time for the hard freeze.

> 
> > ---
> > Enforcement is necessary to ensure that it would be
> > possible to map legacy CPU index based CLI mapping
> > to core based one and replace numa_info[XXX].node_cpu
> > with possible_cpus[] as storage for mapping info.
> > 
> > CC: qemu-ppc@nongnu.org
> > CC: lvivier@redhat.com
> > CC: David Gibson <david@gibson.dropbear.id.au>
> > CC: thuth@redhat.com
> > CC: mdroth@linux.vnet.ibm.com
> > CC: agraf@suse.de
> > ---
> >  hw/ppc/spapr_cpu_core.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 55cd045..1499a8b 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -50,8 +50,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> >                             Error **errp)
> >  {
> >      CPUPPCState *env = &cpu->env;
> > -    CPUState *cs = CPU(cpu);
> > -    int i;
> >  
> >      /* Set time-base frequency to 512 MHz */
> >      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> > @@ -70,12 +68,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> >          }
> >      }
> >  
> > -    /* 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;
> > -    }
> > -
> >      xics_cpu_setup(spapr->xics, cpu);
> >  
> >      qemu_register_reset(spapr_cpu_reset, cpu);
> > @@ -159,11 +151,13 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      const char *typename = object_class_get_name(scc->cpu_class);
> >      size_t size = object_type_get_instance_size(typename);
> >      Error *local_err = NULL;
> > +    int core_node_id = numa_get_node_for_cpu(cc->core_id);;
> >      void *obj;
> >      int i, j;
> >  
> >      sc->threads = g_malloc0(size * cc->nr_threads);
> >      for (i = 0; i < cc->nr_threads; i++) {
> > +        int node_id;
> >          char id[32];
> >          CPUState *cs;
> >  
> > @@ -172,6 +166,19 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >          object_initialize(obj, size, typename);
> >          cs = CPU(obj);
> >          cs->cpu_index = cc->core_id + i;
> > +
> > +        /* Set NUMA node for the added CPUs  */
> > +        node_id = numa_get_node_for_cpu(cs->cpu_index);
> > +        if (node_id != core_node_id) {
> > +            error_setg(&local_err, "Invalid node-id=%d of thread[cpu-index: %d]"
> > +                " on CPU[core-id: %d, node-id: %d], node-id must be the same",
> > +                 node_id, cs->cpu_index, cc->core_id, core_node_id);
> > +            goto err;
> > +        }
> > +        if (node_id < nb_numa_nodes) {
> > +            cs->numa_node = node_id;
> > +        }
> > +
> >          snprintf(id, sizeof(id), "thread[%d]", i);
> >          object_property_add_child(OBJECT(sc), id, obj, &local_err);
> >          if (local_err) {
> 

-- 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.