[Qemu-devel] [PATCH v3 0/4] NUMA: Validate CPU configuration

[Qemu-devel] [PATCH v3 0/4] NUMA: Validate CPU configuration

[Eduardo Habkost]

This adds extra checks to the NUMA code to make sure the CPU configuration is
consistent. This needs to be applied on top of the following series:

  Message-Id: <1423421482-11619-1-git-send-email-ehabkost@redhat.com>
  Subject: [Qemu-devel] [PATCH 0/7] NUMA code cleanup
  From: Eduardo Habkost <ehabkost@redhat.com>
  Date: Sun,  8 Feb 2015 16:51:15 -0200
  Git tree: https://github.com/ehabkost/qemu.git numa-next

Changes v1 -> v2:
 * (none, v1 was tagged by accident and never sent to qemu-devel)

Changes v2 -> v3:
 * Fix off-by-one error on CPU index check
 * Use GString and error_report() instead of calling fprintf() directly
 * Simplify logic of the CPUs-not-present check

Eduardo Habkost (4):
  numa: Fix off-by-one error at MAX_CPUMASK_BITS check
  numa: Reject CPU indexes > max_cpus
  numa: Reject configuration if CPU appears on multiple nodes
  numa: Print warning if no node is assigned to a CPU

 numa.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 3 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v3 1/4] numa: Fix off-by-one error at MAX_CPUMASK_BITS check

[Eduardo Habkost]

Fix the CPU index check to ensure we don't go beyond the size of the
node_cpu bitmap.

CPU index is always less than MAX_CPUMASK_BITS, as documented at
sysemu.h:

> The following shall be true for all CPUs:
>   cpu->cpu_index < max_cpus <= MAX_CPUMASK_BITS

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 numa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/numa.c b/numa.c
index 0d15375..41e496b 100644
--- a/numa.c
+++ b/numa.c
@@ -76,9 +76,9 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
     }
 
     for (cpus = node->cpus; cpus; cpus = cpus->next) {
-        if (cpus->value > MAX_CPUMASK_BITS) {
+        if (cpus->value >= MAX_CPUMASK_BITS) {
             error_setg(errp, "CPU number %" PRIu16 " is bigger than %d",
-                       cpus->value, MAX_CPUMASK_BITS);
+                       cpus->value, MAX_CPUMASK_BITS - 1);
             return;
         }
         bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
-- 
2.1.0

[Qemu-devel] [PATCH v3 2/4] numa: Reject CPU indexes > max_cpus

[Eduardo Habkost]

CPU index is always less than max_cpus, as documented at sysemu.h:

> The following shall be true for all CPUs:
>   cpu->cpu_index < max_cpus <= MAX_CPUMASK_BITS

Reject configuration which uses invalid CPU indexes.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
v1 -> v2: (no changes)

v2 -> v3:
 * Check for (cpu >= max_cpus) instead of (cpu > max_cpus)
 * Reword error message as we are not checking for "bigger than maxcpus"
   CPU indexes
---
 numa.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/numa.c b/numa.c
index 41e496b..4139e46 100644
--- a/numa.c
+++ b/numa.c
@@ -76,9 +76,11 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
     }
 
     for (cpus = node->cpus; cpus; cpus = cpus->next) {
-        if (cpus->value >= MAX_CPUMASK_BITS) {
-            error_setg(errp, "CPU number %" PRIu16 " is bigger than %d",
-                       cpus->value, MAX_CPUMASK_BITS - 1);
+        if (cpus->value >= max_cpus) {
+            error_setg(errp,
+                       "CPU index (%" PRIu16 ")" \
+                       " should be smaller than maxcpus (%d)",
+                       cpus->value, max_cpus);
             return;
         }
         bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
-- 
2.1.0

[Qemu-devel] [PATCH v3 3/4] numa: Reject configuration if CPU appears on multiple nodes

[Eduardo Habkost]

Each CPU can appear in only one NUMA node on the NUMA config. Reject
configuration if a CPU appears in multiple nodes.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
v1 -> v2: (no changes)

v2 -> v3:
 * Rename present_cpus to seen_cpus, to make it less confusing
 * Use GString and error_report() instead of multiple fprintf() calls
---
 numa.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/numa.c b/numa.c
index 4139e46..712faff 100644
--- a/numa.c
+++ b/numa.c
@@ -168,6 +168,41 @@ error:
     return -1;
 }
 
+static char *enumerate_cpus(unsigned long *cpus, int max_cpus)
+{
+    int cpu;
+    bool first = true;
+    GString *s = g_string_new(NULL);
+
+    for (cpu = find_first_bit(cpus, max_cpus);
+        cpu < max_cpus;
+        cpu = find_next_bit(cpus, max_cpus, cpu + 1)) {
+        g_string_append_printf(s, "%s%d", first ? "" : " ", cpu);
+        first = false;
+    }
+    return g_string_free(s, FALSE);
+}
+
+static void validate_numa_cpus(void)
+{
+    int i;
+    DECLARE_BITMAP(seen_cpus, MAX_CPUMASK_BITS);
+
+    bitmap_zero(seen_cpus, MAX_CPUMASK_BITS);
+    for (i = 0; i < nb_numa_nodes; i++) {
+        if (bitmap_intersects(seen_cpus, numa_info[i].node_cpu,
+                              MAX_CPUMASK_BITS)) {
+            bitmap_and(seen_cpus, seen_cpus,
+                       numa_info[i].node_cpu, MAX_CPUMASK_BITS);
+            error_report("CPU(s) present in multiple NUMA nodes: %s",
+                         enumerate_cpus(seen_cpus, max_cpus));;
+            exit(1);
+        }
+        bitmap_or(seen_cpus, seen_cpus,
+                  numa_info[i].node_cpu, MAX_CPUMASK_BITS);
+    }
+}
+
 void parse_numa_opts(void)
 {
     int i;
@@ -245,6 +280,8 @@ void parse_numa_opts(void)
                 set_bit(i, numa_info[i % nb_numa_nodes].node_cpu);
             }
         }
