[Qemu-devel] [PATCH v4 0/1] NUMA: Validate CPU configuration

[Qemu-devel] [PATCH v4 0/1] 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 my numa patch queue:

  https://github.com/ehabkost/qemu.git numa

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

Changes v3 -> v4:
 * Patches 1-3 from v3 are now already in the numa queue (URL above)
 * Clarify error message when some CPUs are missing from the config

Eduardo Habkost (1):
  numa: Print warning if no node is assigned to a CPU

 numa.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

-- 
2.1.0

[Qemu-devel] [PATCH v4 1/1] 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()

v3 -> v4:
 * Clarify error message, mention that all CPUs up to
   maxcpus need to be described in NUMA config
---
 numa.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/numa.c b/numa.c
index 9a3fc15..d8021b9 100644
--- a/numa.c
+++ b/numa.c
@@ -201,6 +201,16 @@ 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);
+        error_report("warning: All CPU(s) up to maxcpus should be described "
+                     "in NUMA config");
+        g_free(msg);
+    }
 }
 
 void parse_numa_opts(void)
-- 
2.1.0

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

[Eduardo Habkost]

On Tue, Feb 24, 2015 at 03:57:23PM -0300, Eduardo Habkost 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.
> 
> 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>

Ping? Any feedback, Acked-by, or Reviewed-by lines?

> ---
> v1 -> v2: (no changes)
> 
> v2 -> v3:
>  * Use enumerate_cpus() and error_report() for error message
>  * Simplify logic using bitmap_full()
> 
> v3 -> v4:
>  * Clarify error message, mention that all CPUs up to
>    maxcpus need to be described in NUMA config
> ---
>  numa.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/numa.c b/numa.c
> index 9a3fc15..d8021b9 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -201,6 +201,16 @@ 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);
> +        error_report("warning: All CPU(s) up to maxcpus should be described "
> +                     "in NUMA config");
> +        g_free(msg);
> +    }
>  }
>  
>  void parse_numa_opts(void)
> -- 
> 2.1.0
> 
> 

-- 
Eduardo

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

[Michael S. Tsirkin]

On Tue, Feb 24, 2015 at 03:57:23PM -0300, Eduardo Habkost 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.

Could you please describe the problematic configuration
in more detail in commit log?
Also, how does this interact with cpu hotplug?

> 
> 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()
> 
> v3 -> v4:
>  * Clarify error message, mention that all CPUs up to
>    maxcpus need to be described in NUMA config
> ---
>  numa.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/numa.c b/numa.c
> index 9a3fc15..d8021b9 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -201,6 +201,16 @@ 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);
> +        error_report("warning: All CPU(s) up to maxcpus should be described "
> +                     "in NUMA config");

What happens with e.g. windows guests when this warning is emitted?
do they blue-screen?

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

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

[Eduardo Habkost]

On Wed, Mar 04, 2015 at 01:30:35PM +0100, Michael S. Tsirkin wrote:
> On Tue, Feb 24, 2015 at 03:57:23PM -0300, Eduardo Habkost 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.
> 
> Could you please describe the problematic configuration
> in more detail in commit log?
> Also, how does this interact with cpu hotplug?

I will try to reword the message to make it clearer. Suggestions are
welcome.

Basically, the problem is:

* Currently all possible CPUs (including hotplug ones) need to be
  present in the SRAT when QEMU is started[1]. QEMU already does that.
* When a CPU is ommitted from the command-line, QEMU is silently
  assigning it to node 0. This is not a very useful default and
  just confuse users.


> > 
> > 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()
> > 
> > v3 -> v4:
> >  * Clarify error message, mention that all CPUs up to
> >    maxcpus need to be described in NUMA config
> > ---
> >  numa.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/numa.c b/numa.c
> > index 9a3fc15..d8021b9 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -201,6 +201,16 @@ 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);
> > +        error_report("warning: All CPU(s) up to maxcpus should be described "
> > +                     "in NUMA config");
> 
> What happens with e.g. windows guests when this warning is emitted?
> do they blue-screen?

As described on the commit log above, the CPU is silently assigned to
node 0 when it is omitted from the command-line. In other words, this
won't make guests confused, but can make users confused.

All possible VCPUs (including the hotplug ones, i.e. up to max_cpus) are
expected to be present in the SRAT, and they are already present. The
only problem is that instead of complaining about missing CPUs, QEMU is
assigning node 0 to them.

-- 
Eduardo

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

[Michael S. Tsirkin]

On Wed, Mar 04, 2015 at 10:35:51AM -0300, Eduardo Habkost wrote:
> On Wed, Mar 04, 2015 at 01:30:35PM +0100, Michael S. Tsirkin wrote:
> > On Tue, Feb 24, 2015 at 03:57:23PM -0300, Eduardo Habkost 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.
> > 
> > Could you please describe the problematic configuration
> > in more detail in commit log?
> > Also, how does this interact with cpu hotplug?
> 
> I will try to reword the message to make it clearer. Suggestions are
> welcome.


Just include the command line that reproduces the problem,
and describe the guest behaviour that results.

