[Qemu-devel] [PATCH v2 0/3] NUMA: Validate CPU configuration

[Qemu-devel] [PATCH v2 0/3] 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

Eduardo Habkost (3):
  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 | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v2 1/3] numa: Reject CPU indexes > max_cpus

[Eduardo Habkost]

The CPU indexes for NUMA nodes make sense only up to max_cpus, and CPU
indexes > max_cpus are ignored. Reject configuration which uses invalid
CPU indexes.

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

diff --git a/numa.c b/numa.c
index 0d15375..f768434 100644
--- a/numa.c
+++ b/numa.c
@@ -76,9 +76,10 @@ 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);
+        if (cpus->value > max_cpus) {
+            error_setg(errp,
+                       "CPU number %" PRIu16 " is bigger than maxcpus (%d)",
+                       cpus->value, max_cpus);
             return;
         }
         bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);
-- 
2.1.0

[Qemu-devel] [PATCH v2 2/3] 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>
---
 numa.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/numa.c b/numa.c
index f768434..f004a74 100644
--- a/numa.c
+++ b/numa.c
@@ -167,6 +167,31 @@ error:
     return -1;
 }
 
+static void validate_numa_cpus(void)
+{
+    int i, cpu;
+    DECLARE_BITMAP(present_cpus, MAX_CPUMASK_BITS);
+
+    bitmap_zero(present_cpus, MAX_CPUMASK_BITS);
+    for (i = 0; i < nb_numa_nodes; i++) {
+        if (bitmap_intersects(present_cpus, numa_info[i].node_cpu,
+                              MAX_CPUMASK_BITS)) {
+            bitmap_and(present_cpus, present_cpus,
+                       numa_info[i].node_cpu, MAX_CPUMASK_BITS);
+            fprintf(stderr, "CPU(s) present in multiple NUMA nodes:");
+            for (cpu = find_first_bit(present_cpus, MAX_CPUMASK_BITS);
+                cpu < MAX_CPUMASK_BITS;
+                cpu = find_next_bit(present_cpus, MAX_CPUMASK_BITS, cpu + 1)) {
+                fprintf(stderr, " %d", cpu);
+            }
+            fprintf(stderr, "\n");
+            exit(1);
+        }
+        bitmap_or(present_cpus, present_cpus,
+                  numa_info[i].node_cpu, MAX_CPUMASK_BITS);
+    }
+}
+
 void parse_numa_opts(void)
 {
     int i;
@@ -244,6 +269,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 v2 3/3] 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>
---
 numa.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/numa.c b/numa.c
index f004a74..722a7fe 100644
--- a/numa.c
+++ b/numa.c
@@ -190,6 +190,17 @@ static void validate_numa_cpus(void)
         bitmap_or(present_cpus, present_cpus,
                   numa_info[i].node_cpu, MAX_CPUMASK_BITS);
     }
+
+    cpu = find_first_zero_bit(present_cpus, MAX_CPUMASK_BITS);
+    if (cpu < max_cpus) {
+        fprintf(stderr, "warning: CPU(s) not present in any NUMA nodes:");
+        for (;
+            cpu < max_cpus;
+            cpu = find_next_zero_bit(present_cpus, MAX_CPUMASK_BITS, cpu + 1)) {
+            fprintf(stderr, " %d", cpu);
+        }
+        fprintf(stderr, "\n");
+    }
 }
 
 void parse_numa_opts(void)
-- 
2.1.0

Re: [Qemu-devel] [PATCH v2 1/3] numa: Reject CPU indexes > max_cpus

[Igor Mammedov]

On Mon,  9 Feb 2015 17:53:14 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> The CPU indexes for NUMA nodes make sense only up to max_cpus, and CPU
> indexes > max_cpus are ignored. Reject configuration which uses invalid
> CPU indexes.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  numa.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/numa.c b/numa.c
> index 0d15375..f768434 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -76,9 +76,10 @@ 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);
> +        if (cpus->value > max_cpus) {
> +            error_setg(errp,
> +                       "CPU number %" PRIu16 " is bigger than maxcpus (%d)",
> +                       cpus->value, max_cpus);
>              return;
>          }
>          bitmap_set(numa_info[nodenr].node_cpu, cpus->value, 1);

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

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

[Igor Mammedov]