+
+        validate_numa_cpus();
     }
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH v3 4/4] numa: Print warning if no node is assigned to a CPU

[Eduardo Habkost]

Instead of silently assigning CPU to node 0 when it is omitted from the
command-line, check if all CPUs up to max_cpus are present in the NUMA
configuration.

I am making this a warning and not a fatal error, to allow management
software to be updated if necessary.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
v1 -> v2: (no changes)

v2 -> v3:
 * Use enumerate_cpus() and error_report() for error message
 * Simplify logic using bitmap_full()
---
 numa.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/numa.c b/numa.c
index 712faff..4310bf9 100644
--- a/numa.c
+++ b/numa.c
@@ -201,6 +201,14 @@ static void validate_numa_cpus(void)
         bitmap_or(seen_cpus, seen_cpus,
                   numa_info[i].node_cpu, MAX_CPUMASK_BITS);
     }
+
+    if (!bitmap_full(seen_cpus, max_cpus)) {
+        char *msg;
+        bitmap_complement(seen_cpus, seen_cpus, max_cpus);
+        msg = enumerate_cpus(seen_cpus, max_cpus);
+        error_report("warning: CPU(s) not present in any NUMA nodes: %s", msg);
+        g_free(msg);
+    }
 }
 
 void parse_numa_opts(void)
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 4/4] numa: Print warning if no node is assigned to a CPU

[Paolo Bonzini]

On 12/02/2015 18:50, Eduardo Habkost wrote:
> +
> +    if (!bitmap_full(seen_cpus, max_cpus)) {
> +        char *msg;
> +        bitmap_complement(seen_cpus, seen_cpus, max_cpus);
> +        msg = enumerate_cpus(seen_cpus, max_cpus);
> +        error_report("warning: CPU(s) not present in any NUMA nodes: %s", msg);
> +        g_free(msg);
> +    }

What happens if you have a single node (useful to give it a memdev via
-numa node,memdev=...)?  It would be nice in this case to avoid the
warning and assign all CPUs to node 0.

Paolo

Re: [Qemu-devel] [PATCH v3 4/4] numa: Print warning if no node is assigned to a CPU

[Eduardo Habkost]

