[v1] qemu: check if numa cell's cpu range match with cpu topology count

[libvirt] [PATCH] qemu: check if numa cell's cpu range match with cpu topology count

Posted by Maxiwell S. Garcia 4 years, 10 months ago

If the XML doesn't have numa cells, this check is not necessary. But
if numa cells are present, it must match with cpu topology count.

Signed-off-by: Maxiwell S. Garcia <maxiwell@linux.ibm.com>
---
 src/qemu/qemu_domain.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 4d3a8868b2..ab81b9a5be 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4173,15 +4173,24 @@ qemuDomainDefValidate(const virDomainDef *def,
     if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
         unsigned int topologycpus;
         unsigned int granularity;
+        unsigned int numacpus;
 
         /* Starting from QEMU 2.5, max vCPU count and overall vCPU topology
          * must agree. We only actually enforce this with QEMU 2.7+, due
          * to the capability check above */
-        if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 &&
-            topologycpus != virDomainDefGetVcpusMax(def)) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("CPU topology doesn't match maximum vcpu count"));
-            goto cleanup;
+        if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0) {
+            if (topologycpus != virDomainDefGetVcpusMax(def)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("CPU topology doesn't match maximum vcpu count"));
+                goto cleanup;
+            }
+
+            numacpus = virDomainNumaGetCPUCountTotal(def->numa);
+            if ((numacpus != 0) && (topologycpus != numacpus)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("CPU topology doesn't match numa CPU count"));
+                goto cleanup;
+            }
         }
 
         /* vCPU hotplug granularity must be respected */
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu: check if numa cell's cpu range match with cpu topology count

Posted by Peter Krempa 4 years, 10 months ago

On Fri, Jun 07, 2019 at 16:22:08 -0300, Maxiwell S. Garcia wrote:
> If the XML doesn't have numa cells, this check is not necessary. But
> if numa cells are present, it must match with cpu topology count.

You should also describe that you are fixing the warning in the VM log
file that says that the non-full NUMA topologies will be deprecated.

> 
> Signed-off-by: Maxiwell S. Garcia <maxiwell@linux.ibm.com>
> ---
>  src/qemu/qemu_domain.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 4d3a8868b2..ab81b9a5be 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -4173,15 +4173,24 @@ qemuDomainDefValidate(const virDomainDef *def,
>      if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
>          unsigned int topologycpus;
>          unsigned int granularity;
> +        unsigned int numacpus;
>  
>          /* Starting from QEMU 2.5, max vCPU count and overall vCPU topology
>           * must agree. We only actually enforce this with QEMU 2.7+, due
>           * to the capability check above */
> -        if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 &&
> -            topologycpus != virDomainDefGetVcpusMax(def)) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> -                           _("CPU topology doesn't match maximum vcpu count"));
> -            goto cleanup;
> +        if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0) {
> +            if (topologycpus != virDomainDefGetVcpusMax(def)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("CPU topology doesn't match maximum vcpu count"));
> +                goto cleanup;
> +            }
> +
> +            numacpus = virDomainNumaGetCPUCountTotal(def->numa);
> +            if ((numacpus != 0) && (topologycpus != numacpus)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("CPU topology doesn't match numa CPU count"));
> +                goto cleanup;
> +            }
>          }

I'm afraid that this might cause regressions. We've ignored the
relationship between the cpu count and NUMA topology for far too long to
have enough possible users that this might break with.

I think we can do this only if it is bound to a new capability so that
it's rejected only for new qemus and thus does not break retroactively.

Until the new version is used we should also probably just add all
non-enumerated vCPUs into the first NUMA node, because that is what qemu
would do anyways.

Also it might require some docs changes.

Additionally you also need to correct the check in qemuDomainSetVcpusMax
to do the same thing.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list