[libvirt] [PATCH] Fix performance & reliabilty of QMP probing
From: "Daniel P. Berrange" <berrange@redhat.com> This previous commit commit 1a50ba2cb07d8bb2aa724062889deb9efd7ad9e9 Author: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Date: Mon Nov 26 15:17:13 2012 +0100 qemu: Fix QMP Capabability Probing Failure which attempted to make sure the QEMU process used for probing ran as the right user id, caused serious performance regression and unreliability in probing. The -daemonize switch in QEMU guarantees that the monitor socket is present before the parent process exits. This means libvirtd is guaranteed to be able to connect immediately. By switching from -daemonize to the virCommandDaemonize API libvirtd was no longer synchronized with QEMU's startup process. The result was that the QEMU monitor failed to open and went into its 200ms sleep loop. This happened for all 25 binaries resulting in 5 seconds worth of sleeping at libvirtd startup. In addition sometimes when libvirt connected, QEMU would be partially initialized and crash causing total failure to probe that binary. This commit reverts the previous change, ensuring we do use the -daemonize flag to QEMU. Startup delay is cut from 7 seconds to 2 seconds on my machine, which is on a par with what it was prior to the capabilities rewrite. To deal with the fact that QEMU needs to be able to create the pidfile, we switch pidfile location fron runDir to libDir, which QEMU is guaranteed to be able to write to. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 50 ++++++++++++++++++++++++++++++-------------- src/qemu/qemu_capabilities.h | 3 +-- 2 files changed, 35 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 95fa3da..703179d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -38,6 +38,7 @@ #include "virbitmap.h" #include "virnodesuspend.h" #include "qemu_monitor.h" +#include "virtime.h" #include <sys/stat.h> #include <unistd.h> @@ -916,11 +917,19 @@ virCapsPtr qemuCapsInit(qemuCapsCachePtr cache) * so just probe for them all - we gracefully fail * if a qemu-system-$ARCH binary can't be found */ - for (i = 0 ; i < VIR_ARCH_LAST ; i++) + unsigned long long a, b; + for (i = 0 ; i < VIR_ARCH_LAST ; i++) { + unsigned long long start, end; + if (virTimeMillisNow(&start) < 0) + goto error; if (qemuCapsInitGuest(caps, cache, virArchFromHost(), i) < 0) goto error; + if (virTimeMillisNow(&end) < 0) + goto error; + VIR_DEBUG("Probed %s in %llums", virArchToString(i), end-start); + } /* QEMU Requires an emulator in the XML */ virCapabilitiesSetEmulatorRequired(caps); @@ -2291,7 +2300,6 @@ qemuCapsInitQMPBasic(qemuCapsPtr caps) static int qemuCapsInitQMP(qemuCapsPtr caps, const char *libDir, - const char *runDir, uid_t runUid, gid_t runGid) { @@ -2324,8 +2332,11 @@ qemuCapsInitQMP(qemuCapsPtr caps, /* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash * with a qemu domain called "capabilities" + * Normally we'd use runDir for pid files, but because we're using + * -daemonize we need QEMU to be allowed to create them, rather + * than libvirtd. So we're using libDir which QEMU can write to */ - if (virAsprintf(&pidfile, "%s/%s", runDir, "capabilities.pidfile") < 0) { + if (virAsprintf(&pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) { virReportOOMError(); goto cleanup; } @@ -2337,6 +2348,13 @@ qemuCapsInitQMP(qemuCapsPtr caps, VIR_DEBUG("Try to get caps via QMP caps=%p", caps); + /* + * We explicitly need to use -daemonize here, rather than + * virCommandDaemonize, because we need to synchronize + * with QEMU creating its monitor socket API. Using + * daemonize guarnatees control won't return to libvirt + * until the socket is present. + */ cmd = virCommandNewArgList(caps->binary, "-S", "-no-user-config", @@ -2344,14 +2362,14 @@ qemuCapsInitQMP(qemuCapsPtr caps, "-nographic", "-M", "none", "-qmp", monarg, + "-pidfile", pidfile, + "-daemonize", NULL); virCommandAddEnvPassCommon(cmd); virCommandClearCaps(cmd); hookData.runUid = runUid; hookData.runGid = runGid; virCommandSetPreExecHook(cmd, qemuCapsHook, &hookData); - virCommandSetPidFile(cmd, pidfile); - virCommandDaemonize(cmd); if (virCommandRun(cmd, &status) < 0) goto cleanup; @@ -2472,7 +2490,6 @@ cleanup: qemuCapsPtr qemuCapsNewForBinary(const char *binary, const char *libDir, - const char *runDir, uid_t runUid, gid_t runGid) { @@ -2502,12 +2519,14 @@ qemuCapsPtr qemuCapsNewForBinary(const char *binary, goto error; } - if ((rv = qemuCapsInitQMP(caps, libDir, runDir, runUid, runGid)) < 0) + if ((rv = qemuCapsInitQMP(caps, libDir, runUid, runGid)) < 0) goto error; - if (!caps->usedQMP && - qemuCapsInitHelp(caps, runUid, runGid) < 0) - goto error; + if (!caps->usedQMP) { + VIR_ERROR("Falling back to help"); + if (qemuCapsInitHelp(caps, runUid, runGid) < 0) + goto error; + } return caps; @@ -2542,8 +2561,9 @@ qemuCapsHashDataFree(void *payload, const void *key ATTRIBUTE_UNUSED) qemuCapsCachePtr -qemuCapsCacheNew(const char *libDir, const char *runDir, - uid_t runUid, gid_t runGid) +qemuCapsCacheNew(const char *libDir, + uid_t runUid, + gid_t runGid) { qemuCapsCachePtr cache; @@ -2561,8 +2581,7 @@ qemuCapsCacheNew(const char *libDir, const char *runDir, if (!(cache->binaries = virHashCreate(10, qemuCapsHashDataFree))) goto error; - if (!(cache->libDir = strdup(libDir)) || - !(cache->runDir = strdup(runDir))) { + if (!(cache->libDir = strdup(libDir))) { virReportOOMError(); goto error; } @@ -2594,7 +2613,7 @@ qemuCapsCacheLookup(qemuCapsCachePtr cache, const char *binary) if (!ret) { VIR_DEBUG("Creating capabilities for %s", binary); - ret = qemuCapsNewForBinary(binary, cache->libDir, cache->runDir, + ret = qemuCapsNewForBinary(binary, cache->libDir, cache->runUid, cache->runGid); if (ret) { VIR_DEBUG("Caching capabilities %p for %s", @@ -2634,7 +2653,6 @@ qemuCapsCacheFree(qemuCapsCachePtr cache) return; VIR_FREE(cache->libDir); - VIR_FREE(cache->runDir); virHashFree(cache->binaries); virMutexDestroy(&cache->lock); VIR_FREE(cache); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 089fa30..5279d07 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -179,7 +179,6 @@ qemuCapsPtr qemuCapsNew(void); qemuCapsPtr qemuCapsNewCopy(qemuCapsPtr caps); qemuCapsPtr qemuCapsNewForBinary(const char *binary, const char *libDir, - const char *runDir, uid_t runUid, gid_t runGid); @@ -219,7 +218,7 @@ int qemuCapsGetMachineTypesCaps(qemuCapsPtr caps, bool qemuCapsIsValid(qemuCapsPtr caps); -qemuCapsCachePtr qemuCapsCacheNew(const char *libDir, const char *runDir, +qemuCapsCachePtr qemuCapsCacheNew(const char *libDir, uid_t uid, gid_t gid); qemuCapsPtr qemuCapsCacheLookup(qemuCapsCachePtr cache, const char *binary); qemuCapsPtr qemuCapsCacheLookupCopy(qemuCapsCachePtr cache, const char *binary); -- 1.8.0.2
On 01/24/2013 11:34 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This commit reverts the previous change, ensuring we do use the -daemonize flag to QEMU. Startup delay is cut from 7 seconds to 2 seconds on my machine, which is on a par with what it was prior to the capabilities rewrite.
To deal with the fact that QEMU needs to be able to create the pidfile, we switch pidfile location fron runDir to libDir, which QEMU is guaranteed to be able to write to.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 50 ++++++++++++++++++++++++++++++-------------- src/qemu/qemu_capabilities.h | 3 +-- 2 files changed, 35 insertions(+), 18 deletions(-)
@@ -916,11 +917,19 @@ virCapsPtr qemuCapsInit(qemuCapsCachePtr cache) * so just probe for them all - we gracefully fail * if a qemu-system-$ARCH binary can't be found */ - for (i = 0 ; i < VIR_ARCH_LAST ; i++) + unsigned long long a, b;
What are 'a' and 'b' for?
+ for (i = 0 ; i < VIR_ARCH_LAST ; i++) { + unsigned long long start, end; + if (virTimeMillisNow(&start) < 0) + goto error; if (qemuCapsInitGuest(caps, cache, virArchFromHost(), i) < 0) goto error; + if (virTimeMillisNow(&end) < 0) + goto error; + VIR_DEBUG("Probed %s in %llums", virArchToString(i), end-start);
spaces around '-'
+ }
/* QEMU Requires an emulator in the XML */ virCapabilitiesSetEmulatorRequired(caps); @@ -2291,7 +2300,6 @@ qemuCapsInitQMPBasic(qemuCapsPtr caps) static int qemuCapsInitQMP(qemuCapsPtr caps, const char *libDir, - const char *runDir, uid_t runUid, gid_t runGid) { @@ -2324,8 +2332,11 @@ qemuCapsInitQMP(qemuCapsPtr caps,
/* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash * with a qemu domain called "capabilities" + * Normally we'd use runDir for pid files, but because we're using + * -daemonize we need QEMU to be allowed to create them, rather + * than libvirtd. So we're using libDir which QEMU can write to */ - if (virAsprintf(&pidfile, "%s/%s", runDir, "capabilities.pidfile") < 0) { + if (virAsprintf(&pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) {
Will this change work across an upgrade? That is, if I have a qemu already running under libvirtd with the pid file in the old location, and then restart to a newer libvirtd, do we ever try to read the pidfile again, and fail because it is not in the right location?
+ /* + * We explicitly need to use -daemonize here, rather than + * virCommandDaemonize, because we need to synchronize + * with QEMU creating its monitor socket API. Using + * daemonize guarnatees control won't return to libvirt
s/guarnatees/guarantees/ -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Jan 24, 2013 at 12:57:48PM -0700, Eric Blake wrote:
On 01/24/2013 11:34 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com> @@ -916,11 +917,19 @@ virCapsPtr qemuCapsInit(qemuCapsCachePtr cache) * so just probe for them all - we gracefully fail * if a qemu-system-$ARCH binary can't be found */ - for (i = 0 ; i < VIR_ARCH_LAST ; i++) + unsigned long long a, b;
What are 'a' and 'b' for?
Left over debug code.
+ for (i = 0 ; i < VIR_ARCH_LAST ; i++) { + unsigned long long start, end; + if (virTimeMillisNow(&start) < 0) + goto error; if (qemuCapsInitGuest(caps, cache, virArchFromHost(), i) < 0) goto error; + if (virTimeMillisNow(&end) < 0) + goto error; + VIR_DEBUG("Probed %s in %llums", virArchToString(i), end-start);
spaces around '-'
Opps, all this left over debug code should have been removed.
+ }
/* QEMU Requires an emulator in the XML */ virCapabilitiesSetEmulatorRequired(caps); @@ -2291,7 +2300,6 @@ qemuCapsInitQMPBasic(qemuCapsPtr caps) static int qemuCapsInitQMP(qemuCapsPtr caps, const char *libDir, - const char *runDir, uid_t runUid, gid_t runGid) { @@ -2324,8 +2332,11 @@ qemuCapsInitQMP(qemuCapsPtr caps,
/* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash * with a qemu domain called "capabilities" + * Normally we'd use runDir for pid files, but because we're using + * -daemonize we need QEMU to be allowed to create them, rather + * than libvirtd. So we're using libDir which QEMU can write to */ - if (virAsprintf(&pidfile, "%s/%s", runDir, "capabilities.pidfile") < 0) { + if (virAsprintf(&pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) {
Will this change work across an upgrade? That is, if I have a qemu already running under libvirtd with the pid file in the old location, and then restart to a newer libvirtd, do we ever try to read the pidfile again, and fail because it is not in the right location?
This code is only executed while probing capabilities, so the QEMU is only running for a fraction of the second. The actual QEMU VM behaviour is unchanged, so there's no upgrade issue. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/24/2013 01:34 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This previous commit
commit 1a50ba2cb07d8bb2aa724062889deb9efd7ad9e9 Author: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Date: Mon Nov 26 15:17:13 2012 +0100
qemu: Fix QMP Capabability Probing Failure
which attempted to make sure the QEMU process used for probing ran as the right user id, caused serious performance regression and unreliability in probing. The -daemonize switch in QEMU guarantees that the monitor socket is present before the parent process exits. This means libvirtd is guaranteed to be able to connect immediately. By switching from -daemonize to the virCommandDaemonize API libvirtd was no longer synchronized with QEMU's startup process. The result was that the QEMU monitor failed to open and went into its 200ms sleep loop. This happened for all 25 binaries resulting in 5 seconds worth of sleeping at libvirtd startup. In addition sometimes when libvirt connected, QEMU would be partially initialized and crash causing total failure to probe that binary.
This commit reverts the previous change, ensuring we do use the -daemonize flag to QEMU. Startup delay is cut from 7 seconds to 2 seconds on my machine, which is on a par with what it was prior to the capabilities rewrite.
To deal with the fact that QEMU needs to be able to create the pidfile, we switch pidfile location fron runDir to libDir, which QEMU is guaranteed to be able to write to.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 50 ++++++++++++++++++++++++++++++-------------- src/qemu/qemu_capabilities.h | 3 +-- 2 files changed, 35 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 95fa3da..703179d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -38,6 +38,7 @@ #include "virbitmap.h" #include "virnodesuspend.h" #include "qemu_monitor.h" +#include "virtime.h"
#include <sys/stat.h> #include <unistd.h> @@ -916,11 +917,19 @@ virCapsPtr qemuCapsInit(qemuCapsCachePtr cache) * so just probe for them all - we gracefully fail * if a qemu-system-$ARCH binary can't be found */ - for (i = 0 ; i < VIR_ARCH_LAST ; i++) + unsigned long long a, b; + for (i = 0 ; i < VIR_ARCH_LAST ; i++) { + unsigned long long start, end; + if (virTimeMillisNow(&start) < 0) + goto error; if (qemuCapsInitGuest(caps, cache, virArchFromHost(), i) < 0) goto error; + if (virTimeMillisNow(&end) < 0) + goto error; + VIR_DEBUG("Probed %s in %llums", virArchToString(i), end-start); + }
Did you intend to leave in this debug code? (if you did, you need to move the definition of a & b up to the top of the block, and maybe rename them to something more descriptive)
/* QEMU Requires an emulator in the XML */ virCapabilitiesSetEmulatorRequired(caps); @@ -2291,7 +2300,6 @@ qemuCapsInitQMPBasic(qemuCapsPtr caps) static int qemuCapsInitQMP(qemuCapsPtr caps, const char *libDir, - const char *runDir, uid_t runUid, gid_t runGid) { @@ -2324,8 +2332,11 @@ qemuCapsInitQMP(qemuCapsPtr caps,
/* ".pidfile" suffix is used rather than ".pid" to avoid a possible clash * with a qemu domain called "capabilities" + * Normally we'd use runDir for pid files, but because we're using + * -daemonize we need QEMU to be allowed to create them, rather + * than libvirtd. So we're using libDir which QEMU can write to */ - if (virAsprintf(&pidfile, "%s/%s", runDir, "capabilities.pidfile") < 0) { + if (virAsprintf(&pidfile, "%s/%s", libDir, "capabilities.pidfile") < 0) { virReportOOMError(); goto cleanup; } @@ -2337,6 +2348,13 @@ qemuCapsInitQMP(qemuCapsPtr caps,
VIR_DEBUG("Try to get caps via QMP caps=%p", caps);
+ /* + * We explicitly need to use -daemonize here, rather than + * virCommandDaemonize, because we need to synchronize + * with QEMU creating its monitor socket API. Using + * daemonize guarnatees control won't return to libvirt
s/guarnatees/guarantees/
+ * until the socket is present. + */ cmd = virCommandNewArgList(caps->binary, "-S", "-no-user-config", @@ -2344,14 +2362,14 @@ qemuCapsInitQMP(qemuCapsPtr caps, "-nographic", "-M", "none", "-qmp", monarg, + "-pidfile", pidfile, + "-daemonize", NULL); virCommandAddEnvPassCommon(cmd); virCommandClearCaps(cmd); hookData.runUid = runUid; hookData.runGid = runGid; virCommandSetPreExecHook(cmd, qemuCapsHook, &hookData); - virCommandSetPidFile(cmd, pidfile); - virCommandDaemonize(cmd);
if (virCommandRun(cmd, &status) < 0) goto cleanup; @@ -2472,7 +2490,6 @@ cleanup:
qemuCapsPtr qemuCapsNewForBinary(const char *binary, const char *libDir, - const char *runDir, uid_t runUid, gid_t runGid) { @@ -2502,12 +2519,14 @@ qemuCapsPtr qemuCapsNewForBinary(const char *binary, goto error; }
- if ((rv = qemuCapsInitQMP(caps, libDir, runDir, runUid, runGid)) < 0) + if ((rv = qemuCapsInitQMP(caps, libDir, runUid, runGid)) < 0) goto error;
- if (!caps->usedQMP && - qemuCapsInitHelp(caps, runUid, runGid) < 0) - goto error; + if (!caps->usedQMP) { + VIR_ERROR("Falling back to help"); + if (qemuCapsInitHelp(caps, runUid, runGid) < 0) + goto error; + }
return caps;
@@ -2542,8 +2561,9 @@ qemuCapsHashDataFree(void *payload, const void *key ATTRIBUTE_UNUSED)
qemuCapsCachePtr -qemuCapsCacheNew(const char *libDir, const char *runDir, - uid_t runUid, gid_t runGid) +qemuCapsCacheNew(const char *libDir, + uid_t runUid, + gid_t runGid) { qemuCapsCachePtr cache;
@@ -2561,8 +2581,7 @@ qemuCapsCacheNew(const char *libDir, const char *runDir,
if (!(cache->binaries = virHashCreate(10, qemuCapsHashDataFree))) goto error; - if (!(cache->libDir = strdup(libDir)) || - !(cache->runDir = strdup(runDir))) { + if (!(cache->libDir = strdup(libDir))) { virReportOOMError(); goto error; } @@ -2594,7 +2613,7 @@ qemuCapsCacheLookup(qemuCapsCachePtr cache, const char *binary) if (!ret) { VIR_DEBUG("Creating capabilities for %s", binary); - ret = qemuCapsNewForBinary(binary, cache->libDir, cache->runDir, + ret = qemuCapsNewForBinary(binary, cache->libDir, cache->runUid, cache->runGid); if (ret) { VIR_DEBUG("Caching capabilities %p for %s", @@ -2634,7 +2653,6 @@ qemuCapsCacheFree(qemuCapsCachePtr cache) return;
VIR_FREE(cache->libDir); - VIR_FREE(cache->runDir); virHashFree(cache->binaries); virMutexDestroy(&cache->lock); VIR_FREE(cache); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 089fa30..5279d07 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -179,7 +179,6 @@ qemuCapsPtr qemuCapsNew(void); qemuCapsPtr qemuCapsNewCopy(qemuCapsPtr caps); qemuCapsPtr qemuCapsNewForBinary(const char *binary, const char *libDir, - const char *runDir, uid_t runUid, gid_t runGid);
@@ -219,7 +218,7 @@ int qemuCapsGetMachineTypesCaps(qemuCapsPtr caps, bool qemuCapsIsValid(qemuCapsPtr caps);
-qemuCapsCachePtr qemuCapsCacheNew(const char *libDir, const char *runDir, +qemuCapsCachePtr qemuCapsCacheNew(const char *libDir, uid_t uid, gid_t gid); qemuCapsPtr qemuCapsCacheLookup(qemuCapsCachePtr cache, const char *binary); qemuCapsPtr qemuCapsCacheLookupCopy(qemuCapsCachePtr cache, const char *binary);
ACK with the above issues fixed.
On Thu, Jan 24, 2013 at 02:59:49PM -0500, Laine Stump wrote:
On 01/24/2013 01:34 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This previous commit
commit 1a50ba2cb07d8bb2aa724062889deb9efd7ad9e9 Author: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> Date: Mon Nov 26 15:17:13 2012 +0100
qemu: Fix QMP Capabability Probing Failure
which attempted to make sure the QEMU process used for probing ran as the right user id, caused serious performance regression and unreliability in probing. The -daemonize switch in QEMU guarantees that the monitor socket is present before the parent process exits. This means libvirtd is guaranteed to be able to connect immediately. By switching from -daemonize to the virCommandDaemonize API libvirtd was no longer synchronized with QEMU's startup process. The result was that the QEMU monitor failed to open and went into its 200ms sleep loop. This happened for all 25 binaries resulting in 5 seconds worth of sleeping at libvirtd startup. In addition sometimes when libvirt connected, QEMU would be partially initialized and crash causing total failure to probe that binary.
This commit reverts the previous change, ensuring we do use the -daemonize flag to QEMU. Startup delay is cut from 7 seconds to 2 seconds on my machine, which is on a par with what it was prior to the capabilities rewrite.
To deal with the fact that QEMU needs to be able to create the pidfile, we switch pidfile location fron runDir to libDir, which QEMU is guaranteed to be able to write to.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 50 ++++++++++++++++++++++++++++++-------------- src/qemu/qemu_capabilities.h | 3 +-- 2 files changed, 35 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 95fa3da..703179d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -38,6 +38,7 @@ #include "virbitmap.h" #include "virnodesuspend.h" #include "qemu_monitor.h" +#include "virtime.h"
#include <sys/stat.h> #include <unistd.h> @@ -916,11 +917,19 @@ virCapsPtr qemuCapsInit(qemuCapsCachePtr cache) * so just probe for them all - we gracefully fail * if a qemu-system-$ARCH binary can't be found */ - for (i = 0 ; i < VIR_ARCH_LAST ; i++) + unsigned long long a, b; + for (i = 0 ; i < VIR_ARCH_LAST ; i++) { + unsigned long long start, end; + if (virTimeMillisNow(&start) < 0) + goto error; if (qemuCapsInitGuest(caps, cache, virArchFromHost(), i) < 0) goto error; + if (virTimeMillisNow(&end) < 0) + goto error; + VIR_DEBUG("Probed %s in %llums", virArchToString(i), end-start); + }
Did you intend to leave in this debug code? (if you did, you need to move the definition of a & b up to the top of the block, and maybe rename them to something more descriptive)
No, it should have been removed. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/24/2013 07:34 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This commit reverts the previous change, ensuring we do use the -daemonize flag to QEMU. Startup delay is cut from 7 seconds to 2 seconds on my machine, which is on a par with what it was prior to the capabilities rewrite.
I have not that many QEMU instances, so I didn't see that degradation.
To deal with the fact that QEMU needs to be able to create the pidfile, we switch pidfile location fron runDir to libDir, which QEMU is guaranteed to be able to write to.
I tested your patch (with minor changes) and the QMP probing works without as expected with selinux, etc. Besides the debugging stuff that Eric and Laine mentioned you will also need to fix the CapsCacheNew signature hit in qemu_driver.c. I assume you have done that in your local tree but it's not in this patch. -- Mit freundlichen Grüßen/Kind Regards Viktor Mihajlovski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Fri, Jan 25, 2013 at 04:14:18PM +0100, Viktor Mihajlovski wrote:
On 01/24/2013 07:34 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This commit reverts the previous change, ensuring we do use the -daemonize flag to QEMU. Startup delay is cut from 7 seconds to 2 seconds on my machine, which is on a par with what it was prior to the capabilities rewrite.
I have not that many QEMU instances, so I didn't see that degradation.
To deal with the fact that QEMU needs to be able to create the pidfile, we switch pidfile location fron runDir to libDir, which QEMU is guaranteed to be able to write to.
I tested your patch (with minor changes) and the QMP probing works without as expected with selinux, etc. Besides the debugging stuff that Eric and Laine mentioned you will also need to fix the CapsCacheNew signature hit in qemu_driver.c. I assume you have done that in your local tree but it's not in this patch.
Oh that change accidentally crept into the following patch and has already been pushed Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Viktor Mihajlovski