diff mbox

[v2,2/3] numa: Reject configuration if CPU appears on multiple nodes

Message ID 1423511596-2584-3-git-send-email-ehabkost@redhat.com
State New
Headers show

Commit Message

Eduardo Habkost Feb. 9, 2015, 7:53 p.m. UTC 
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(+)

Comments

Igor Mammedov Feb. 12, 2015, 2:22 p.m. UTC | #1 
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();
>      }
>  }
>
Igor Mammedov Feb. 12, 2015, 3:01 p.m. UTC | #2 
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();
>      }
>  }
>
Paolo Bonzini Feb. 12, 2015, 3:18 p.m. UTC | #3 
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
Eduardo Habkost Feb. 12, 2015, 3:24 p.m. UTC | #4 
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 Habkost Feb. 12, 2015, 3:26 p.m. UTC | #5 
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!
Igor Mammedov Feb. 12, 2015, 4:01 p.m. UTC | #6 
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
Igor Mammedov Feb. 12, 2015, 4:04 p.m. UTC | #7 
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
>
Eduardo Habkost Feb. 12, 2015, 5:54 p.m. UTC | #8 
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)
diff mbox

Patch

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();
     }
 }