1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemu: Introduce caching whether /dev/kvm is accessible

Marc Hartmayer posted 1 patch 5 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20181029165358.5591-1-mhartmay@linux.ibm.com
There is a newer version of this series
src/qemu/qemu_capabilities.c | 56 ++++++++++++++++++++++++++++++++++--
1 file changed, 54 insertions(+), 2 deletions(-)
[libvirt] [PATCH] qemu: Introduce caching whether /dev/kvm is accessible
Posted by Marc Hartmayer 5 years, 5 months ago 
Introduce caching whether /dev/kvm is usable as the QEMU user:QEMU
group. This reduces the overhead of the QEMU capabilities cache
lookup. Before this patch there were many fork() calls used for
checking whether /dev/kvm is accessible. Now we store the result
whether /dev/kvm is accessible or not and we only need to re-run the
virFileAccessibleAs check if the ctime of /dev/kvm has changed.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 src/qemu/qemu_capabilities.c | 56 ++++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e228f52ec0bb..ea95915f0f71 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3238,6 +3238,10 @@ struct _virQEMUCapsCachePriv {
     virArch hostArch;
     unsigned int microcodeVersion;
     char *kernelVersion;
+
+    /* cache whether /dev/kvm is usable as runUid:runGuid */
+    virTristateBool kvmUsable;
+    time_t kvmCtime;
 };
 typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
 typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
@@ -3824,6 +3828,54 @@ virQEMUCapsSaveFile(void *data,
 }
 
 
+/* Determine whether '/dev/kvm' is usable as QEMU user:QEMU group. */
+static bool
+virQEMUCapsKVMUsable(virQEMUCapsCachePrivPtr priv)
+{
+    struct stat sb;
+    static const char *kvm_device = "/dev/kvm";
+    virTristateBool value;
+    virTristateBool cached_value = priv->kvmUsable;
+    time_t kvm_ctime;
+    time_t cached_kvm_ctime = priv->kvmCtime;
+
+    if (stat(kvm_device, &sb) < 0) {
+        virReportSystemError(errno,
+                             _("Failed to stat %s"), kvm_device);
+        return false;
+    }
+    kvm_ctime = sb.st_ctime;
+
+    if (cached_value != VIR_TRISTATE_BOOL_ABSENT) {
+        if (kvm_ctime != cached_kvm_ctime) {
+            VIR_DEBUG("%s has changed (%lld vs %lld)", kvm_device,
+                      (long long)kvm_ctime, (long long)cached_kvm_ctime);
+            goto update;
+        }
+
+        return cached_value == VIR_TRISTATE_BOOL_YES;
+    }
+
+ update:
+    if (virFileAccessibleAs(kvm_device, R_OK | W_OK,
+                            priv->runUid, priv->runGid) == 0) {
+        value = VIR_TRISTATE_BOOL_YES;
+    } else {
+        value = VIR_TRISTATE_BOOL_NO;
+    }
+
+    /* There is a race window between 'stat' and
+     * 'virFileAccessibleAs'. However, since we're only interested in
+     * detecting changes *after* the virFileAccessibleAs check, we can
+     * neglect this here.
+     */
+    priv->kvmCtime = kvm_ctime;
+    priv->kvmUsable = value;
+
+    return value == VIR_TRISTATE_BOOL_YES;
+}
+
+
 static bool
 virQEMUCapsIsValid(void *data,
                    void *privData)
@@ -3872,8 +3924,7 @@ virQEMUCapsIsValid(void *data,
         return true;
     }
 
