[libvirt] [PATCH] qemu: Check for maximum vcpus exceeding cpu topology

Kothapally Madhu Pavan posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20170207125621.10378.2543.stgit@ltcjuno01
src/qemu/qemu_process.c |    7 +++++++
1 file changed, 7 insertions(+)
[libvirt] [PATCH] qemu: Check for maximum vcpus exceeding cpu topology
Posted by Kothapally Madhu Pavan 7 years, 1 month ago
This patch will prevent guest to start when the maximum
vcpus are greater than cpu topology limit. Currently similar
checks do exist only during setvcpus. The patch adds the
missing check. The c9cb35c255222 reverted similar check to
avoid older configs from vanishing. The current patch adds
it in domain validation part to be consistent with the
original intent.

Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com>
---
 src/qemu/qemu_process.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 184440d..f0d42b8 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3738,6 +3738,13 @@ qemuValidateCpuCount(virDomainDefPtr def,
         return -1;
     }
 
+    if (def->cpu->sockets && virDomainDefGetVcpusMax(def) >
+        def->cpu->sockets * def->cpu->cores * def->cpu->threads) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("Maximum CPUs greater than topology limit"));
+        return -1;
+    }
+
     if (maxCpus > 0 && virDomainDefGetVcpusMax(def) > maxCpus) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                        _("Maximum CPUs greater than specified machine type limit"));

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Check for maximum vcpus exceeding cpu topology
Posted by Peter Krempa 7 years, 1 month ago
On Tue, Feb 07, 2017 at 07:56:29 -0500, Kothapally Madhu Pavan wrote:
> This patch will prevent guest to start when the maximum
> vcpus are greater than cpu topology limit. Currently similar
> checks do exist only during setvcpus. The patch adds the
> missing check. The c9cb35c255222 reverted similar check to
> avoid older configs from vanishing. The current patch adds
> it in domain validation part to be consistent with the
> original intent.
> 
> Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com>
> ---
>  src/qemu/qemu_process.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 184440d..f0d42b8 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3738,6 +3738,13 @@ qemuValidateCpuCount(virDomainDefPtr def,
>          return -1;
>      }
>  
> +    if (def->cpu->sockets && virDomainDefGetVcpusMax(def) >
> +        def->cpu->sockets * def->cpu->cores * def->cpu->threads) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("Maximum CPUs greater than topology limit"));
> +        return -1;
> +    }

This exact logic is done in qemuDomainDefValidate:

+    if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 &&
+        topologycpus != virDomainDefGetVcpusMax(def)) {
+        /* presence of query-hotpluggable-cpus should be a good enough witness */
+        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("CPU topology doesn't match maximum vcpu count"));
+            goto cleanup;
         }
     }

See commit 043ba4a40a4ae26cf616146d0d1c129d65b156b8. The only difference
is that the code is validated only for certain versions of qemu known to
reject such configuration.

Is that not enough?

Peter
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Check for maximum vcpus exceeding cpu topology
Posted by Madhu Pavan 7 years ago

On 02/07/2017 06:41 PM, Peter Krempa wrote:
> On Tue, Feb 07, 2017 at 07:56:29 -0500, Kothapally Madhu Pavan wrote:
>> This patch will prevent guest to start when the maximum
>> vcpus are greater than cpu topology limit. Currently similar
>> checks do exist only during setvcpus. The patch adds the
>> missing check. The c9cb35c255222 reverted similar check to
>> avoid older configs from vanishing. The current patch adds
>> it in domain validation part to be consistent with the
>> original intent.
>>
>> Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com>
>> ---
>>   src/qemu/qemu_process.c |    7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 184440d..f0d42b8 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -3738,6 +3738,13 @@ qemuValidateCpuCount(virDomainDefPtr def,
>>           return -1;
>>       }
>>   
>> +    if (def->cpu->sockets && virDomainDefGetVcpusMax(def) >
>> +        def->cpu->sockets * def->cpu->cores * def->cpu->threads) {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                       _("Maximum CPUs greater than topology limit"));
>> +        return -1;
>> +    }
> This exact logic is done in qemuDomainDefValidate:
>
> +    if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 &&
> +        topologycpus != virDomainDefGetVcpusMax(def)) {
> +        /* presence of query-hotpluggable-cpus should be a good enough witness */
> +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("CPU topology doesn't match maximum vcpu count"));
> +            goto cleanup;
>           }
>       }
>
> See commit 043ba4a40a4ae26cf616146d0d1c129d65b156b8. The only difference
> is that the code is validated only for certain versions of qemu known to
> reject such configuration.
>
> Is that not enough?
<vcpu placement='static'>64</vcpu>
<cpu>
     <topology sockets='1' cores='4' threads='8'/>
</cpu>

# virsh start one-0
error: Failed to start domain one-0
error: internal error: process exited while connecting to monitor: 
2017-02-08T06:41:59.615692Z qemu-system-ppc64: cpu topology: sockets (1) 
* cores (4) * threads (8) < smp_cpus (64)

I have qemu version 2.5.0 which doesn't have cpu hotplug capability 
hence I get this error. Any ways we can improve and error out using libvirt?

I tried with qemu-2.4.1 and still see the error, from this I can say smp 
topology check is maintained by qemu even before cpu hotplug feature is 
added.

When we had vcpu count validation in XML parser, domains with above 
mentioned config were rejected irrespective of qemu capabilities.

