| Message ID | 1423763435-3696-5-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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)
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!
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! >
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)
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(+)