-    kvmUsable = virFileAccessibleAs("/dev/kvm", R_OK | W_OK,
-                                    priv->runUid, priv->runGid) == 0;
+    kvmUsable = virQEMUCapsKVMUsable(priv);
 
     if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) &&
         kvmUsable) {
@@ -4684,6 +4735,7 @@ virQEMUCapsCacheNew(const char *libDir,
     priv->runUid = runUid;
     priv->runGid = runGid;
     priv->microcodeVersion = microcodeVersion;
+    priv->kvmUsable = VIR_TRISTATE_BOOL_ABSENT;
 
     if (uname(&uts) == 0 &&
         virAsprintf(&priv->kernelVersion, "%s %s", uts.release, uts.version) < 0)
-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Introduce caching whether /dev/kvm is accessible
Posted by Daniel P. Berrangé 5 years, 5 months ago 
On Mon, Oct 29, 2018 at 05:53:58PM +0100, Marc Hartmayer wrote:
> Introduce caching whether /dev/kvm is usable as the QEMU user:QEMU
> group. This reduces the overhead of the QEMU capabilities cache
> lookup. Before this patch there were many fork() calls used for
> checking whether /dev/kvm is accessible. Now we store the result
> whether /dev/kvm is accessible or not and we only need to re-run the
> virFileAccessibleAs check if the ctime of /dev/kvm has changed.
> 
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  src/qemu/qemu_capabilities.c | 56 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 54 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index e228f52ec0bb..ea95915f0f71 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3238,6 +3238,10 @@ struct _virQEMUCapsCachePriv {
>      virArch hostArch;
>      unsigned int microcodeVersion;
>      char *kernelVersion;
> +
> +    /* cache whether /dev/kvm is usable as runUid:runGuid */
> +    virTristateBool kvmUsable;
> +    time_t kvmCtime;
>  };
>  typedef struct _virQEMUCapsCachePriv virQEMUCapsCachePriv;
>  typedef virQEMUCapsCachePriv *virQEMUCapsCachePrivPtr;
> @@ -3824,6 +3828,54 @@ virQEMUCapsSaveFile(void *data,
>  }
>  
>  
> +/* Determine whether '/dev/kvm' is usable as QEMU user:QEMU group. */
> +static bool
> +virQEMUCapsKVMUsable(virQEMUCapsCachePrivPtr priv)
> +{
> +    struct stat sb;
> +    static const char *kvm_device = "/dev/kvm";
> +    virTristateBool value;
> +    virTristateBool cached_value = priv->kvmUsable;
> +    time_t kvm_ctime;
> +    time_t cached_kvm_ctime = priv->kvmCtime;
> +
> +    if (stat(kvm_device, &sb) < 0) {
> +        virReportSystemError(errno,
> +                             _("Failed to stat %s"), kvm_device);
> +        return false;
> +    }
> +    kvm_ctime = sb.st_ctime;
> +
> +    if (cached_value != VIR_TRISTATE_BOOL_ABSENT) {
> +        if (kvm_ctime != cached_kvm_ctime) {
> +            VIR_DEBUG("%s has changed (%lld vs %lld)", kvm_device,
> +                      (long long)kvm_ctime, (long long)cached_kvm_ctime);
> +            goto update;
> +        }
> +
> +        return cached_value == VIR_TRISTATE_BOOL_YES;
> +    }
> +
> + update:

I don't like this use of goto's. It could be simplified thus to give
a linear flow:

     if (kvm_ctime != cached_kvm_ctime) {
        VIR_DEBUG("%s has changed (%lld vs %lld)", kvm_device,
                  (long long)kvm_ctime, (long long)cached_kvm_ctime);
        cached_value = VIR_TRISTATE_BOOL_ABSENT;
     }

     if (cached_value != VIR_TRISTATE_BOOL_ABSENT) {
        return cached_value == VIR_TRISTATE_BOOL_YES;
     }

> +    if (virFileAccessibleAs(kvm_device, R_OK | W_OK,
> +                            priv->runUid, priv->runGid) == 0) {
> +        value = VIR_TRISTATE_BOOL_YES;
> +    } else {
> +        value = VIR_TRISTATE_BOOL_NO;
> +    }
> +
> +    /* There is a race window between 'stat' and
> +     * 'virFileAccessibleAs'. However, since we're only interested in
> +     * detecting changes *after* the virFileAccessibleAs check, we can
> +     * neglect this here.
> +     */
> +    priv->kvmCtime = kvm_ctime;
> +    priv->kvmUsable = value;
> +
> +    return value == VIR_TRISTATE_BOOL_YES;
> +}

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.