Re: [libvirt] [PATCH] Fix performance & reliabilty of QMP probing

On 01/24/2013 01:34 PM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > This previous commit > > commit 1a50ba2cb07d8bb2aa724062889deb9efd7ad9e9 > Author: Viktor Mihajlovski <mihajlov(a)linux.vnet.ibm.com&gt; > 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(a)redhat.com&gt; > --- > 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)