On Thu, Feb 12, 2015 at 07:22:37PM +0100, Paolo Bonzini wrote:
> On 12/02/2015 18:50, Eduardo Habkost wrote:
> > +
> > +    if (!bitmap_full(seen_cpus, max_cpus)) {
> > +        char *msg;
> > +        bitmap_complement(seen_cpus, seen_cpus, max_cpus);
> > +        msg = enumerate_cpus(seen_cpus, max_cpus);
> > +        error_report("warning: CPU(s) not present in any NUMA nodes: %s", msg);
> > +        g_free(msg);
> > +    }
> 
> What happens if you have a single node (useful to give it a memdev via
> -numa node,memdev=...)?  It would be nice in this case to avoid the
> warning and assign all CPUs to node 0.

The existing code at set_numa_nodes()/parse_numa_opts() should already do what
you suggest when we have only one node:

    for (i = 0; i < nb_numa_nodes; i++) {
        if (!bitmap_empty(numa_info[i].node_cpu, MAX_CPUMASK_BITS)) {
            break;
        }
    }
    /* assigning the VCPUs round-robin is easier to implement, guest OSes
     * must cope with this anyway, because there are BIOSes out there in
     * real machines which also use this scheme.
     */
    if (i == nb_numa_nodes) {
        for (i = 0; i < max_cpus; i++) {
            set_bit(i, numa_info[i % nb_numa_nodes].node_cpu);
        }
    }

I don't like how it ignores socket topology, but it is good enough for the
one-node case, at least.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 0/4] NUMA: Validate CPU configuration

[Eduardo Habkost]

Ping? Can somebody help review this?

On Thu, Feb 12, 2015 at 03:50:31PM -0200, Eduardo Habkost wrote:
> This adds extra checks to the NUMA code to make sure the CPU configuration is
> consistent. This needs to be applied on top of the following series:
> 
>   Message-Id: <1423421482-11619-1-git-send-email-ehabkost@redhat.com>
>   Subject: [Qemu-devel] [PATCH 0/7] NUMA code cleanup
>   From: Eduardo Habkost <ehabkost@redhat.com>
>   Date: Sun,  8 Feb 2015 16:51:15 -0200
>   Git tree: https://github.com/ehabkost/qemu.git numa-next
> 
> Changes v1 -> v2:
>  * (none, v1 was tagged by accident and never sent to qemu-devel)
> 
> Changes v2 -> v3:
>  * Fix off-by-one error on CPU index check
>  * Use GString and error_report() instead of calling fprintf() directly
>  * Simplify logic of the CPUs-not-present check
> 
> Eduardo Habkost (4):
>   numa: Fix off-by-one error at MAX_CPUMASK_BITS check
>   numa: Reject CPU indexes > max_cpus
>   numa: Reject configuration if CPU appears on multiple nodes
>   numa: Print warning if no node is assigned to a CPU
> 
>  numa.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 50 insertions(+), 3 deletions(-)
> 
> -- 
> 2.1.0
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 1/4] numa: Fix off-by-one error at MAX_CPUMASK_BITS check

[Igor Mammedov]

On Thu, 12 Feb 2015 15:50:32 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Fix the CPU index check to ensure we don't go beyond the size of the
> node_cpu bitmap.
> 
> CPU index is always less than MAX_CPUMASK_BITS, as documented at
> sysemu.h:
> 
> > The following shall be true for all CPUs:
> >   cpu->cpu_index < max_cpus <= MAX_CPUMASK_BITS
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  numa.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/numa.c b/numa.c
> index 0d15375..41e496b 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -76,9 +76,9 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
>      }
>  
>      for (cpus = node->cpus; cpus; cpus = cpus->next) {
> -        if (cpus->value > MAX_CPUMASK_BITS) {
> +        if (cpus->value >= MAX_CPUMASK_BITS) {
>              error_setg(errp, "CPU number %" PRIu16 " is bigger than %d",
> -                       cpus->value, MAX_CPUMASK_BITS);
> +                       cpus->value, MAX_CPUMASK_BITS - 1);
>              return;
>          }
>          bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);

