Re: [PATCH 4/7] qemu: conf: Cache domCaps in qemuCaps

[Daniel Henrique Barboza <danielhb413@xxxxxxxxx>]

On 4/4/19 8:37 PM, Cole Robinson wrote:
> qemuCaps is tied to a binary on disk. domCaps is tied to a combo
> of binary+machine+arch+virttype values. For the qemu driver this almost
> entirely translates to a permutation of qemuCaps though
>
> Upcoming patches want to use the domCaps data store at XML validate
> time, but we need to cache the data so we aren't repeatedly
> regenerating it.
>
> Add a domCapsCache hash table to qemuCaps. This ensures that the domCaps
> cache is blown away whenever qemuCaps needs to be regenerated. Adjust
> virQEMUDriverGetDomainCapabilities to search the cache and add to it
> if we don't find a hit.
>
> Signed-off-by: Cole Robinson <crobinso@xxxxxxxxxx>

Nit: there is a 80+ char line at the start of virQEMUDriverSearchDomcaps,
but I'm not sure if this is more like a "nice to have" rule or if it's
enforced like in QEMU.


Reviewed-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>


> ---
>   src/qemu/qemu_capabilities.c | 11 +++++++
>   src/qemu/qemu_capabilities.h |  1 +
>   src/qemu/qemu_conf.c         | 59 +++++++++++++++++++++++++++++++-----
>   3 files changed, 64 insertions(+), 7 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 46ba5e30b5..a3c1348157 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -582,6 +582,7 @@ struct _virQEMUCaps {
>   
>       virArch arch;
>   
> +    virHashTablePtr domCapsCache;
>       virDomainCapsCPUModelsPtr kvmCPUModels;
>       virDomainCapsCPUModelsPtr tcgCPUModels;
>   
> @@ -1476,6 +1477,9 @@ virQEMUCapsNew(void)
>       if (!(qemuCaps->flags = virBitmapNew(QEMU_CAPS_LAST)))
>           goto error;
>   
> +    if (!(qemuCaps->domCapsCache = virHashCreate(5, virObjectFreeHashData)))
> +        goto error;
> +
>       return qemuCaps;
>   
>    error:
> @@ -1628,6 +1632,7 @@ void virQEMUCapsDispose(void *obj)
>       }
>       VIR_FREE(qemuCaps->machineTypes);
>   
> +    virHashFree(qemuCaps->domCapsCache);
>       virObjectUnref(qemuCaps->kvmCPUModels);
>       virObjectUnref(qemuCaps->tcgCPUModels);
>   
> @@ -1790,6 +1795,12 @@ const char *virQEMUCapsGetPackage(virQEMUCapsPtr qemuCaps)
>   }
>   
>   
> +virHashTablePtr virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps)
> +{
> +    return qemuCaps->domCapsCache;
> +}
> +
> +
>   int
>   virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
>                                virDomainVirtType type,
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index c6f6980684..2a37f0c2ff 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -538,6 +538,7 @@ const char *virQEMUCapsGetBinary(virQEMUCapsPtr qemuCaps);
>   virArch virQEMUCapsGetArch(virQEMUCapsPtr qemuCaps);
>   unsigned int virQEMUCapsGetVersion(virQEMUCapsPtr qemuCaps);
>   const char *virQEMUCapsGetPackage(virQEMUCapsPtr qemuCaps);
> +virHashTablePtr virQEMUCapsGetDomainCapsCache(virQEMUCapsPtr qemuCaps);
>   unsigned int virQEMUCapsGetKVMVersion(virQEMUCapsPtr qemuCaps);
>   int virQEMUCapsAddCPUDefinitions(virQEMUCapsPtr qemuCaps,
>                                    virDomainVirtType type,
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 635fdcc5a4..6a7c183075 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -1358,10 +1358,39 @@ virCapsPtr virQEMUDriverGetCapabilities(virQEMUDriverPtr driver,
>   }
>   
>   
> +struct virQEMUDriverSearchDomcapsData {
> +    const char *path;
> +    const char *machine;
> +    virArch arch;
> +    virDomainVirtType virttype;
> +};
> +
> +
> +static int
> +virQEMUDriverSearchDomcaps(const void *payload,
> +                           const void *name ATTRIBUTE_UNUSED,
> +                           const void *opaque)
> +{
> +    virDomainCapsPtr domCaps = (virDomainCapsPtr) payload;
> +    struct virQEMUDriverSearchDomcapsData *data = (struct virQEMUDriverSearchDomcapsData *) opaque;
> +
> +    if (STREQ_NULLABLE(data->path, domCaps->path) &&
> +        STREQ_NULLABLE(data->machine, domCaps->machine) &&
> +        data->arch == domCaps->arch &&
> +        data->virttype == domCaps->virttype)
> +        return 1;
> +
> +    return 0;
> +}
> +
>   /**
>    * virQEMUDriverGetDomainCapabilities:
>    *
> - * Build a virDomainCapsPtr instance for the passed data.
> + * Get a reference to the virDomainCapsPtr instance from the virQEMUCapsPtr
> + * domCapsCache. If there's no domcaps in the cache, create a new instance,
> + * add it to the cache, and return a reference.
> + *
> + * The caller must release the reference with virObjetUnref
>    *
>    * Returns: a reference to a virDomainCapsPtr instance or NULL
>    */
> @@ -1375,18 +1404,34 @@ virQEMUDriverGetDomainCapabilities(virQEMUDriverPtr driver,
>       virDomainCapsPtr ret = NULL, domCaps = NULL;
>       virCapsPtr caps = NULL;
>       virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> +    virHashTablePtr domCapsCache = virQEMUCapsGetDomainCapsCache(qemuCaps);
> +    struct virQEMUDriverSearchDomcapsData data = {
> +        .path = virQEMUCapsGetBinary(qemuCaps),
> +        .machine = machine,
> +        .arch = arch,
> +        .virttype = virttype,
> +    };
>   
>       if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>           goto cleanup;
>   
> -    if (!(domCaps = virDomainCapsNew(virQEMUCapsGetBinary(qemuCaps), machine,
> -                                     arch, virttype)))
> -        goto cleanup;
> +    domCaps = virHashSearch(domCapsCache,
> +                            virQEMUDriverSearchDomcaps, &data, NULL);
> +    if (!domCaps) {
> +        /* hash miss, build new domcaps */
> +        if (!(domCaps = virDomainCapsNew(data.path, data.machine,
> +                                         data.arch, data.virttype)))
> +            goto cleanup;
>   
> -    if (virQEMUCapsFillDomainCaps(caps, domCaps, qemuCaps,
> -                                  cfg->firmwares, cfg->nfirmwares) < 0)
> -        goto cleanup;
> +        if (virQEMUCapsFillDomainCaps(caps, domCaps, qemuCaps,
> +                                      cfg->firmwares, cfg->nfirmwares) < 0)
> +            goto cleanup;
> +
> +        if (virHashAddEntry(domCapsCache, machine, domCaps) < 0)
> +            goto cleanup;
> +    }
>   
> +    virObjectRef(domCaps);
>       VIR_STEAL_PTR(ret, domCaps);
>    cleanup:
>       virObjectUnref(domCaps);

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