Re: [PATCH v4 3/5] hw/virtio: change dmabuf mutex to QemuMutex

["Alex Bennée" <alex.bennee@linaro.org>]

Albert Esteve <aesteve@redhat.com> writes:

> On Tue, Feb 20, 2024 at 11:39 AM Manos Pitsidianakis <manos.pitsidianakis@linaro.org> wrote:
>
>  Hello Albert,
>
>  This is a point of confusion for me; Volker recently pointed out in a 
>  patch for virtio-snd that all its code runs under the BQL. 
>
> Hello Manos,
>
> I updated it to QemuMutex after a suggestion from Alex Benee, but I was not
> really aware it existed before his comment. So for your question I had to check what
> exactly BQL stands for in this context (big QEMU lock). Therefore, as you can see,
> I am probably not the right person to answer it. 
>  
>  Is this code
>  ever called without BQL, for example do the backend read/write functions 
>  from vhost-user.c run without the BQL?
>
> To my understanding, they should as every access to the shared table may incur
> in a race. But I'd need to read the code better to verify if that is
> indeed the case.

The BQL will be held for any MMIO access but that is not all the
accesses that happen with VirtIO. Access to the shared buffers is
controlled by protocol and certain accesses need to be atomic.

The question is are all these functions triggered by MMIO operations or
by other events?

