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 ehabk...@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

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 ehabk...@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 ehabk...@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

[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 ehabk...@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 ehabk...@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

[Igor Mammedov]

On Mon,  9 Feb 2015 17:53:15 -0200
Eduardo Habkost ehabk...@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 ehabk...@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

[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 ehabk...@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 ehabk...@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 ehabk...@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 pbonz...@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
 

[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 ehabk...@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