[libvirt] [PATCH 0/4] Fix the domain capabilities wrt maxvcpus
This series addresses the comments to my patch in http://www.redhat.com/archives/libvir-list/2016-May/msg00218.html The v2 got quite a lot of criticism to not to change the virConnectGetMaxVCPUS() instead document to use the virConnectGetDomainCapabilities(). The virConnectGetDomainCapabilities() is extended to check for the host limits and the NR_CPUs are also returned. I am planning to follow up this series with possible fixes to maxvcpus command once this merged. --- Shivaprasad G Bhat (4): Document to not rely on virConnectGetMaxVcpus API Rename and move kvmGetMaxVCPUs to utils and extend it Check for VFIO too where the Legacy passthrough is checked Check the kvm host cpu max limits in virConnectGetDomainCapabilities() docs/formatdomaincaps.html.in | 4 +-- src/conf/domain_capabilities.c | 10 +++++-- src/conf/domain_capabilities.h | 1 + src/libvirt-host.c | 6 ++++ src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 13 +++++++-- src/qemu/qemu_capabilities.h | 3 +- src/qemu/qemu_driver.c | 57 +++------------------------------------- src/util/virhostcpu.c | 37 ++++++++++++++++++++++++++ src/util/virhostcpu.h | 7 +++++ tests/domaincapstest.c | 3 +- 11 files changed, 78 insertions(+), 64 deletions(-) -- Signature
The API virConnectGetMaxVcpus doesnt 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 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libvirt-host.c b/src/libvirt-host.c index 24277b7..f6b0c30 100644 --- a/src/libvirt-host.c +++ b/src/libvirt-host.c @@ -313,7 +313,11 @@ 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. The API doesn't take emulator + * limits into consideration. Hence the maximum reported here can be limited by + * the emulator max limits. It is recommended to use the + * virConnectGetDomainCapabilities() API specifying the emulator details to get + * the accurate results. * * Returns the maximum of virtual CPU or -1 in case of error. */
On Wed, 2016-06-15 at 09:56 +0000, Shivaprasad G Bhat wrote:
The API virConnectGetMaxVcpus doesnt really reflect the actual usable number
s/doesnt/doesn't/
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 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libvirt-host.c b/src/libvirt-host.c index 24277b7..f6b0c30 100644 --- a/src/libvirt-host.c +++ b/src/libvirt-host.c @@ -313,7 +313,11 @@ 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. The API doesn't take emulator + * limits into consideration. Hence the maximum reported here can be limited by + * the emulator max limits. It is recommended to use the + * virConnectGetDomainCapabilities() API specifying the emulator details to get + * the accurate results.
I'd reword this slightly, along the lines of 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. */
I feel like this patch should actually be last one in the series, after the preferred way to retrieve the information has actually been implemented. -- Andrea Bolognani Software Engineer - Virtualization Team
This function needs to be used at two different places and make it global now. Also, extend it to return the NR_CPUs when needed. 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 | 37 +++++++++++++++++++++++++++++++++ src/util/virhostcpu.h | 7 ++++++ 4 files changed, 46 insertions(+), 51 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e939de3..569f8e8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1066,6 +1066,7 @@ virLogManagerNew; nodeCapsInitNUMA; nodeGetInfo; virHostCPUGetCount; +virHostCPUGetKVMVCPUs; virHostCPUGetMap; virHostCPUGetOnlineBitmap; virHostCPUGetPresentBitmap; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a45fd55..65ef68c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -122,24 +122,6 @@ VIR_LOG_INIT("qemu.qemu_driver"); #define QEMU_SCHED_MIN_QUOTA 1000LL #define QEMU_SCHED_MAX_QUOTA 18446744073709551LL -#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 @@ -1269,38 +1251,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) { @@ -1338,7 +1288,7 @@ qemuConnectGetMaxVcpus(virConnectPtr conn ATTRIBUTE_UNUSED, const char *type) return 16; if (STRCASEEQ(type, "kvm")) - return kvmGetMaxVCPUs(); + return virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_MAXVCPUS); if (STRCASEEQ(type, "kqemu")) return 1; diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 00c09cd..5712eda 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> @@ -1297,3 +1298,39 @@ 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 +virHostCPUGetKVMVCPUs(virHostCPUKVMWrapperFlags flag) +{ + 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 (flag & VIR_HOSTCPU_KVM_MAXVCPUS && + (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 ((flag & VIR_HOSTCPU_KVM_MAXVCPUS || flag & VIR_HOSTCPU_KVM_NR_VCPUS) && + (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..c1b855a 100644 --- a/src/util/virhostcpu.h +++ b/src/util/virhostcpu.h @@ -51,4 +51,11 @@ int virHostCPUGetInfo(virArch hostarch, unsigned int *cores, unsigned int *threads); +typedef enum { + VIR_HOSTCPU_KVM_MAXVCPUS = (1 << 0), + VIR_HOSTCPU_KVM_NR_VCPUS = (1 << 1), +} virHostCPUKVMWrapperFlags; + +int virHostCPUGetKVMVCPUs(virHostCPUKVMWrapperFlags flag); + #endif /* __VIR_HOSTCPU_H__*/
On Wed, 2016-06-15 at 09:56 +0000, Shivaprasad G Bhat wrote:
This function needs to be used at two different places and make it global now. Also, extend it to return the NR_CPUs when needed. 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 | 37 +++++++++++++++++++++++++++++++++ src/util/virhostcpu.h | 7 ++++++ 4 files changed, 46 insertions(+), 51 deletions(-)
This should really be two patches: one moving the function to its new location while marking it as a private exported symbol, and the other one chaging its signature and behavior. Actually, make it three patches - see below.
-/* 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
We no longer support any distribution old enough not to have these definitions, so you can just drop them (as a separate patch).
+int +virHostCPUGetKVMVCPUs(virHostCPUKVMWrapperFlags flag) +{ + 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 (flag & VIR_HOSTCPU_KVM_MAXVCPUS && + (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 ((flag & VIR_HOSTCPU_KVM_MAXVCPUS || flag & VIR_HOSTCPU_KVM_NR_VCPUS) && + (ret = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_NR_VCPUS)) > 0) + goto cleanup;
You dropped a blank line here, and the "goto" is not indented properly
+ /* 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;
[...]
+typedef enum { + VIR_HOSTCPU_KVM_MAXVCPUS = (1 << 0), + VIR_HOSTCPU_KVM_NR_VCPUS = (1 << 1), +} virHostCPUKVMWrapperFlags; + +int virHostCPUGetKVMVCPUs(virHostCPUKVMWrapperFlags flag);
The naming for the function, and especially for the enum and its values, is IMHO kinda awkward after the change... We could have something like virHostCPUGetKVMMaxVCPUs() virHostCPUGetKVMRecommendedVCPUs() instead. The attached patch, which applies cleanly on top of your series, shows what I have in mind :) -- Andrea Bolognani Software Engineer - Virtualization Team
On PPC the Legacy patthrough is not supported and only VFIO is supported. So, the checks at places to confirm if the host is passthrough capable checks only Legacy, fix it. This is seen at only one place now. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65ef68c..6b316a0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18413,7 +18413,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, cfg = virQEMUDriverGetConfig(driver); - if (qemuHostdevHostSupportsPassthroughLegacy()) + if (qemuHostdevHostSupportsPassthroughLegacy() || + qemuHostdevHostSupportsPassthroughVFIO()) virttype = VIR_DOMAIN_VIRT_KVM; else virttype = VIR_DOMAIN_VIRT_QEMU;
On Wed, 2016-06-15 at 09:57 +0000, Shivaprasad G Bhat wrote:
On PPC the Legacy patthrough is not supported and only
s/patthrough/passthrough/
VFIO is supported. So, the checks at places to confirm if the host is passthrough capable checks only Legacy, fix it. This is seen at only one place now. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65ef68c..6b316a0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18413,7 +18413,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, cfg = virQEMUDriverGetConfig(driver); - if (qemuHostdevHostSupportsPassthroughLegacy()) + if (qemuHostdevHostSupportsPassthroughLegacy() || + qemuHostdevHostSupportsPassthroughVFIO()) virttype = VIR_DOMAIN_VIRT_KVM; else virttype = VIR_DOMAIN_VIRT_QEMU;
This looks like an unrelated bugfix. If that's the case, please yank it out of the series and submit it separately. Looks good otherwise :) -- Andrea Bolognani Software Engineer - Virtualization Team
On Wed, 2016-06-22 at 16:37 +0200, Andrea Bolognani wrote:
On Wed, 2016-06-15 at 09:57 +0000, Shivaprasad G Bhat wrote:
On PPC the Legacy patthrough is not supported and only
s/patthrough/passthrough/
VFIO is supported. So, the checks at places to confirm if the host is passthrough capable checks only Legacy, fix it. This is seen at only one place now. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 65ef68c..6b316a0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18413,7 +18413,8 @@ qemuConnectGetDomainCapabilities(virConnectPtr conn, cfg = virQEMUDriverGetConfig(driver); - if (qemuHostdevHostSupportsPassthroughLegacy()) + if (qemuHostdevHostSupportsPassthroughLegacy() || + qemuHostdevHostSupportsPassthroughVFIO()) virttype = VIR_DOMAIN_VIRT_KVM; else virttype = VIR_DOMAIN_VIRT_QEMU;
This looks like an unrelated bugfix. If that's the case, please yank it out of the series and submit it separately.
Looks good otherwise :)
Since you've confirmed (on IRC) that this is indeed an independed bugfix, and the patch is already good to go, I've amended the commit messages and pushed it. -- 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> --- docs/formatdomaincaps.html.in | 4 ++-- src/conf/domain_capabilities.c | 10 +++++++--- src/conf/domain_capabilities.h | 1 + src/qemu/qemu_capabilities.c | 13 ++++++++++--- src/qemu/qemu_capabilities.h | 3 ++- src/qemu/qemu_driver.c | 2 +- tests/domaincapstest.c | 3 ++- 7 files changed, 25 insertions(+), 11 deletions(-) diff --git a/docs/formatdomaincaps.html.in b/docs/formatdomaincaps.html.in index d5a8414..d28a5b6 100644 --- a/docs/formatdomaincaps.html.in +++ b/docs/formatdomaincaps.html.in @@ -86,14 +86,14 @@ <pre> <domainCapabilities> ... - <vcpu max='255'/> + <vcpu max='255' suggested='96'/> ... </domainCapabilities> </pre> <dl> <dt><code>vcpu</code></dt> - <dd>The maximum number of supported virtual CPUs</dd> + <dd>The maximum number of supported virtual CPUs. The suggested attribute if present, gives the recommended maximum vcpus for the KVM host.</dd> </dl> <h3><a name="elementsOSBIOS">BIOS bootloader</a></h3> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 1676f0e..452cad4 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -329,9 +329,13 @@ virDomainCapsFormatInternal(virBufferPtr buf, virBufferAsprintf(buf, "<machine>%s</machine>\n", caps->machine); virBufferAsprintf(buf, "<arch>%s</arch>\n", arch_str); - if (caps->maxvcpus) - virBufferAsprintf(buf, "<vcpu max='%d'/>\n", caps->maxvcpus); - + if (caps->maxvcpus) { + if (!caps->suggestedvcpus) + virBufferAsprintf(buf, "<vcpu max='%d'/>\n", caps->maxvcpus); + else + virBufferAsprintf(buf, "<vcpu max='%d' suggested='%d'/>\n", + caps->maxvcpus, caps->suggestedvcpus); + } virDomainCapsOSFormat(buf, &caps->os); virBufferAddLit(buf, "<devices>\n"); diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h index 492a9cf..f440436 100644 --- a/src/conf/domain_capabilities.h +++ b/src/conf/domain_capabilities.h @@ -112,6 +112,7 @@ struct _virDomainCaps { /* Some machine specific info */ int maxvcpus; + int suggestedvcpus; virDomainCapsOS os; virDomainCapsDeviceDisk disk; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 1ef5937..df10aa8 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" @@ -4333,16 +4334,22 @@ 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); + int hostmaxvcpus = 0; - domCaps->maxvcpus = maxvcpus; + domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine); + if (virttype == VIR_DOMAIN_VIRT_KVM) { + hostmaxvcpus = virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_MAXVCPUS); + domCaps->suggestedvcpus = virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_NR_VCPUS); + 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 f7ede4a..ffe07e5 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -494,6 +494,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 6b316a0..21aa053 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18485,7 +18485,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 9fb2c97..ea438e3 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 Wed, 2016-06-15 at 09:58 +0000, Shivaprasad G Bhat wrote:
<dl> <dt><code>vcpu</code></dt> - <dd>The maximum number of supported virtual CPUs</dd> + <dd>The maximum number of supported virtual CPUs. The suggested attribute if present, gives the recommended maximum vcpus for the KVM host.</dd> </dl>
Rewrite as Information about the number of virtual CPUs that can be assigned to a guest. The <code>max</code> attribute contains the maximum number of virtual CPUs; the <code>suggested</code> attribute, if present, contains the recommended number of virtual CPUs. making sure lines are at most 80 columns long. I would prefer to use "recommended" instead of "suggested" for the attribute name, and for all related variables, as the relevant comment in linux/kvm.h reads /* returns recommended max vcpus per vm */ and "to recommend" is generally stronger than "to suggest", ie. you should think twice before disregarding a recommendation.
- if (caps->maxvcpus) - virBufferAsprintf(buf, "<vcpu max='%d'/>\n", caps->maxvcpus); - + if (caps->maxvcpus) { + if (!caps->suggestedvcpus) + virBufferAsprintf(buf, "<vcpu max='%d'/>\n", caps->maxvcpus); + else + virBufferAsprintf(buf, "<vcpu max='%d' suggested='%d'/>\n", + caps->maxvcpus, caps->suggestedvcpus); + }
You could change this to if (caps->maxvcpus) { virBufferAsprintf(buf, "<vcpu max='%d'", caps->maxvcpus); if (caps->suggestedvcpus) virBufferAsprintf(buf, " suggested='%d'", caps->suggestedvcpus); virBufferAddLit(buf, "/>\n"); } but it's up to you, really.
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); + int hostmaxvcpus = 0; - domCaps->maxvcpus = maxvcpus; + domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps->machine); + if (virttype == VIR_DOMAIN_VIRT_KVM) { + hostmaxvcpus = virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_MAXVCPUS);
You can move the definition of the 'hostmaxvcpus' variable in here, it's not used in the outer scope anyway. You should also perform some error checking here: virHostCPUGetKVMVCPUs() can return a negative number, for example if /dev/kvm can't be accessed. If that happens, I'm not sure whether it would be better to simply ignore the failure and just report the QEMU limits, or if we should abort virQEMUCapsFillDomainCaps() altogether. Probably the former. Everything else looks good. -- Andrea Bolognani Software Engineer - Virtualization Team
On Wed, 2016-06-15 at 09:58 +0000, Shivaprasad G Bhat wrote:
- domCaps->maxvcpus = maxvcpus; + domCaps->maxvcpus = virQEMUCapsGetMachineMaxCpus(qemuCaps, domCaps- machine); + if (virttype == VIR_DOMAIN_VIRT_KVM) { + hostmaxvcpus = virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_MAXVCPUS); + domCaps->suggestedvcpus = virHostCPUGetKVMVCPUs(VIR_HOSTCPU_KVM_NR_VCPUS); + domCaps->maxvcpus = MIN(domCaps->maxvcpus, hostmaxvcpus); + }
Forgot to mention this yesterday: domCaps->suggestedvcpus will have to be capped by hostmaxvcpus as well, because otherwise you would end up with stuff like <vcpu max='1' suggested='160'/> for the 'isapc' or 'xenpv' machine types. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, 2016-06-15 at 09:54 +0000, Shivaprasad G Bhat wrote:
This series addresses the comments to my patch in http://www.redhat.com/archives/libvir-list/2016-May/msg00218.html The v2 got quite a lot of criticism to not to change the virConnectGetMaxVCPUS() instead document to use the virConnectGetDomainCapabilities(). The virConnectGetDomainCapabilities() is extended to check for the host limits and the NR_CPUs are also returned. I am planning to follow up this series with possible fixes to maxvcpus command once this merged.
Sorry for being late in reviewing this series. If you can post a v2 addressing my comments in the next couple of days, there's still time for it to be merged in time for 2.0.0 - the freeze is on Sunday. Please note that it doesn't apply cleanly on top of current master, but the conflict is easy enough to fix. -- Andrea Bolognani Software Engineer - Virtualization Team
On Wed, 2016-06-15 at 09:54 +0000, Shivaprasad G Bhat wrote:
This series addresses the comments to my patch in http://www.redhat.com/archives/libvir-list/2016-May/msg00218.html
The v2 got quite a lot of criticism to not to change the virConnectGetMaxVCPUS() instead document to use the virConnectGetDomainCapabilities(). The virConnectGetDomainCapabilities() is extended to check for the host limits and the NR_CPUs are also returned. I am planning to follow up this series with possible fixes to maxvcpus command once this merged. Sorry for being late in reviewing this series.
If you can post a v2 addressing my comments in the next couple of days, there's still time for it to be merged in time for 2.0.0 - the freeze is on Sunday. Thanks Andrea. As discussed with you on IRC, I am dropping my changes for "suggested" cpus in
On 06/22/2016 08:55 PM, Andrea Bolognani wrote: the next version and follow it up separately, so just the bug fix is easier for the build.
Please note that it doesn't apply cleanly on top of current master, but the conflict is easy enough to fix.
-- Andrea Bolognani Software Engineer - Virtualization Team
participants (2)
-
Andrea Bolognani -
Shivaprasad G Bhat