[libvirt] [PATCH v2 0/3] Add caching of QEMU probed capabilities
A followup to https://www.redhat.com/archives/libvir-list/2014-March/msg00297.html Probing capabilities takes 200-300ms per binary and we have as many as 26 binaries. This noticably slows down libvirtd startup. It does not look like performance of probing QEMU can be improved, so this series introduces caching of the capabilities information. So the first time libvirtd starts it'll be slow, but thereafter it is fast. The cache is invalidated any time the QEMU binary timestamp changes or the libvirtd binary or driver module timestamp changes. In v2: - Store timestamps in XML file instead of non-portable utimes() - Use ctime instead of mtime since the latter can be faked by package managers to go backwards in time. Daniel P. Berrange (3): Add helper APIs to track if libvirtd or loadable modules have changed Change QEMU capabilities cache to check ctime instead of mtime Cache result of QEMU capabilities extraction daemon/libvirtd.c | 2 + src/driver.c | 2 + src/libvirt_private.syms | 2 + src/qemu/qemu_capabilities.c | 442 +++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_driver.c | 1 + src/util/virutil.c | 23 +++ src/util/virutil.h | 4 + 8 files changed, 467 insertions(+), 11 deletions(-) -- 1.8.5.3
The future QEMU capabilities cache needs to be able to invalidate itself if the libvirtd binary or any loadable modules are changed on disk. Record the 'ctime' value for these binaries and provide helper APIs to query it. This approach assumes that if libvirt.so is changed, then libvirtd will also change, which should usually be the case with libtool's wrapper scripts that cause libvirtd to get re-linked Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/libvirtd.c | 2 ++ src/driver.c | 2 ++ src/libvirt_private.syms | 2 ++ src/util/virutil.c | 23 +++++++++++++++++++++++ src/util/virutil.h | 4 ++++ 5 files changed, 33 insertions(+) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 72f0e81..36adaf0 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1152,6 +1152,8 @@ int main(int argc, char **argv) { exit(EXIT_FAILURE); } + virUpdateSelfLastChanged(argv[0]); + if (strstr(argv[0], "lt-libvirtd") || strstr(argv[0], "/daemon/.libs/libvirtd")) { char *tmp = strrchr(argv[0], '/'); diff --git a/src/driver.c b/src/driver.c index ab2a253..721cbeb 100644 --- a/src/driver.c +++ b/src/driver.c @@ -74,6 +74,8 @@ virDriverLoadModule(const char *name) goto cleanup; } + virUpdateSelfLastChanged(modfile); + handle = dlopen(modfile, RTLD_NOW | RTLD_GLOBAL); if (!handle) { VIR_ERROR(_("failed to load module %s %s"), modfile, dlerror()); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 00e3d9c..f1607cd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1954,6 +1954,7 @@ virGetGroupID; virGetGroupList; virGetGroupName; virGetHostname; +virGetSelfLastChanged; virGetUnprivSGIOSysfsPath; virGetUserCacheDirectory; virGetUserConfigDirectory; @@ -1983,6 +1984,7 @@ virSetNonBlock; virSetUIDGID; virSetUIDGIDWithCaps; virStrIsPrint; +virUpdateSelfLastChanged; virValidateWWN; diff --git a/src/util/virutil.c b/src/util/virutil.c index 7a2fbb0..b6106fc 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2173,3 +2173,26 @@ bool virIsSUID(void) { return getuid() != geteuid(); } + + +static time_t selfLastChanged; + +time_t virGetSelfLastChanged(void) +{ + return selfLastChanged; +} + + +void virUpdateSelfLastChanged(const char *path) +{ + struct stat sb; + + if (stat(path, &sb) < 0) + return; + + if (sb.st_ctime > selfLastChanged) { + VIR_DEBUG("Setting self last changed to %lld for '%s'", + (long long)sb.st_ctime, path); + selfLastChanged = sb.st_ctime; + } +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 029265c..cffe1ed 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -194,4 +194,8 @@ const char *virGetEnvBlockSUID(const char *name); const char *virGetEnvAllowSUID(const char *name); bool virIsSUID(void); + +time_t virGetSelfLastChanged(void); +void virUpdateSelfLastChanged(const char *path); + #endif /* __VIR_UTIL_H__ */ -- 1.8.5.3
On 03/10/2014 10:54 AM, Daniel P. Berrange wrote:
The future QEMU capabilities cache needs to be able to invalidate itself if the libvirtd binary or any loadable modules are changed on disk. Record the 'ctime' value for these binaries and provide helper APIs to query it. This approach assumes that if libvirt.so is changed, then libvirtd will also change, which should usually be the case with libtool's wrapper scripts that cause libvirtd to get re-linked
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
+void virUpdateSelfLastChanged(const char *path) +{ + struct stat sb; + + if (stat(path, &sb) < 0) + return; + + if (sb.st_ctime > selfLastChanged) { + VIR_DEBUG("Setting self last changed to %lld for '%s'", + (long long)sb.st_ctime, path); + selfLastChanged = sb.st_ctime;
No sub-second resolution? I suppose it's okay, although gnulib gives you functions to access and compare full timestamp resolution. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Debian's package manager will preserve mtime timesatmp on binaries from the time they are built, rather than installed. So if a user downgrades their QEMU dpkg, the libvirt capabilities cache will not refresh. The fix is to use ctime instead of mtime since it cannot be faked. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index cae25e0..df5581f 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -259,7 +259,7 @@ struct _virQEMUCaps { bool usedQMP; char *binary; - time_t mtime; + time_t ctime; virBitmapPtr flags; @@ -2796,7 +2796,7 @@ virQEMUCapsPtr virQEMUCapsNewForBinary(const char *binary, binary); goto error; } - qemuCaps->mtime = sb.st_mtime; + qemuCaps->ctime = sb.st_ctime; /* Make sure the binary we are about to try exec'ing exists. * Technically we could catch the exec() failure, but that's @@ -2838,7 +2838,7 @@ bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps) if (stat(qemuCaps->binary, &sb) < 0) return false; - return sb.st_mtime == qemuCaps->mtime; + return sb.st_ctime == qemuCaps->ctime; } -- 1.8.5.3
On 03/10/2014 10:54 AM, Daniel P. Berrange wrote:
Debian's package manager will preserve mtime timesatmp on binaries from the time they are built, rather than installed. So if a user downgrades their QEMU dpkg, the libvirt capabilities cache will not refresh. The fix is to use ctime instead of mtime since it cannot be faked.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Mar 10, 2014 at 04:54:29PM +0000, Daniel P. Berrange wrote:
Debian's package manager will preserve mtime timesatmp on binaries
There's a 'timestamp' typo here if this hasn't been pushet yet. Christophe
Extracting capabilities from QEMU takes a notable amount of time when all QEMU binaries are installed. Each system emulator needs about 200-300ms multiplied by 26 binaries == ~5-8 seconds. This change causes the QEMU driver to save an XML file containing the content of the virQEMUCaps object instance in the cache dir eg /var/cache/libvirt/qemu/capabilities/$SHA256(binarypath).xml or $HOME/.cache/libvirt/qemu/cache/capabilities/$SHA256(binarypath).xml We attempt to load this and only if it fails, do we fallback to probing the QEMU binary. The ctime of the QEMU binary and libvirtd are stored in the cached file and its data discarded if either of them change. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 436 ++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_driver.c | 1 + 3 files changed, 431 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index df5581f..db95377 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -25,6 +25,7 @@ #include "qemu_capabilities.h" #include "viralloc.h" +#include "vircrypto.h" #include "virlog.h" #include "virerror.h" #include "virfile.h" @@ -253,6 +254,14 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "usb-kbd", /* 165 */ ); + +/* + * Update the XML parser/formatter when adding more + * information to this struct so that it gets cached + * correctly. It does not have to be ABI-stable. A + * newer libvirtd will simply discard the cache if + * it fails to parse the XML doc and save a new one. + */ struct _virQEMUCaps { virObject object; @@ -281,7 +290,7 @@ struct _virQEMUCapsCache { virMutex lock; virHashTablePtr binaries; char *libDir; - char *runDir; + char *cacheDir; uid_t runUid; gid_t runGid; }; @@ -2339,6 +2348,403 @@ int virQEMUCapsProbeQMP(virQEMUCapsPtr qemuCaps, } +/* + * Parsing a doc that looks like + * + * <qemuCaps> + * <usedQMP/> + * <flag name='foo'/> + * <flag name='bar'/> + * ... + * <cpu name="pentium3"/> + * ... + * <machine name="pc-1.0" alias="pc" maxCpus="4"/> + * ... + * </qemuCaps> + */ +static int +virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const char *filename, + time_t *qemuctime, time_t *selfctime) +{ + xmlDocPtr doc = NULL; + int ret = -1; + size_t i; + int n; + xmlNodePtr *nodes = NULL; + xmlXPathContextPtr ctxt = NULL; + char *str; + long long int l; + + if (!(doc = virXMLParseFile(filename))) + goto cleanup; + + if (!(ctxt = xmlXPathNewContext(doc))) { + virReportOOMError(); + goto cleanup; + } + + ctxt->node = xmlDocGetRootElement(doc); + + if (STRNEQ((const char *)ctxt->node->name, "qemuCaps")) { + virReportError(VIR_ERR_XML_ERROR, + _("unexpected root element <%s>, " + "expecting <qemuCaps>"), + ctxt->node->name); + goto cleanup; + } + + if (virXPathLongLong("string(./qemuctime)", ctxt, &l) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing qemuctime in QEMU capabilities XML")); + goto cleanup; + } + *qemuctime = (time_t)l; + + if (virXPathLongLong("string(./selfctime)", ctxt, &l) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing selfctime in QEMU capabilities XML")); + goto cleanup; + } + *selfctime = (time_t)l; + + qemuCaps->usedQMP = virXPathBoolean("count(.//usedQMP) > 0", + ctxt) > 0; + + if ((n = virXPathNodeSet(".//flag", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse qemu capabilities flags")); + goto cleanup; + } + VIR_DEBUG("Got flags %d", n); + if (n > 0) { + for (i = 0; i < n; i++) { + int flag; + if (!(str = virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing flag name in QEMU capabilities cache")); + goto cleanup; + } + flag = virQEMUCapsTypeFromString(str); + if (flag < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unknown qemu capabilities flag %s"), str); + VIR_FREE(str); + goto cleanup; + } + VIR_FREE(str); + virQEMUCapsSet(qemuCaps, flag); + } + } + VIR_FREE(nodes); + + if (virXPathUInt("string(.//version)", ctxt, &qemuCaps->version) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing version in QEMU capabilities cache")); + goto cleanup; + } + + if (virXPathUInt("string(.//kvmVersion)", ctxt, &qemuCaps->kvmVersion) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing version in QEMU capabilities cache")); + goto cleanup; + } + + if (!(str = virXPathString("string(.//arch)", ctxt))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing arch in QEMU capabilities cache")); + goto cleanup; + } + if (!(qemuCaps->arch = virArchFromString(str))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown arch %s in QEMU capabilities cache"), str); + goto cleanup; + } + + if ((n = virXPathNodeSet(".//cpu", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse qemu capabilities cpus")); + goto cleanup; + } + if (n > 0) { + qemuCaps->ncpuDefinitions = n; + if (VIR_ALLOC_N(qemuCaps->cpuDefinitions, + qemuCaps->ncpuDefinitions) < 0) + goto cleanup; + + for (i = 0; i < n; i++) { + if (!(str = virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing cpu name in QEMU capabilities cache")); + goto cleanup; + } + qemuCaps->cpuDefinitions[i] = str; + } + } + VIR_FREE(nodes); + + + if ((n = virXPathNodeSet(".//machine", ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse qemu capabilities machines")); + goto cleanup; + } + if (n > 0) { + qemuCaps->nmachineTypes = n; + if (VIR_ALLOC_N(qemuCaps->machineTypes, + qemuCaps->nmachineTypes) < 0 || + VIR_ALLOC_N(qemuCaps->machineAliases, + qemuCaps->nmachineTypes) < 0 || + VIR_ALLOC_N(qemuCaps->machineMaxCpus, + qemuCaps->nmachineTypes) < 0) + goto cleanup; + + for (i = 0; i < n; i++) { + if (!(str = virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing machine name in QEMU capabilities cache")); + goto cleanup; + } + qemuCaps->machineTypes[i] = str; + + qemuCaps->machineAliases[i] = virXMLPropString(nodes[i], "alias"); + + str = virXMLPropString(nodes[i], "maxCpus"); + if (str && + virStrToLong_ui(str, NULL, 10, &(qemuCaps->machineMaxCpus[i])) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed machine cpu count in QEMU capabilities cache")); + goto cleanup; + } + } + } + VIR_FREE(nodes); + + ret = 0; + cleanup: + VIR_FREE(nodes); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(doc); + return ret; +} + + +static int +virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *xml = NULL; + int ret = -1; + size_t i; + + virBufferAddLit(&buf, "<qemuCaps>\n"); + + virBufferAsprintf(&buf, " <qemuctime>%llu</qemuctime>\n", + (long long)qemuCaps->ctime); + virBufferAsprintf(&buf, " <selfctime>%llu</selfctime>\n", + (long long)virGetSelfLastChanged()); + + if (qemuCaps->usedQMP) + virBufferAddLit(&buf, " <usedQMP/>\n"); + + for (i = 0; i < QEMU_CAPS_LAST; i++) { + if (virQEMUCapsGet(qemuCaps, i)) { + virBufferAsprintf(&buf, " <flag name='%s'/>\n", + virQEMUCapsTypeToString(i)); + } + } + + virBufferAsprintf(&buf, " <version>%d</version>\n", + qemuCaps->version); + + virBufferAsprintf(&buf, " <kvmVersion>%d</kvmVersion>\n", + qemuCaps->kvmVersion); + + virBufferAsprintf(&buf, " <arch>%s</arch>\n", + virArchToString(qemuCaps->arch)); + + for (i = 0; i < qemuCaps->ncpuDefinitions; i++) { + virBufferEscapeString(&buf, " <cpu name='%s'/>\n", + qemuCaps->cpuDefinitions[i]); + } + + for (i = 0; i < qemuCaps->nmachineTypes; i++) { + virBufferEscapeString(&buf, " <machine name='%s'", + qemuCaps->machineTypes[i]); + if (qemuCaps->machineAliases[i]) + virBufferEscapeString(&buf, " alias='%s'", + qemuCaps->machineAliases[i]); + virBufferAsprintf(&buf, " maxCpus='%u'/>\n", + qemuCaps->machineMaxCpus[i]); + } + + virBufferAddLit(&buf, "</qemuCaps>\n"); + + if (virBufferError(&buf)) + goto cleanup; + + xml = virBufferContentAndReset(&buf); + + if (virFileWriteStr(filename, xml, 0600) < 0) { + virReportSystemError(errno, + _("Failed to save '%s' for '%s'"), + filename, qemuCaps->binary); + goto cleanup; + } + + VIR_DEBUG("Saved caps '%s' for '%s' with (%lld, %lld)", + filename, qemuCaps->binary, + (long long)qemuCaps->ctime, + (long long)virGetSelfLastChanged()); + + ret = 0; + cleanup: + VIR_FREE(xml); + return ret; +} + +static int +virQEMUCapsRememberCached(virQEMUCapsPtr qemuCaps, const char *cacheDir) +{ + char *capsdir = NULL; + char *capsfile = NULL; + int ret = -1; + char *binaryhash = NULL; + + if (virAsprintf(&capsdir, "%s/capabilities", cacheDir) < 0) + goto cleanup; + + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, + qemuCaps->binary, + &binaryhash) < 0) + goto cleanup; + + if (virAsprintf(&capsfile, "%s/%s.xml", capsdir, binaryhash) < 0) + goto cleanup; + + if (virFileMakePath(capsdir) < 0) { + virReportSystemError(errno, + _("Unable to create directory '%s'"), + capsdir); + goto cleanup; + } + + if (virQEMUCapsSaveCache(qemuCaps, capsfile) < 0) + goto cleanup; + + ret = 0; + cleanup: + VIR_FREE(binaryhash); + VIR_FREE(capsfile); + VIR_FREE(capsdir); + return ret; +} + + +static void +virQEMUCapsReset(virQEMUCapsPtr qemuCaps) +{ + size_t i; + + virBitmapClearAll(qemuCaps->flags); + qemuCaps->version = qemuCaps->kvmVersion = 0; + qemuCaps->arch = VIR_ARCH_NONE; + + for (i = 0; i < qemuCaps->ncpuDefinitions; i++) { + VIR_FREE(qemuCaps->cpuDefinitions[i]); + } + VIR_FREE(qemuCaps->cpuDefinitions); + qemuCaps->ncpuDefinitions = 0; + + for (i = 0; i < qemuCaps->nmachineTypes; i++) { + VIR_FREE(qemuCaps->machineTypes[i]); + VIR_FREE(qemuCaps->machineAliases[i]); + } + VIR_FREE(qemuCaps->machineTypes); + VIR_FREE(qemuCaps->machineAliases); + qemuCaps->nmachineTypes = 0; +} + + +static int +virQEMUCapsInitCached(virQEMUCapsPtr qemuCaps, const char *cacheDir) +{ + char *capsdir = NULL; + char *capsfile = NULL; + int ret = -1; + char *binaryhash = NULL; + struct stat sb; + time_t qemuctime; + time_t selfctime; + + if (virAsprintf(&capsdir, "%s/capabilities", cacheDir) < 0) + goto cleanup; + + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, + qemuCaps->binary, + &binaryhash) < 0) + goto cleanup; + + if (virAsprintf(&capsfile, "%s/%s.xml", capsdir, binaryhash) < 0) + goto cleanup; + + if (virFileMakePath(capsdir) < 0) { + virReportSystemError(errno, + _("Unable to create directory '%s'"), + capsdir); + goto cleanup; + } + + if (stat(capsfile, &sb) < 0) { + if (errno == ENOENT) { + VIR_DEBUG("No cached capabilities '%s' for '%s'", + capsfile, qemuCaps->binary); + ret = 0; + goto cleanup; + } + virReportSystemError(errno, + _("Unable to access cache '%s' for '%s'"), + capsfile, qemuCaps->binary); + goto cleanup; + } + + if (virQEMUCapsLoadCache(qemuCaps, capsfile, &qemuctime, &selfctime) < 0) { + virErrorPtr err = virGetLastError(); + VIR_WARN("Failed to load cached caps from '%s' for '%s': %s", + capsfile, qemuCaps->binary, err ? NULLSTR(err->message) : + _("unknown error")); + virResetLastError(); + ret = 0; + virQEMUCapsReset(qemuCaps); + goto cleanup; + } + + /* Discard if cache is older that QEMU binary */ + if (qemuctime != qemuCaps->ctime || + selfctime < virGetSelfLastChanged()) { + VIR_DEBUG("Outdated cached capabilities '%s' for '%s' " + "(%lld vs %lld, %lld vs %lld)", + capsfile, qemuCaps->binary, + (long long)qemuctime, (long long)qemuCaps->ctime, + (long long)selfctime, (long long)virGetSelfLastChanged()); + ignore_value(unlink(capsfile)); + virQEMUCapsReset(qemuCaps); + ret = 0; + goto cleanup; + } + + VIR_DEBUG("Loaded '%s' for '%s' ctime %lld usedQMP=%d", + capsfile, qemuCaps->binary, + (long long)qemuCaps->ctime, qemuCaps->usedQMP); + + ret = 1; + cleanup: + VIR_FREE(binaryhash); + VIR_FREE(capsfile); + VIR_FREE(capsdir); + return ret; +} + + #define QEMU_SYSTEM_PREFIX "qemu-system-" static int @@ -2779,6 +3185,7 @@ virQEMUCapsLogProbeFailure(const char *binary) virQEMUCapsPtr virQEMUCapsNewForBinary(const char *binary, const char *libDir, + const char *cacheDir, uid_t runUid, gid_t runGid) { @@ -2808,15 +3215,23 @@ virQEMUCapsPtr virQEMUCapsNewForBinary(const char *binary, goto error; } - if ((rv = virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid)) < 0) { - virQEMUCapsLogProbeFailure(binary); + if ((rv = virQEMUCapsInitCached(qemuCaps, cacheDir)) < 0) goto error; - } - if (!qemuCaps->usedQMP && - virQEMUCapsInitHelp(qemuCaps, runUid, runGid) < 0) { - virQEMUCapsLogProbeFailure(binary); - goto error; + if (rv == 0) { + if (virQEMUCapsInitQMP(qemuCaps, libDir, runUid, runGid) < 0) { + virQEMUCapsLogProbeFailure(binary); + goto error; + } + + if (!qemuCaps->usedQMP && + virQEMUCapsInitHelp(qemuCaps, runUid, runGid) < 0) { + virQEMUCapsLogProbeFailure(binary); + goto error; + } + + if (virQEMUCapsRememberCached(qemuCaps, cacheDir) < 0) + goto error; } return qemuCaps; @@ -2851,6 +3266,7 @@ virQEMUCapsHashDataFree(void *payload, const void *key ATTRIBUTE_UNUSED) virQEMUCapsCachePtr virQEMUCapsCacheNew(const char *libDir, + const char *cacheDir, uid_t runUid, gid_t runGid) { @@ -2870,6 +3286,8 @@ virQEMUCapsCacheNew(const char *libDir, goto error; if (VIR_STRDUP(cache->libDir, libDir) < 0) goto error; + if (VIR_STRDUP(cache->cacheDir, cacheDir) < 0) + goto error; cache->runUid = runUid; cache->runGid = runGid; @@ -2899,6 +3317,7 @@ virQEMUCapsCacheLookup(virQEMUCapsCachePtr cache, const char *binary) VIR_DEBUG("Creating capabilities for %s", binary); ret = virQEMUCapsNewForBinary(binary, cache->libDir, + cache->cacheDir, cache->runUid, cache->runGid); if (ret) { VIR_DEBUG("Caching capabilities %p for %s", @@ -2938,6 +3357,7 @@ virQEMUCapsCacheFree(virQEMUCapsCachePtr cache) return; VIR_FREE(cache->libDir); + VIR_FREE(cache->cacheDir); virHashFree(cache->binaries); virMutexDestroy(&cache->lock); VIR_FREE(cache); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index b5445e7..a9082d5 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -218,6 +218,7 @@ virQEMUCapsPtr virQEMUCapsNew(void); virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps); virQEMUCapsPtr virQEMUCapsNewForBinary(const char *binary, const char *libDir, + const char *cacheDir, uid_t runUid, gid_t runGid); @@ -262,6 +263,7 @@ bool virQEMUCapsIsValid(virQEMUCapsPtr qemuCaps); virQEMUCapsCachePtr virQEMUCapsCacheNew(const char *libDir, + const char *cacheDir, uid_t uid, gid_t gid); virQEMUCapsPtr virQEMUCapsCacheLookup(virQEMUCapsCachePtr cache, const char *binary); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8a54b8a..7fea07c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -744,6 +744,7 @@ qemuStateInitialize(bool privileged, } qemu_driver->qemuCapsCache = virQEMUCapsCacheNew(cfg->libDir, + cfg->cacheDir, run_uid, run_gid); if (!qemu_driver->qemuCapsCache) -- 1.8.5.3
On 03/10/2014 10:54 AM, Daniel P. Berrange wrote:
Extracting capabilities from QEMU takes a notable amount of time when all QEMU binaries are installed. Each system emulator needs about 200-300ms multiplied by 26 binaries == ~5-8 seconds.
This change causes the QEMU driver to save an XML file containing the content of the virQEMUCaps object instance in the cache dir eg /var/cache/libvirt/qemu/capabilities/$SHA256(binarypath).xml or $HOME/.cache/libvirt/qemu/cache/capabilities/$SHA256(binarypath).xml
Good, http://www.tldp.org/LDP/Linux-Filesystem-Hierarchy/html/var.html states that /var/cache normally persists across reboots.
We attempt to load this and only if it fails, do we fallback to probing the QEMU binary. The ctime of the QEMU binary and libvirtd are stored in the cached file and its data discarded if either of them change.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_capabilities.c | 436 ++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_capabilities.h | 2 + src/qemu/qemu_driver.c | 1 + 3 files changed, 431 insertions(+), 8 deletions(-)
+ +/* + * Update the XML parser/formatter when adding more + * information to this struct so that it gets cached + * correctly. It does not have to be ABI-stable. A + * newer libvirtd will simply discard the cache if + * it fails to parse the XML doc and save a new one.
A newer libvirtd will discard the cache anyways, because the timestamp differs from the cached ctime.
+/* + * Parsing a doc that looks like + * + * <qemuCaps> + * <usedQMP/> + * <flag name='foo'/> + * <flag name='bar'/> + * ... + * <cpu name="pentium3"/> + * ... + * <machine name="pc-1.0" alias="pc" maxCpus="4"/> + * ... + * </qemuCaps>
Ought to mention the cached ctime elements.
+ */ +static int +virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const char *filename, + time_t *qemuctime, time_t *selfctime) +{
+ if (virXPathLongLong("string(./qemuctime)", ctxt, &l) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing qemuctime in QEMU capabilities XML"));
+ qemuCaps->usedQMP = virXPathBoolean("count(.//usedQMP) > 0",
Why ./qemuctime but .//usedQMP? What difference does // make in XPath?
+ +static int +virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *xml = NULL; + int ret = -1; + size_t i; + + virBufferAddLit(&buf, "<qemuCaps>\n"); + + virBufferAsprintf(&buf, " <qemuctime>%llu</qemuctime>\n",
Conflicts with Laine's work to require virBufferAddIndent() rather than hard-coding indentation. Will be interesting to see who gets in first :)
+static void +virQEMUCapsReset(virQEMUCapsPtr qemuCaps) +{ + size_t i; + + virBitmapClearAll(qemuCaps->flags); + qemuCaps->version = qemuCaps->kvmVersion = 0; + qemuCaps->arch = VIR_ARCH_NONE;
Shouldn't you also reset qemuCaps->usedQMP and qemuCaps->ctime, to get the struct back to a known-default state? Or is this only ever going to be used just before freeing the struct, at which point all the reset-to-0 code is wasted CPU cycles? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Mar 10, 2014 at 12:50:20PM -0600, Eric Blake wrote:
On 03/10/2014 10:54 AM, Daniel P. Berrange wrote:
Extracting capabilities from QEMU takes a notable amount of time when all QEMU binaries are installed. Each system emulator needs about 200-300ms multiplied by 26 binaries == ~5-8 seconds.
This change causes the QEMU driver to save an XML file containing the content of the virQEMUCaps object instance in the cache dir eg /var/cache/libvirt/qemu/capabilities/$SHA256(binarypath).xml or $HOME/.cache/libvirt/qemu/cache/capabilities/$SHA256(binarypath).xml
+ */ +static int +virQEMUCapsLoadCache(virQEMUCapsPtr qemuCaps, const char *filename, + time_t *qemuctime, time_t *selfctime) +{
+ if (virXPathLongLong("string(./qemuctime)", ctxt, &l) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing qemuctime in QEMU capabilities XML"));
+ qemuCaps->usedQMP = virXPathBoolean("count(.//usedQMP) > 0",
Why ./qemuctime but .//usedQMP? What difference does // make in XPath?
That's a bug, but thanks to xpath it just happens to still work. ./foo means match '<foo>' as an immediate child of current node. .//foo means match '<foo>' as an arbitrarily deep child of the current node.
+static int +virQEMUCapsSaveCache(virQEMUCapsPtr qemuCaps, const char *filename) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + const char *xml = NULL; + int ret = -1; + size_t i; + + virBufferAddLit(&buf, "<qemuCaps>\n"); + + virBufferAsprintf(&buf, " <qemuctime>%llu</qemuctime>\n",
Conflicts with Laine's work to require virBufferAddIndent() rather than hard-coding indentation. Will be interesting to see who gets in first :)
Well it doesn't conflict so much as mean Laine has more work todo :-P
+static void +virQEMUCapsReset(virQEMUCapsPtr qemuCaps) +{ + size_t i; + + virBitmapClearAll(qemuCaps->flags); + qemuCaps->version = qemuCaps->kvmVersion = 0; + qemuCaps->arch = VIR_ARCH_NONE;
Shouldn't you also reset qemuCaps->usedQMP and qemuCaps->ctime, to get the struct back to a known-default state? Or is this only ever going to be used just before freeing the struct, at which point all the reset-to-0 code is wasted CPU cycles?
It should reset usedQMP, but not ctime - the ctime value always matches the current QEMU binary and we don't overwrite that when loading the XML. Regards, 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 (3)
-
Christophe Fergeau -
Daniel P. Berrange -
Eric Blake