On Tue, Feb 20, 2024 at 11:39 AM Manos Pitsidianakis <
manos.pitsidiana...@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 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 <aest...@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 <aest...@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
> >
> >
>
>
