Re: [libvirt] [PATCH 4/4] Check the kvm host cpu max limits in virConnectGetDomainCapabilities()

2016-06-23 Thread Andrea Bolognani 
On Wed, 2016-06-15 at 09:58 +, 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

  

for the 'isapc' or 'xenpv' machine types.


-- 
Andrea Bolognani / Red Hat / Virtualization

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

Re: [libvirt] [PATCH 4/4] Check the kvm host cpu max limits in virConnectGetDomainCapabilities()

2016-06-22 Thread Andrea Bolognani 
On Wed, 2016-06-15 at 09:58 +, Shivaprasad G Bhat wrote:
>  
>vcpu
> -  The maximum number of supported virtual CPUs
> +  The maximum number of supported virtual CPUs. The suggested 
> attribute if present, gives the recommended
> maximum vcpus for the KVM host.
>  

Rewrite as

  Information about the number of virtual CPUs that can be
  assigned to a guest. The max attribute contains
  the maximum number of virtual CPUs; the
  suggested 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, "\n", caps->maxvcpus);
> -
> +if (caps->maxvcpus) {
> +if (!caps->suggestedvcpus)
> +virBufferAsprintf(buf, "\n", caps->maxvcpus);
> +else
> +virBufferAsprintf(buf, "\n",
> +  caps->maxvcpus, caps->suggestedvcpus);
> +}

You could change this to

  if (caps->maxvcpus) {
  virBufferAsprintf(buf, "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 = >os;
>  virDomainCapsDeviceDiskPtr disk = >disk;
>  virDomainCapsDeviceHostdevPtr hostdev = >hostdev;
>  virDomainCapsDeviceGraphicsPtr graphics = >graphics;
>  virDomainCapsDeviceVideoPtr video = >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

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

[libvirt] [PATCH 4/4] Check the kvm host cpu max limits in virConnectGetDomainCapabilities()

2016-06-15 Thread Shivaprasad G Bhat 
The qemu limit and host limit both should be considered for
the domain vcpu max limits.

Signed-off-by: Shivaprasad G Bhat 
---
 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 @@
 
 domainCapabilities
   ...
-  vcpu max='255'/
+  vcpu max='255' suggested='96'/
   ...
 /domainCapabilities
 
 
 
   vcpu
-  The maximum number of supported virtual CPUs
+  The maximum number of supported virtual CPUs. The suggested 
attribute if present, gives the recommended maximum vcpus for the KVM host.
 
 
 BIOS bootloader
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, "%s\n", caps->machine);
 virBufferAsprintf(buf, "%s\n", arch_str);
 
-if (caps->maxvcpus)
-virBufferAsprintf(buf, "\n", caps->maxvcpus);
-
+if (caps->maxvcpus) {
+if (!caps->suggestedvcpus)
+virBufferAsprintf(buf, "\n", caps->maxvcpus);
+else
+virBufferAsprintf(buf, "\n",
+  caps->maxvcpus, caps->suggestedvcpus);
+}
 virDomainCapsOSFormat(buf, >os);
 
 virBufferAddLit(buf, "\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 = >os;
 virDomainCapsDeviceDiskPtr disk = >disk;
 virDomainCapsDeviceHostdevPtr hostdev = >hostdev;
 virDomainCapsDeviceGraphicsPtr graphics = >graphics;
 virDomainCapsDeviceVideoPtr video = >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