Re: [Qemu-devel] [PATCH v3 2/4] numa: Reject CPU indexes > max_cpus

[Igor Mammedov]

On Thu, 12 Feb 2015 15:50:33 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> CPU index is always less than max_cpus, as documented at sysemu.h:
> 
> > The following shall be true for all CPUs:
> >   cpu->cpu_index < max_cpus <= MAX_CPUMASK_BITS
> 
> Reject configuration which uses invalid CPU indexes.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Excluding nitpicking below
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
> v1 -> v2: (no changes)
> 
> v2 -> v3:
>  * Check for (cpu >= max_cpus) instead of (cpu > max_cpus)
>  * Reword error message as we are not checking for "bigger than maxcpus"
>    CPU indexes
> ---
>  numa.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/numa.c b/numa.c
> index 41e496b..4139e46 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -76,9 +76,11 @@ static void numa_node_parse(NumaNodeOptions *node, QemuOpts *opts, Error **errp)
>      }
>  
>      for (cpus = node->cpus; cpus; cpus = cpus->next) {
> -        if (cpus->value >= MAX_CPUMASK_BITS) {
> -            error_setg(errp, "CPU number %" PRIu16 " is bigger than %d",
> -                       cpus->value, MAX_CPUMASK_BITS - 1);
> +        if (cpus->value >= max_cpus) {
> +            error_setg(errp,
> +                       "CPU index (%" PRIu16 ")" \
'\' is not really necessary here

> +                       " should be smaller than maxcpus (%d)",
> +                       cpus->value, max_cpus);
>              return;
>          }
>          bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);

Re: [Qemu-devel] [PATCH v3 3/4] numa: Reject configuration if CPU appears on multiple nodes

[Igor Mammedov]

On Thu, 12 Feb 2015 15:50:34 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Each CPU can appear in only one NUMA node on the NUMA config. Reject
> configuration if a CPU appears in multiple nodes.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> v1 -> v2: (no changes)
> 
> v2 -> v3:
>  * Rename present_cpus to seen_cpus, to make it less confusing
>  * Use GString and error_report() instead of multiple fprintf() calls
With comment below fixed:
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  numa.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/numa.c b/numa.c
> index 4139e46..712faff 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -168,6 +168,41 @@ error:
>      return -1;
>  }
>  
> +static char *enumerate_cpus(unsigned long *cpus, int max_cpus)
> +{
> +    int cpu;
> +    bool first = true;
> +    GString *s = g_string_new(NULL);
> +
> +    for (cpu = find_first_bit(cpus, max_cpus);
> +        cpu < max_cpus;
> +        cpu = find_next_bit(cpus, max_cpus, cpu + 1)) {
> +        g_string_append_printf(s, "%s%d", first ? "" : " ", cpu);
> +        first = false;
> +    }
> +    return g_string_free(s, FALSE);
> +}
> +
> +static void validate_numa_cpus(void)
> +{
> +    int i;
> +    DECLARE_BITMAP(seen_cpus, MAX_CPUMASK_BITS);
> +
> +    bitmap_zero(seen_cpus, MAX_CPUMASK_BITS);
> +    for (i = 0; i < nb_numa_nodes; i++) {
> +        if (bitmap_intersects(seen_cpus, numa_info[i].node_cpu,
> +                              MAX_CPUMASK_BITS)) {
> +            bitmap_and(seen_cpus, seen_cpus,
> +                       numa_info[i].node_cpu, MAX_CPUMASK_BITS);
> +            error_report("CPU(s) present in multiple NUMA nodes: %s",
> +                         enumerate_cpus(seen_cpus, max_cpus));;
> +            exit(1);
s/1/EXIT_FAILURE/

> +        }
> +        bitmap_or(seen_cpus, seen_cpus,
> +                  numa_info[i].node_cpu, MAX_CPUMASK_BITS);
> +    }
> +}
> +
>  void parse_numa_opts(void)
>  {
>      int i;
> @@ -245,6 +280,8 @@ void parse_numa_opts(void)
>                  set_bit(i, numa_info[i % nb_numa_nodes].node_cpu);
>              }
>          }
> +
> +        validate_numa_cpus();
>      }
>  }
>  

