[libvirt] [PATCH v2 0/3] Fix the domain capabilities wrt maxvcpus
This series drops the suggested cpu in the domcapabilties output which was implemented in previous version. So, this has just the fix for the domcapabililties output wrt maxvcpus. I'll follow up this series with suggested cpus and the virsh maxvcpus fix. The previous version is available here. http://www.redhat.com/archives/libvir-list/2016-June/msg00947.html --- Shivaprasad G Bhat (3): Rename kvmGetMaxVCPUs() to virHostCPUGetKVMMaxVCPUs() Check the kvm host cpu max limits in virConnectGetDomainCapabilities Document to not rely on virConnectGetMaxVcpus API src/libvirt-host.c | 5 +++- src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 11 ++++++--- src/qemu/qemu_capabilities.h | 3 ++ src/qemu/qemu_driver.c | 54 ++---------------------------------------- src/util/virhostcpu.c | 36 ++++++++++++++++++++++++++++ src/util/virhostcpu.h | 2 ++ tests/domaincapstest.c | 3 ++ 8 files changed, 57 insertions(+), 58 deletions(-) -- Signature
This kvmGetMaxVCPUs() needs to be used at two different places so move it to utils with appropriate name and mark it as private global now. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 52 +--------------------------------------------- src/util/virhostcpu.c | 36 ++++++++++++++++++++++++++++++++ src/util/virhostcpu.h | 2 ++ 4 files changed, 40 insertions(+), 51 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ef30f7f..a9eda05 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1072,6 +1072,7 @@ virLogManagerNew; nodeCapsInitNUMA; nodeGetInfo; virHostCPUGetCount; +virHostCPUGetKVMMaxVCPUs; virHostCPUGetMap; virHostCPUGetOnlineBitmap; virHostCPUGetPresentBitmap; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dd3d624..4e6e4c9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -124,24 +124,6 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_GUEST_VCPU_MAX_ID 4096 -#if HAVE_LINUX_KVM_H -# include <linux/kvm.h> -#endif - -/* device for kvm ioctls */ -#define KVM_DEVICE "/dev/kvm" - -/* add definitions missing in older linux/kvm.h */ -#ifndef KVMIO -# define KVMIO 0xAE -#endif -#ifndef KVM_CHECK_EXTENSION -# define KVM_CHECK_EXTENSION _IO(KVMIO, 0x03) -#endif -#ifndef KVM_CAP_NR_VCPUS -# define KVM_CAP_NR_VCPUS 9 /* returns max vcpus per vm */ -#endif - #define QEMU_NB_BLKIO_PARAM 6 #define QEMU_NB_BANDWIDTH_PARAM 7 @@ -1261,38 +1243,6 @@ static int qemuConnectIsAlive(virConnectPtr conn ATTRIBUTE_UNUSED) } -static int -kvmGetMaxVCPUs(void) -{ - int fd; - int ret; - - if ((fd = open(KVM_DEVICE, O_RDONLY)) < 0) { - virReportSystemError(errno, _("Unable to open %s"), KVM_DEVICE); - return -1; - } - -#ifdef KVM_CAP_MAX_VCPUS - /* at first try KVM_CAP_MAX_VCPUS to determine the maximum count */ - if ((ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS)) > 0) - goto cleanup; -#endif /* KVM_CAP_MAX_VCPUS */ - - /* as a fallback get KVM_CAP_NR_VCPUS (the recommended maximum number of - * vcpus). Note that on most machines this is set to 160. */ - if ((ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS)) > 0) - goto cleanup; - - /* if KVM_CAP_NR_VCPUS doesn't exist either, kernel documentation states - * that 4 should be used as the maximum number of cpus */ - ret = 4; - - cleanup: - VIR_FORCE_CLOSE(fd); - return ret; -} - - static char * qemuConnectGetSysinfo(virConnectPtr conn, unsigned int flags) { @@ -1330,7 +1280,7 @@ qemuConnectGetMaxVcpus(virConnectPtr conn ATTRIBUTE_UNUSED, const char *type) return 16; if (STRCASEEQ(type, "kvm")) - return kvmGetMaxVCPUs(); + return virHostCPUGetKVMMaxVCPUs(); if (STRCASEEQ(type, "kqemu")) return 1; diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index f38fbec..a528af7 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -37,6 +37,7 @@ #if HAVE_LINUX_KVM_H # include <linux/kvm.h> #endif +#define KVM_DEVICE "/dev/kvm" #if defined(__FreeBSD__) || defined(__APPLE__) # include <sys/time.h> @@ -1286,3 +1287,38 @@ virHostCPUGetThreadsPerSubcore(virArch arch ATTRIBUTE_UNUSED) } #endif /* HAVE_LINUX_KVM_H && defined(KVM_CAP_PPC_SMT) */ + +#ifndef KVM_CAP_NR_VCPUS +# define KVM_CAP_NR_VCPUS 9 /* returns max vcpus per vm */ +#endif + +int +virHostCPUGetKVMMaxVCPUs(void) +{ + int fd; + int ret; + + if ((fd = open(KVM_DEVICE, O_RDONLY)) < 0) { + virReportSystemError(errno, _("Unable to open %s"), KVM_DEVICE); + return -1; + } + +#ifdef KVM_CAP_MAX_VCPUS + /* at first try KVM_CAP_MAX_VCPUS to determine the maximum count */ + if ((ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_MAX_VCPUS)) > 0) + goto cleanup; +#endif /* KVM_CAP_MAX_VCPUS */ + + /* as a fallback get KVM_CAP_NR_VCPUS (the recommended maximum number of + * vcpus). Note that on most machines this is set to 160. */ + if ((ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS)) > 0) + goto cleanup; + + /* if KVM_CAP_NR_VCPUS doesn't exist either, kernel documentation states + * that 4 should be used as the maximum number of cpus */ + ret = 4; + + cleanup: + VIR_FORCE_CLOSE(fd); + return ret; +} diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h index e5ffc70..bc9cf98 100644 --- a/src/util/virhostcpu.h +++ b/src/util/virhostcpu.h @@ -51,4 +51,6 @@ int virHostCPUGetInfo(virArch hostarch, unsigned int *cores, unsigned int *threads); +int virHostCPUGetKVMMaxVCPUs(void); + #endif /* __VIR_HOSTCPU_H__*/
On Fri, 2016-06-24 at 20:33 +0530, Shivaprasad G Bhat wrote:
-/* add definitions missing in older linux/kvm.h */ -#ifndef KVMIO -# define KVMIO 0xAE -#endif -#ifndef KVM_CHECK_EXTENSION -# define KVM_CHECK_EXTENSION _IO(KVMIO, 0x03) -#endif -#ifndef KVM_CAP_NR_VCPUS -# define KVM_CAP_NR_VCPUS 9 /* returns max vcpus per vm */ -#endif
[...]
+#ifndef KVM_CAP_NR_VCPUS +# define KVM_CAP_NR_VCPUS 9 /* returns max vcpus per vm */ +#endif
As mentioned during the previous round of reviews, you can't just get rid of code like that during what's supposed to be a simple code motion: you have to do so in a separate commit. I've tweaked the commit so that definitions are preserved, we'll clean up later. ACK and pushed. -- Andrea Bolognani / Red Hat / Virtualization
The qemu limit and host limit both should be considered for the domain vcpu max limits. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 11 ++++++++--- src/qemu/qemu_capabilities.h | 3 ++- src/qemu/qemu_driver.c | 2 +- tests/domaincapstest.c | 3 ++- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 01466fc..ff5ad19 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -38,6 +38,7 @@ #include "virbitmap.h" #include "virnodesuspend.h" #include "virnuma.h" +#include "virhostcpu.h" #include "qemu_monitor.h" #include "virstring.h" #include "qemu_hostdev.h" @@ -4336,16 +4337,20 @@ int virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps, virQEMUCapsPtr qemuCaps, virFirmwarePtr *firmwares, - size_t nfirmwares) + size_t nfirmwares, + virDomainVirtType virttype) { virDomainCapsOSPtr os = &domCaps->os; virDomainCapsDeviceDiskPtr disk = &domCaps->disk; virDomainCapsDeviceHostdevPtr hostdev = &domCaps->hostdev; virDomainCapsDeviceGraphicsPtr graphics = &domCaps->graphics; virDomainCapsDeviceVideoPtr video = &domCaps->video; - int maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine); - domCaps->maxvcpus = maxvcpus; + domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine); + if (virttype == VIR_DOMAIN_VIRT_KVM) { + int hostmaxvcpus = virHostCPUGetKVMMaxVCPUs(); + domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus); + } if (virQEMUCapsFillDomainOSCaps(os, firmwares, nfirmwares) < 0 || virQEMUCapsFillDomainDeviceDiskCaps(qemuCaps, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index affb639..9d891c8 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -492,6 +492,7 @@ int virQEMUCapsInitGuestFromBinary(virCapsPtr caps, int virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps, virQEMUCapsPtr qemuCaps, virFirmwarePtr *firmwares, - size_t nfirmwares); + size_t nfirmwares, + virDomainVirtType virttype); #endif /* __QEMU_CAPABILITIES_H__*/ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4e6e4c9..59b657b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18475,7 +18475,7 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, goto cleanup; if (virQEMUCapsFillDomainCaps(domCaps, qemuCaps, - cfg->firmwares, cfg->nfirmwares) < 0) + cfg->firmwares, cfg->nfirmwares, virttype) < 0) goto cleanup; ret = virDomainCapsFormat(domCaps); diff --git a/tests/domaincapstest.c b/tests/domaincapstest.c index ae31146..01ebfcc 100644 --- a/tests/domaincapstest.c +++ b/tests/domaincapstest.c @@ -129,7 +129,8 @@ fillQemuCaps(virDomainCapsPtr domCaps, if (virQEMUCapsFillDomainCaps(domCaps, qemuCaps, cfg->firmwares, - cfg->nfirmwares) < 0) + cfg->nfirmwares, + VIR_DOMAIN_VIRT_QEMU) < 0) goto cleanup; /* The function above tries to query host's KVM & VFIO capabilities by
On Fri, 2016-06-24 at 20:34 +0530, Shivaprasad G Bhat wrote:
The qemu limit and host limit both should be considered for the domain vcpu max limits. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 11 ++++++++--- src/qemu/qemu_capabilities.h | 3 ++- src/qemu/qemu_driver.c | 2 +- tests/domaincapstest.c | 3 ++- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 01466fc..ff5ad19 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -38,6 +38,7 @@ #include "virbitmap.h" #include "virnodesuspend.h" #include "virnuma.h" +#include "virhostcpu.h" #include "qemu_monitor.h" #include "virstring.h" #include "qemu_hostdev.h" @@ -4336,16 +4337,20 @@ int virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps, virQEMUCapsPtr qemuCaps, virFirmwarePtr *firmwares, - size_t nfirmwares) + size_t nfirmwares, + virDomainVirtType virttype)
I missed this the first time around, but passing virttype here is kinda pointless as it can be retrieved from domCaps->virttype. However, that would cause the test suite to depend on the host state, /dev/kvm in particular). So ACK to this approach for now, but there should be a follow-up patch that updates domaincapstest to access a mocked /dev/kvm and gets rid of this extra parameter.
{ virDomainCapsOSPtr os = &domCaps->os; virDomainCapsDeviceDiskPtr disk = &domCaps->disk; virDomainCapsDeviceHostdevPtr hostdev = &domCaps->hostdev; virDomainCapsDeviceGraphicsPtr graphics = &domCaps->graphics; virDomainCapsDeviceVideoPtr video = &domCaps->video; - int maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine); - domCaps->maxvcpus = maxvcpus; + domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine);
Line's too long, should have been split into two.
+ if (virttype == VIR_DOMAIN_VIRT_KVM) { + int hostmaxvcpus = virHostCPUGetKVMMaxVCPUs(); + domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus); + }
I mentioned during the first round of reviews that the value returned by virHostCPUGetKVMMaxVCPUs() should be checked, because it could be <0 if /dev/kvm could not be accessed. ACK, I've squashed in the following trivial diff and pushed the patch. diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index ff5ad19..28d5321 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4346,10 +4346,12 @@ virQEMUCapsFillDomainCaps(virDomainCapsPtr domCaps, virDomainCapsDeviceGraphicsPtr graphics = &domCaps->graphics; virDomainCapsDeviceVideoPtr video = &domCaps->video; - domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine); + domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, + domCaps->machine); if (virttype == VIR_DOMAIN_VIRT_KVM) { int hostmaxvcpus = virHostCPUGetKVMMaxVCPUs(); - domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus); + if (hostmaxvcpus >= 0) + domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus); } if (virQEMUCapsFillDomainOSCaps(os, firmwares, nfirmwares) < 0 || -- Andrea Bolognani / Red Hat / Virtualization
The API virConnectGetMaxVcpus doesn't really reflect the actual usable number of cpus as the maximum limits can be different for kvm and/or qemu. So update the documentation to use virConnectGetDomainCapabilities() instead. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/libvirt-host.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libvirt-host.c b/src/libvirt-host.c index 24277b7..2a3de03 100644 --- a/src/libvirt-host.c +++ b/src/libvirt-host.c @@ -313,7 +313,10 @@ virConnectGetSysinfo(virConnectPtr conn, unsigned int flags) * * Provides the maximum number of virtual CPUs supported for a guest VM of a * specific type. The 'type' parameter here corresponds to the 'type' - * attribute in the <domain> element of the XML. + * attribute in the <domain> element of the XML. This API doesn't take emulator + * limits into consideration, hence the returned value is not guaranteed to be + * usable. It is recommended to use virConnectGetDomainCapabilities() and look + * for "<vcpu max='...'>" in its output instead. * * Returns the maximum of virtual CPU or -1 in case of error. */
On Fri, 2016-06-24 at 20:34 +0530, Shivaprasad G Bhat wrote:
The API virConnectGetMaxVcpus doesn't really reflect the actual usable number of cpus as the maximum limits can be different for kvm and/or qemu. So update the documentation to use virConnectGetDomainCapabilities() instead.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/libvirt-host.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/libvirt-host.c b/src/libvirt-host.c index 24277b7..2a3de03 100644 --- a/src/libvirt-host.c +++ b/src/libvirt-host.c @@ -313,7 +313,10 @@ virConnectGetSysinfo(virConnectPtr conn, unsigned int flags) * * Provides the maximum number of virtual CPUs supported for a guest VM of a * specific type. The 'type' parameter here corresponds to the 'type' - * attribute in the <domain> element of the XML. + * attribute in the <domain> element of the XML. This API doesn't take emulator + * limits into consideration, hence the returned value is not guaranteed to be + * usable. It is recommended to use virConnectGetDomainCapabilities() and look + * for "<vcpu max='...'>" in its output instead. * * Returns the maximum of virtual CPU or -1 in case of error. */
ACK and pushed. -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Shivaprasad G Bhat