>
> The only thing I can say is that, if this change is confusing or may lead to issues
> related to the scope of the lock, it may be better to dismiss the change and
> split it to its own specific patch, so I have the chance to verify the chance in
> a better way without delaying the other commits here.
>  
>  
>  On Mon, 19 Feb 2024 16:34, Albert Esteve <aesteve@redhat.com> wrote:
>  >Change GMutex by QemuMutex to be able to use
>  >lock contexts with `WITH_QEMU_LOCK_GUARD`.
>  >
>  >As the lock needs to be initialised and there
>  >is no central point for initialisation, add
>  >an init public function and call it from
>  >virtio.c, each time a new backend structure
>  >is initialised.
>  >
>  >Signed-off-by: Albert Esteve <aesteve@redhat.com>
>  >---
>  > hw/display/virtio-dmabuf.c        | 55 +++++++++++++++++--------------
>  > hw/virtio/virtio.c                |  3 ++
>  > include/hw/virtio/virtio-dmabuf.h |  5 +++
>  > tests/unit/test-virtio-dmabuf.c   |  5 +++
>  > 4 files changed, 43 insertions(+), 25 deletions(-)
>  >
>  >diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
>  >index 497cb6fa7c..961094a561 100644
>  >--- a/hw/display/virtio-dmabuf.c
>  >+++ b/hw/display/virtio-dmabuf.c
>  >@@ -11,11 +11,12 @@
>  >  */
>  > 
>  > #include "qemu/osdep.h"
>  >+#include "include/qemu/lockable.h"
>  > 
>  > #include "hw/virtio/virtio-dmabuf.h"
>  > 
>  > 
>  >-static GMutex lock;
>  >+static QemuMutex lock;
>  > static GHashTable *resource_uuids;
>  > 
>  > /*
>  >@@ -27,23 +28,27 @@ static int uuid_equal_func(const void *lhv, const void *rhv)
>  >     return qemu_uuid_is_equal(lhv, rhv);
>  > }
>  > 
>  >+void virtio_dmabuf_init(void) {
>  >+    qemu_mutex_init(&lock);
>  >+}
>  >+
>  > static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject *value)
>  > {
>  >     bool result = true;
>  > 
>  >-    g_mutex_lock(&lock);
>  >-    if (resource_uuids == NULL) {
>  >-        resource_uuids = g_hash_table_new_full(qemu_uuid_hash,
>  >-                                               uuid_equal_func,
>  >-                                               NULL,
>  >-                                               g_free);
>  >-    }
>  >-    if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
>  >-        g_hash_table_insert(resource_uuids, uuid, value);
>  >-    } else {
>  >-        result = false;
>  >+    WITH_QEMU_LOCK_GUARD(&lock) {
>  >+        if (resource_uuids == NULL) {
>  >+            resource_uuids = g_hash_table_new_full(qemu_uuid_hash,
>  >+                                                uuid_equal_func,
>  >+                                                NULL,
>  >+                                                g_free);
>  >+        }
>  >+        if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
>  >+            g_hash_table_insert(resource_uuids, uuid, value);
>  >+        } else {
>  >+            result = false;
>  >+        }
>  >     }
>  >-    g_mutex_unlock(&lock);
>  > 
>  >     return result;
>  > }
>  >@@ -87,9 +92,9 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
>  > bool virtio_remove_resource(const QemuUUID *uuid)
>  > {
>  >     bool result;
>  >-    g_mutex_lock(&lock);
>  >-    result = g_hash_table_remove(resource_uuids, uuid);
>  >-    g_mutex_unlock(&lock);
>  >+    WITH_QEMU_LOCK_GUARD(&lock) {
>  >+        result = g_hash_table_remove(resource_uuids, uuid);
>  >+    }
>  > 
>  >     return result;
>  > }
>  >@@ -98,11 +103,11 @@ static VirtioSharedObject *get_shared_object(const QemuUUID *uuid)
>  > {
>  >     gpointer lookup_res = NULL;
>  > 
>  >-    g_mutex_lock(&lock);
>  >-    if (resource_uuids != NULL) {
>  >-        lookup_res = g_hash_table_lookup(resource_uuids, uuid);
>  >+    WITH_QEMU_LOCK_GUARD(&lock) {
>  >+        if (resource_uuids != NULL) {
>  >+            lookup_res = g_hash_table_lookup(resource_uuids, uuid);
>  >+        }
>  >     }
>  >-    g_mutex_unlock(&lock);
>  > 
>  >     return (VirtioSharedObject *) lookup_res;
>  > }
>  >@@ -138,9 +143,9 @@ SharedObjectType virtio_object_type(const QemuUUID *uuid)
>  > 
>  > void virtio_free_resources(void)
>  > {
>  >-    g_mutex_lock(&lock);
>  >-    g_hash_table_destroy(resource_uuids);
>  >-    /* Reference count shall be 0 after the implicit unref on destroy */
>  >-    resource_uuids = NULL;
>  >-    g_mutex_unlock(&lock);
>  >+    WITH_QEMU_LOCK_GUARD(&lock) {
>  >+        g_hash_table_destroy(resource_uuids);
>  >+        /* Reference count shall be 0 after the implicit unref on destroy */
>  >+        resource_uuids = NULL;
>  >+    }
>  > }
>  >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>  >index d229755eae..88189e7178 100644
>  >--- a/hw/virtio/virtio.c
>  >+++ b/hw/virtio/virtio.c
>  >@@ -29,6 +29,7 @@
>  > #include "hw/virtio/virtio-bus.h"
>  > #include "hw/qdev-properties.h"
>  > #include "hw/virtio/virtio-access.h"
>  >+#include "hw/virtio/virtio-dmabuf.h"
>  > #include "sysemu/dma.h"
>  > #include "sysemu/runstate.h"
>  > #include "virtio-qmp.h"
>  >@@ -3221,6 +3222,8 @@ void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size)
>  >     int i;
>  >     int nvectors = k->query_nvectors ? k->query_nvectors(qbus->parent) : 0;
>  > 
>  >+    // Ensure virtio dmabuf table is initialised.
>  >+    virtio_dmabuf_init();
>  >     if (nvectors) {
>  >         vdev->vector_queues =
>  >             g_malloc0(sizeof(*vdev->vector_queues) * nvectors);
>  >diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
>  >index 891a43162d..627d84dce9 100644
>  >--- a/include/hw/virtio/virtio-dmabuf.h
>  >+++ b/include/hw/virtio/virtio-dmabuf.h
>  >@@ -50,6 +50,11 @@ typedef struct VirtioSharedObject {
>  >     } value;
>  > } VirtioSharedObject;
>  > 
>  >+/**
>  >+ * virtio_dmabuf_init() - Initialise virtio dmabuf internal structures.
>  >+ */
>  >+void virtio_dmabuf_init(void);
>  >+
>  > /**
>  >  * virtio_add_dmabuf() - Add a new dma-buf resource to the lookup table
>  >  * @uuid: new resource's UUID
>  >diff --git a/tests/unit/test-virtio-dmabuf.c b/tests/unit/test-virtio-dmabuf.c
>  >index a45ec52f42..20213455ee 100644
>  >--- a/tests/unit/test-virtio-dmabuf.c
>  >+++ b/tests/unit/test-virtio-dmabuf.c
>  >@@ -27,6 +27,7 @@ static void test_add_remove_resources(void)
>  >     QemuUUID uuid;
>  >     int i, dmabuf_fd;
>  > 
>  >+    virtio_dmabuf_init();
>  >     for (i = 0; i < 100; ++i) {
>  >         qemu_uuid_generate(&uuid);
>  >         dmabuf_fd = g_random_int_range(3, 500);
>  >@@ -46,6 +47,7 @@ static void test_add_remove_dev(void)
>  >     struct vhost_dev *dev = g_new0(struct vhost_dev, 1);
>  >     int i;
>  > 
>  >+    virtio_dmabuf_init();
>  >     for (i = 0; i < 100; ++i) {
>  >         qemu_uuid_generate(&uuid);
>  >         virtio_add_vhost_device(&uuid, dev);
>  >@@ -64,6 +66,7 @@ static void test_remove_invalid_resource(void)
>  >     QemuUUID uuid;
>  >     int i;
>  > 
>  >+    virtio_dmabuf_init();
>  >     for (i = 0; i < 20; ++i) {
>  >         qemu_uuid_generate(&uuid);
>  >         g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
>  >@@ -78,6 +81,7 @@ static void test_add_invalid_resource(void)
>  >     struct vhost_dev *dev = NULL;
>  >     int i, dmabuf_fd = -2, alt_dmabuf = 2;
>  > 
>  >+    virtio_dmabuf_init();
>  >     for (i = 0; i < 20; ++i) {
>  >         qemu_uuid_generate(&uuid);
>  >         /* Add a new resource with invalid (negative) resource fd */
>  >@@ -108,6 +112,7 @@ static void test_free_resources(void)
>  >     QemuUUID uuids[20];
>  >     int i, dmabuf_fd;
>  > 
>  >+    virtio_dmabuf_init();
>  >     for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
>  >         qemu_uuid_generate(&uuids[i]);
>  >         dmabuf_fd = g_random_int_range(3, 500);
>  >-- 
>  >2.43.1
>  >
>  >

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro