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

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

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. Is this code
ever called without BQL, for example do the backend read/write functions 
from vhost-user.c run without the BQL?

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