On Mon,  9 Feb 2015 17:53:15 -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>
> ---
>  numa.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/numa.c b/numa.c
> index f768434..f004a74 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -167,6 +167,31 @@ error:
>      return -1;
>  }
>  
> +static void validate_numa_cpus(void)
> +{
> +    int i, cpu;
> +    DECLARE_BITMAP(present_cpus, MAX_CPUMASK_BITS);
> +
> +    bitmap_zero(present_cpus, MAX_CPUMASK_BITS);
> +    for (i = 0; i < nb_numa_nodes; i++) {
> +        if (bitmap_intersects(present_cpus, numa_info[i].node_cpu,
> +                              MAX_CPUMASK_BITS)) {
> +            bitmap_and(present_cpus, present_cpus,
> +                       numa_info[i].node_cpu, MAX_CPUMASK_BITS);

> +            fprintf(stderr, "CPU(s) present in multiple NUMA nodes:");
> +            for (cpu = find_first_bit(present_cpus, MAX_CPUMASK_BITS);
> +                cpu < MAX_CPUMASK_BITS;
> +                cpu = find_next_bit(present_cpus, MAX_CPUMASK_BITS, cpu + 1)) {
> +                fprintf(stderr, " %d", cpu);
> +            }
> +            fprintf(stderr, "\n");
how about replacing a bunch if fprintf's with something like this:

cpu = 0;
while((cpu = find_next_bit(present_cpus, MAX_CPUMASK_BITS, cpu + 1)) !=MAX_CPUMASK_BITS)
    str = g_strdup_printf("%s %d", str, cpu);
    
error_report("CPU(s) present in multiple NUMA nodes: %s\n", str);


> +            exit(1);
> +        }
> +        bitmap_or(present_cpus, present_cpus,
> +                  numa_info[i].node_cpu, MAX_CPUMASK_BITS);
> +    }
> +}
> +
>  void parse_numa_opts(void)
>  {
>      int i;
> @@ -244,6 +269,8 @@ void parse_numa_opts(void)
>                  set_bit(i, numa_info[i % nb_numa_nodes].node_cpu);
>              }
>          }
> +
> +        validate_numa_cpus();
>      }
>  }
>  

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

[Igor Mammedov]

On Mon,  9 Feb 2015 17:53:15 -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>
> ---
>  numa.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/numa.c b/numa.c
> index f768434..f004a74 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -167,6 +167,31 @@ error:
>      return -1;
>  }
>  
> +static void validate_numa_cpus(void)
> +{
> +    int i, cpu;
> +    DECLARE_BITMAP(present_cpus, MAX_CPUMASK_BITS);
naming is a bit confusing, it's not really present CPUs but
more like possible_cpus

> +
> +    bitmap_zero(present_cpus, MAX_CPUMASK_BITS);
> +    for (i = 0; i < nb_numa_nodes; i++) {
> +        if (bitmap_intersects(present_cpus, numa_info[i].node_cpu,
> +                              MAX_CPUMASK_BITS)) {
> +            bitmap_and(present_cpus, present_cpus,
> +                       numa_info[i].node_cpu, MAX_CPUMASK_BITS);
> +            fprintf(stderr, "CPU(s) present in multiple NUMA nodes:");
> +            for (cpu = find_first_bit(present_cpus, MAX_CPUMASK_BITS);
> +                cpu < MAX_CPUMASK_BITS;
> +                cpu = find_next_bit(present_cpus, MAX_CPUMASK_BITS, cpu + 1)) {
> +                fprintf(stderr, " %d", cpu);
> +            }
> +            fprintf(stderr, "\n");
> +            exit(1);
> +        }
> +        bitmap_or(present_cpus, present_cpus,
> +                  numa_info[i].node_cpu, MAX_CPUMASK_BITS);
> +    }
> +}
> +
>  void parse_numa_opts(void)
>  {
>      int i;
> @@ -244,6 +269,8 @@ void parse_numa_opts(void)
>                  set_bit(i, numa_info[i % nb_numa_nodes].node_cpu);
>              }
>          }
> +
> +        validate_numa_cpus();
>      }
>  }
>  

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

[Paolo Bonzini]

On 12/02/2015 15:22, Igor Mammedov wrote:
> how about replacing a bunch if fprintf's with something like this:
> 
> cpu = 0;
> while((cpu = find_next_bit(present_cpus, MAX_CPUMASK_BITS, cpu + 1)) !=MAX_CPUMASK_BITS)
>     str = g_strdup_printf("%s %d", str, cpu);
>     
> error_report("CPU(s) present in multiple NUMA nodes: %s\n", str);

As written above this leaks memory, and there should be no space before
the final %s.  So it's not that simple. :)  But using error_report is
nicer indeed, and you can use GString to avoid the leak.

Paolo

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

[Eduardo Habkost]