Thanks,
Madhu.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Check for maximum vcpus exceeding cpu topology
Posted by Peter Krempa 7 years ago
On Mon, Feb 20, 2017 at 17:54:48 +0530, Madhu Pavan wrote:
> 
> 
> On 02/07/2017 06:41 PM, Peter Krempa wrote:
> > On Tue, Feb 07, 2017 at 07:56:29 -0500, Kothapally Madhu Pavan wrote:
> > > This patch will prevent guest to start when the maximum
> > > vcpus are greater than cpu topology limit. Currently similar
> > > checks do exist only during setvcpus. The patch adds the
> > > missing check. The c9cb35c255222 reverted similar check to
> > > avoid older configs from vanishing. The current patch adds
> > > it in domain validation part to be consistent with the
> > > original intent.
> > > 
> > > Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com>
> > > ---
> > >   src/qemu/qemu_process.c |    7 +++++++
> > >   1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > > index 184440d..f0d42b8 100644
> > > --- a/src/qemu/qemu_process.c
> > > +++ b/src/qemu/qemu_process.c
> > > @@ -3738,6 +3738,13 @@ qemuValidateCpuCount(virDomainDefPtr def,
> > >           return -1;
> > >       }
> > > +    if (def->cpu->sockets && virDomainDefGetVcpusMax(def) >
> > > +        def->cpu->sockets * def->cpu->cores * def->cpu->threads) {
> > > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > +                       _("Maximum CPUs greater than topology limit"));
> > > +        return -1;
> > > +    }
> > This exact logic is done in qemuDomainDefValidate:
> > 
> > +    if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 &&
> > +        topologycpus != virDomainDefGetVcpusMax(def)) {
> > +        /* presence of query-hotpluggable-cpus should be a good enough witness */
> > +        if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) {
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("CPU topology doesn't match maximum vcpu count"));
> > +            goto cleanup;
> >           }
> >       }
> > 
> > See commit 043ba4a40a4ae26cf616146d0d1c129d65b156b8. The only difference
> > is that the code is validated only for certain versions of qemu known to
> > reject such configuration.
> > 
> > Is that not enough?
> <vcpu placement='static'>64</vcpu>
> <cpu>
>     <topology sockets='1' cores='4' threads='8'/>
> </cpu>
> 
> # virsh start one-0
> error: Failed to start domain one-0
> error: internal error: process exited while connecting to monitor:
> 2017-02-08T06:41:59.615692Z qemu-system-ppc64: cpu topology: sockets (1) *
> cores (4) * threads (8) < smp_cpus (64)
> 
> I have qemu version 2.5.0 which doesn't have cpu hotplug capability hence I
> get this error. Any ways we can improve and error out using libvirt?
> 
> I tried with qemu-2.4.1 and still see the error, from this I can say smp
> topology check is maintained by qemu even before cpu hotplug feature is
> added.

Well, the fun part is that the check was added in two commits in two
different versions of qemu:

qemu checks that topologyvcpus < cpus since

commit ec2cbbdd80463efd4bc81a9d1362a2acb3097a21
Author: Eduardo Habkost <ehabkost@redhat.com>
Date:   Fri Dec 19 00:59:47 2014 -0200

    vl: Don't silently change topology when all -smp options were set
    
    QEMU tries to change the "threads" option even if it was explicitly set
    in the command-line, and it shouldn't do that.

[...]

v2.2.0-399-gec2cbbdd80

and then later the check for topologyvcpus > cpus was added 

commit a32ef3bfc12c8d0588f43f74dcc5280885bbdb30
Author: Thomas Huth <thuth@redhat.com>
Date:   Wed Jul 22 15:59:50 2015 +0200

    vl: Add another sanity check to smp_parse() function
    
    The code in smp_parse already checks the topology information for
    sockets * cores * threads < cpus and bails out with an error in
    that case. However, it is still possible to supply a bad configuration
    the other way round, e.g. with:

v2.4.0-952-ga32ef3bfc1

> When we had vcpu count validation in XML parser, domains with above
> mentioned config were rejected irrespective of qemu capabilities.

That is wrong. We accepted such configurations and there are qemu
versions which accepted such configuration previously which should still
be supported by libvirt.

I think that it would be possible to somewhat justify the breakage of
such invalid configuration, but since qemu already errors out for very
old versions I don't see a compelling reason to do so.

Since there is no other sane witness for libvirt which would allow us to
check that the qemu version has already the checks in place I decided to
use the cpu hotplug capability since it's the closest.

If you know of a better witness which would allow us to know which qemu
versions already rejects invalid topology you are welcome to fix the
existing condition. Note that version number checks are not acceptable.

Peter

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Check for maximum vcpus exceeding cpu topology
Posted by Daniel P. Berrange 7 years, 1 month ago
On Tue, Feb 07, 2017 at 07:56:29AM -0500, Kothapally Madhu Pavan wrote:
> This patch will prevent guest to start when the maximum
> vcpus are greater than cpu topology limit. Currently similar
> checks do exist only during setvcpus. The patch adds the
> missing check. The c9cb35c255222 reverted similar check to
> avoid older configs from vanishing. The current patch adds
> it in domain validation part to be consistent with the
> original intent.
> 
> Signed-off-by: Kothapally Madhu Pavan <kmp@linux.vnet.ibm.com>
> ---
>  src/qemu/qemu_process.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 184440d..f0d42b8 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -3738,6 +3738,13 @@ qemuValidateCpuCount(virDomainDefPtr def,
>          return -1;
>      }
>  
> +    if (def->cpu->sockets && virDomainDefGetVcpusMax(def) >
> +        def->cpu->sockets * def->cpu->cores * def->cpu->threads) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("Maximum CPUs greater than topology limit"));
> +        return -1;
> +    }

This looks wrong.  def->cpu->sockets is sockets-per-NUMA node.
So you're comparing the total number of VCPUS against the CPUs
permitted in a single guest NUMA node. 


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

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