> Basically, the problem is:
> 
> * Currently all possible CPUs (including hotplug ones) need to be
>   present in the SRAT when QEMU is started[1]. QEMU already does that.
> * When a CPU is ommitted from the command-line, QEMU is silently
>   assigning it to node 0. This is not a very useful default and
>   just confuse users.

It's reasonable if user doesn't want to play with numa, isn't it?

> 
> > > 
> > > 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()
> > > 
> > > v3 -> v4:
> > >  * Clarify error message, mention that all CPUs up to
> > >    maxcpus need to be described in NUMA config
> > > ---
> > >  numa.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/numa.c b/numa.c
> > > index 9a3fc15..d8021b9 100644
> > > --- a/numa.c
> > > +++ b/numa.c
> > > @@ -201,6 +201,16 @@ 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);
> > > +        error_report("warning: All CPU(s) up to maxcpus should be described "
> > > +                     "in NUMA config");
> > 
> > What happens with e.g. windows guests when this warning is emitted?
> > do they blue-screen?
> 
> As described on the commit log above, the CPU is silently assigned to
> node 0 when it is omitted from the command-line. In other words, this
> won't make guests confused, but can make users confused.
> 
> All possible VCPUs (including the hotplug ones, i.e. up to max_cpus) are
> expected to be present in the SRAT, and they are already present. The
> only problem is that instead of complaining about missing CPUs, QEMU is
> assigning node 0 to them.
> 
> -- 
> Eduardo

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

[Eduardo Habkost]

On Wed, Mar 04, 2015 at 02:50:59PM +0100, Michael S. Tsirkin wrote:
> On Wed, Mar 04, 2015 at 10:35:51AM -0300, Eduardo Habkost wrote:
> > On Wed, Mar 04, 2015 at 01:30:35PM +0100, Michael S. Tsirkin wrote:
> > > On Tue, Feb 24, 2015 at 03:57:23PM -0300, Eduardo Habkost 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.
> > > 
> > > Could you please describe the problematic configuration
> > > in more detail in commit log?
> > > Also, how does this interact with cpu hotplug?
> > 
> > I will try to reword the message to make it clearer. Suggestions are
> > welcome.
> 
> 
> Just include the command line that reproduces the problem,
> and describe the guest behaviour that results.

There's no guest behavior to describe, except that it will see the CPU assigned
to node 0 (as already described above).

I submitted a new version without a command-line example, already. I assumed it
would be obvious to anybody who used the -numa command-line argument before,
but here it is:

Correct command-line, no warning:

  $ qemu-system-x86_64 -smp 2,maxcpus=4
  $ qemu-system-x86_64 -smp 2,maxcpus=4 -numa node,cpus=0-3

Incomplete command-line, with warnings:

  $ qemu-system-x86_64 -smp 2,maxcpus=4 -numa node,cpus=0
  qemu-system-x86_64: warning: CPU(s) not present in any NUMA nodes: 1 2 3
  qemu-system-x86_64: warning: All CPU(s) up to maxcpus should be described in NUMA config

  $ qemu-system-x86_64 -smp 2,maxcpus=4 -numa node,cpus=0-2
  qemu-system-x86_64: warning: CPU(s) not present in any NUMA nodes: 3
  qemu-system-x86_64: warning: All CPU(s) up to maxcpus should be described in NUMA config


> 
> > Basically, the problem is:
> > 
> > * Currently all possible CPUs (including hotplug ones) need to be
> >   present in the SRAT when QEMU is started[1]. QEMU already does that.
> > * When a CPU is ommitted from the command-line, QEMU is silently
> >   assigning it to node 0. This is not a very useful default and
> >   just confuse users.
> 
> It's reasonable if user doesn't want to play with numa, isn't it?

This is validation of the configuration when NUMA nodes are configured.
If the user doesn't want to play with numa, there won't be any -numa
arguments in the command-line.

> 
> > 
> > > > 
> > > > 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()
> > > > 
> > > > v3 -> v4:
> > > >  * Clarify error message, mention that all CPUs up to
> > > >    maxcpus need to be described in NUMA config
> > > > ---
> > > >  numa.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/numa.c b/numa.c
> > > > index 9a3fc15..d8021b9 100644
> > > > --- a/numa.c
> > > > +++ b/numa.c
> > > > @@ -201,6 +201,16 @@ 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);
> > > > +        error_report("warning: All CPU(s) up to maxcpus should be described "
> > > > +                     "in NUMA config");
> > > 
> > > What happens with e.g. windows guests when this warning is emitted?
> > > do they blue-screen?
> > 
> > As described on the commit log above, the CPU is silently assigned to
> > node 0 when it is omitted from the command-line. In other words, this
> > won't make guests confused, but can make users confused.
> > 
> > All possible VCPUs (including the hotplug ones, i.e. up to max_cpus) are
> > expected to be present in the SRAT, and they are already present. The
> > only problem is that instead of complaining about missing CPUs, QEMU is
> > assigning node 0 to them.
> > 
> > -- 
> > Eduardo

-- 
Eduardo