On Thu, Feb 12, 2015 at 04:01:49PM +0100, Igor Mammedov wrote:
> On Mon,  9 Feb 2015 17:53:15 -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>
> > ---
> >  numa.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/numa.c b/numa.c
> > index f768434..f004a74 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -167,6 +167,31 @@ error:
> >      return -1;
> >  }
> >  
> > +static void validate_numa_cpus(void)
> > +{
> > +    int i, cpu;
> > +    DECLARE_BITMAP(present_cpus, MAX_CPUMASK_BITS);
> naming is a bit confusing, it's not really present CPUs but
> more like possible_cpus

I meant "present in the NUMA configuration". "Possible" wouldn't
describe it IMO, as it is just tracking the CPUs seen in the config. I
will rename it to "seen_cpus" in the next version.

-- 
Eduardo

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

[Eduardo Habkost]

On Thu, Feb 12, 2015 at 04:18:31PM +0100, Paolo Bonzini wrote:
> 
> 
> On 12/02/2015 15:22, Igor Mammedov wrote:
> > how about replacing a bunch if fprintf's with something like this:
> > 
> > cpu = 0;
> > while((cpu = find_next_bit(present_cpus, MAX_CPUMASK_BITS, cpu + 1)) !=MAX_CPUMASK_BITS)
> >     str = g_strdup_printf("%s %d", str, cpu);
> >     
> > error_report("CPU(s) present in multiple NUMA nodes: %s\n", str);
> 
> As written above this leaks memory, and there should be no space before
> the final %s.  So it's not that simple. :)  But using error_report is
> nicer indeed, and you can use GString to avoid the leak.

Complex string-building logic was exactly what I was trying to avoid
when I decided to use fprintf(). But I didn't know about GString, I will
give it a try on the next version.  Thanks!

-- 
Eduardo

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

[Igor Mammedov]

On Thu, 12 Feb 2015 13:24:26 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Feb 12, 2015 at 04:01:49PM +0100, Igor Mammedov wrote:
> > On Mon,  9 Feb 2015 17:53:15 -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>
> > > ---
> > >  numa.c | 27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/numa.c b/numa.c
> > > index f768434..f004a74 100644
> > > --- a/numa.c
> > > +++ b/numa.c
> > > @@ -167,6 +167,31 @@ error:
> > >      return -1;
> > >  }
> > >  
> > > +static void validate_numa_cpus(void)
> > > +{
> > > +    int i, cpu;
> > > +    DECLARE_BITMAP(present_cpus, MAX_CPUMASK_BITS);
> > naming is a bit confusing, it's not really present CPUs but
> > more like possible_cpus
> 
> I meant "present in the NUMA configuration". "Possible" wouldn't
> describe it IMO, as it is just tracking the CPUs seen in the config. I
> will rename it to "seen_cpus" in the next version.
or maybe numa_cpus

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

[Igor Mammedov]

On Thu, 12 Feb 2015 16:18:31 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 12/02/2015 15:22, Igor Mammedov wrote:
> > how about replacing a bunch if fprintf's with something like this:
> > 
> > cpu = 0;
> > while((cpu = find_next_bit(present_cpus, MAX_CPUMASK_BITS, cpu + 1)) !=MAX_CPUMASK_BITS)
> >     str = g_strdup_printf("%s %d", str, cpu);
> >     
> > error_report("CPU(s) present in multiple NUMA nodes: %s\n", str);
> 
> As written above this leaks memory, and there should be no space before
> the final %s.  So it's not that simple. :) 
it hasn't been meant as working drop in code

> But using error_report is
> nicer indeed, and you can use GString to avoid the leak.
good to learn about GString

> 
> Paolo
> 

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

[Eduardo Habkost]

On Thu, Feb 12, 2015 at 05:01:54PM +0100, Igor Mammedov wrote:
> On Thu, 12 Feb 2015 13:24:26 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Feb 12, 2015 at 04:01:49PM +0100, Igor Mammedov wrote:
[...]
> > > > +    DECLARE_BITMAP(present_cpus, MAX_CPUMASK_BITS);
> > > naming is a bit confusing, it's not really present CPUs but
> > > more like possible_cpus
> > 
> > I meant "present in the NUMA configuration". "Possible" wouldn't
> > describe it IMO, as it is just tracking the CPUs seen in the config. I
> > will rename it to "seen_cpus" in the next version.
> or maybe numa_cpus

We're already inside numa.c in a function called validate_numa_cpus(). I
believe a "numa_" prefix would be redundant.

(I just sent v3 of the series using seen_cpus, anyway)

-- 
Eduardo