diff mbox series

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

Message ID 980d16e0dd03fdebba118fc2ab8a3a0b5dc08b76.1554419814.git.crobinso@redhat.com
State New
Headers show
Series qemu: use domCaps for validation | expand

Commit Message

Cole Robinson April 4, 2019, 11:37 p.m. UTC 
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@redhat.com>

---
 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(-)

-- 
2.21.0

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

Comments

Daniel Henrique Barboza April 26, 2019, 8:58 p.m. UTC | #1 
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@redhat.com>


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@gmail.com>



> ---

>   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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Michal Prívozník April 29, 2019, 12:35 p.m. UTC | #2 
On 4/5/19 1:37 AM, 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@redhat.com>

> ---

>   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(-)

> 


At first I was affraid, I was petrified, that we will need to invalidate 
this cache whenever corresponding qemuCaps is invalidated. But that 
should be done automatically, because virQEMUCapsCacheLookup() unrefs 
qemuCaps when if finds outdated capabilities.

Might be worth mentioning in the commit message. Or not. Your call.

Michal

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

Patch

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);