Re: [Qemu-devel] [PATCH v3 4/4] numa: Print warning if no node is assigned to a CPU

[Igor Mammedov]

On Thu, 12 Feb 2015 15:50:35 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Instead of silently assigning CPU to node 0 when it is omitted from the
> command-line, check if all CPUs up to max_cpus are present in the NUMA
> configuration.
That would also trigger warning for possible (i.e. to be hotplugged) CPUs
as well.

> 
> I am making this a warning and not a fatal error, to allow management
> software to be updated if necessary.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> v1 -> v2: (no changes)
> 
> v2 -> v3:
>  * Use enumerate_cpus() and error_report() for error message
>  * Simplify logic using bitmap_full()
> ---
>  numa.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/numa.c b/numa.c
> index 712faff..4310bf9 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -201,6 +201,14 @@ static void validate_numa_cpus(void)
>          bitmap_or(seen_cpus, seen_cpus,
>                    numa_info[i].node_cpu, MAX_CPUMASK_BITS);
>      }
> +
> +    if (!bitmap_full(seen_cpus, max_cpus)) {
> +        char *msg;
> +        bitmap_complement(seen_cpus, seen_cpus, max_cpus);
> +        msg = enumerate_cpus(seen_cpus, max_cpus);
> +        error_report("warning: CPU(s) not present in any NUMA nodes: %s", msg);
Maybe add to this that all CPUs up to maxcpus must be described in numa config?

> +        g_free(msg);
> +    }
>  }
>  
>  void parse_numa_opts(void)

Re: [Qemu-devel] [PATCH v3 4/4] numa: Print warning if no node is assigned to a CPU

[Eduardo Habkost]

On Tue, Feb 24, 2015 at 09:01:04AM +0100, Igor Mammedov wrote:
> On Thu, 12 Feb 2015 15:50:35 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > Instead of silently assigning CPU to node 0 when it is omitted from the
> > command-line, check if all CPUs up to max_cpus are present in the NUMA
> > configuration.
> That would also trigger warning for possible (i.e. to be hotplugged) CPUs
> as well.

And that's correct, right? As far as I can see, there's no _PXM method
in our Processor objects, so we need to know the NUMA node for all
possible VCPUs in the moment the SRAT is generated. Or am I missing
something?


[...]
> > +    if (!bitmap_full(seen_cpus, max_cpus)) {
> > +        char *msg;
> > +        bitmap_complement(seen_cpus, seen_cpus, max_cpus);
> > +        msg = enumerate_cpus(seen_cpus, max_cpus);
> > +        error_report("warning: CPU(s) not present in any NUMA nodes: %s", msg);
> Maybe add to this that all CPUs up to maxcpus must be described in numa config?

I will update it. Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 4/4] numa: Print warning if no node is assigned to a CPU

[Igor Mammedov]

On Tue, 24 Feb 2015 15:19:28 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Feb 24, 2015 at 09:01:04AM +0100, Igor Mammedov wrote:
> > On Thu, 12 Feb 2015 15:50:35 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > Instead of silently assigning CPU to node 0 when it is omitted from the
> > > command-line, check if all CPUs up to max_cpus are present in the NUMA
> > > configuration.
> > That would also trigger warning for possible (i.e. to be hotplugged) CPUs
> > as well.
> 
> And that's correct, right? As far as I can see, there's no _PXM method
> in our Processor objects, so we need to know the NUMA node for all
> possible VCPUs in the moment the SRAT is generated. Or am I missing
> something?
Yep, that's how it's now. And probably it should stay so.
Just define topology at startup and there won't be warnings.

> 
> 
> [...]
> > > +    if (!bitmap_full(seen_cpus, max_cpus)) {
> > > +        char *msg;
> > > +        bitmap_complement(seen_cpus, seen_cpus, max_cpus);
> > > +        msg = enumerate_cpus(seen_cpus, max_cpus);
> > > +        error_report("warning: CPU(s) not present in any NUMA nodes: %s", msg);
> > Maybe add to this that all CPUs up to maxcpus must be described in numa config?
> 
> I will update it. Thanks!
>