[v1] Virtio dmabuf improvements

[PATCH 2/3] hw/virtio: cleanup shared resources

Posted by Albert Esteve 1 year ago

Ensure that we cleanup all virtio shared
resources when the vhost devices is cleaned
up (after a hot unplug, or a crash).

To track all owned uuids of a device, add
a GSList to the vhost_dev struct. This way
we can avoid traversing the full table
for every cleanup, whether they actually
own any shared resource or not.

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 hw/virtio/vhost-user.c    | 2 ++
 hw/virtio/vhost.c         | 4 ++++
 include/hw/virtio/vhost.h | 6 ++++++
 3 files changed, 12 insertions(+)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 5fdff0241f..04848d1fa0 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1598,6 +1598,7 @@ vhost_user_backend_handle_shared_object_add(struct vhost_dev *dev,
     QemuUUID uuid;
 
     memcpy(uuid.data, object->uuid, sizeof(object->uuid));
+    dev->shared_uuids = g_slist_append(dev->shared_uuids, &uuid);
     return virtio_add_vhost_device(&uuid, dev);
 }
 
@@ -1623,6 +1624,7 @@ vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
     }
 
     memcpy(uuid.data, object->uuid, sizeof(object->uuid));
+    dev->shared_uuids = g_slist_remove_all(dev->shared_uuids, &uuid);
     return virtio_remove_resource(&uuid);
 }
 
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 9c9ae7109e..3aff94664b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -16,6 +16,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/virtio/vhost.h"
+#include "hw/virtio/virtio-dmabuf.h"
 #include "qemu/atomic.h"
 #include "qemu/range.h"
 #include "qemu/error-report.h"
@@ -1599,6 +1600,9 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     migrate_del_blocker(&hdev->migration_blocker);
     g_free(hdev->mem);
     g_free(hdev->mem_sections);
+    /* free virtio shared objects */
+    g_slist_foreach(hdev->shared_uuids, (GFunc)virtio_remove_resource, NULL);
+    g_slist_free_full(g_steal_pointer(&hdev->shared_uuids), g_free);
     if (hdev->vhost_ops) {
         hdev->vhost_ops->vhost_backend_cleanup(hdev);
     }
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 5e8183f64a..376bc8446d 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -118,6 +118,12 @@ struct vhost_dev {
      */
     uint64_t protocol_features;
 
+    /**
+     * @shared_uuids: contains the UUIDs of all the exported
+     * virtio objects owned by the vhost device.
+     */
+    GSList *shared_uuids;
+
     uint64_t max_queues;
     uint64_t backend_cap;
     /* @started: is the vhost device started? */
-- 
2.41.0

Re: [PATCH 2/3] hw/virtio: cleanup shared resources

Posted by Marc-André Lureau 11 months, 4 weeks ago

Hi

On Tue, Nov 7, 2023 at 1:37 PM Albert Esteve <aesteve@redhat.com> wrote:
>
> Ensure that we cleanup all virtio shared
> resources when the vhost devices is cleaned
> up (after a hot unplug, or a crash).
>
> To track all owned uuids of a device, add
> a GSList to the vhost_dev struct. This way
> we can avoid traversing the full table
> for every cleanup, whether they actually
> own any shared resource or not.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
>  hw/virtio/vhost-user.c    | 2 ++
>  hw/virtio/vhost.c         | 4 ++++
>  include/hw/virtio/vhost.h | 6 ++++++
>  3 files changed, 12 insertions(+)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 5fdff0241f..04848d1fa0 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1598,6 +1598,7 @@ vhost_user_backend_handle_shared_object_add(struct vhost_dev *dev,
>      QemuUUID uuid;
>
>      memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> +    dev->shared_uuids = g_slist_append(dev->shared_uuids, &uuid);

This will point to the stack variable.

>      return virtio_add_vhost_device(&uuid, dev);
>  }
>
> @@ -1623,6 +1624,7 @@ vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
>      }
>
>      memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> +    dev->shared_uuids = g_slist_remove_all(dev->shared_uuids, &uuid);
>      return virtio_remove_resource(&uuid);
>  }
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 9c9ae7109e..3aff94664b 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -16,6 +16,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "hw/virtio/vhost.h"
> +#include "hw/virtio/virtio-dmabuf.h"
>  #include "qemu/atomic.h"
>  #include "qemu/range.h"
>  #include "qemu/error-report.h"
> @@ -1599,6 +1600,9 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
>      migrate_del_blocker(&hdev->migration_blocker);
>      g_free(hdev->mem);
>      g_free(hdev->mem_sections);
> +    /* free virtio shared objects */
> +    g_slist_foreach(hdev->shared_uuids, (GFunc)virtio_remove_resource, NULL);
> +    g_slist_free_full(g_steal_pointer(&hdev->shared_uuids), g_free);

(and will crash here)

Imho, you should just traverse the hashtable, instead of introducing
another list.

>      if (hdev->vhost_ops) {
>          hdev->vhost_ops->vhost_backend_cleanup(hdev);
>      }
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 5e8183f64a..376bc8446d 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -118,6 +118,12 @@ struct vhost_dev {
>       */
>      uint64_t protocol_features;
>
> +    /**
> +     * @shared_uuids: contains the UUIDs of all the exported
> +     * virtio objects owned by the vhost device.
> +     */
> +    GSList *shared_uuids;
> +
>      uint64_t max_queues;
>      uint64_t backend_cap;
>      /* @started: is the vhost device started? */
> --
> 2.41.0
>


-- 
Marc-André Lureau

Re: [PATCH 2/3] hw/virtio: cleanup shared resources

Posted by Albert Esteve 11 months, 3 weeks ago

On Mon, Dec 4, 2023 at 9:00 AM Marc-André Lureau <marcandre.lureau@gmail.com>
wrote:

> Hi
>
> On Tue, Nov 7, 2023 at 1:37 PM Albert Esteve <aesteve@redhat.com> wrote:
> >
> > Ensure that we cleanup all virtio shared
> > resources when the vhost devices is cleaned
> > up (after a hot unplug, or a crash).
> >
> > To track all owned uuids of a device, add
> > a GSList to the vhost_dev struct. This way
> > we can avoid traversing the full table
> > for every cleanup, whether they actually
> > own any shared resource or not.
> >
> > Signed-off-by: Albert Esteve <aesteve@redhat.com>
> > ---
> >  hw/virtio/vhost-user.c    | 2 ++
> >  hw/virtio/vhost.c         | 4 ++++
> >  include/hw/virtio/vhost.h | 6 ++++++
> >  3 files changed, 12 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 5fdff0241f..04848d1fa0 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -1598,6 +1598,7 @@ vhost_user_backend_handle_shared_object_add(struct
> vhost_dev *dev,
> >      QemuUUID uuid;
> >
> >      memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> > +    dev->shared_uuids = g_slist_append(dev->shared_uuids, &uuid);
>
> This will point to the stack variable.
>
> >      return virtio_add_vhost_device(&uuid, dev);
> >  }
> >
> > @@ -1623,6 +1624,7 @@
> vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
> >      }
> >
> >      memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> > +    dev->shared_uuids = g_slist_remove_all(dev->shared_uuids, &uuid);
> >      return virtio_remove_resource(&uuid);
> >  }
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 9c9ae7109e..3aff94664b 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -16,6 +16,7 @@
> >  #include "qemu/osdep.h"
> >  #include "qapi/error.h"
> >  #include "hw/virtio/vhost.h"
> > +#include "hw/virtio/virtio-dmabuf.h"
> >  #include "qemu/atomic.h"
> >  #include "qemu/range.h"
> >  #include "qemu/error-report.h"
> > @@ -1599,6 +1600,9 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
> >      migrate_del_blocker(&hdev->migration_blocker);
> >      g_free(hdev->mem);
> >      g_free(hdev->mem_sections);
> > +    /* free virtio shared objects */
> > +    g_slist_foreach(hdev->shared_uuids, (GFunc)virtio_remove_resource,
> NULL);
> > +    g_slist_free_full(g_steal_pointer(&hdev->shared_uuids), g_free);
>
> (and will crash here)
>
> Imho, you should just traverse the hashtable, instead of introducing
> another list.
>

Ok, I was probably doing premature optimization. I guess it should
not happen as often, or track as many resources, as to require
a separate list. I will just traverse.

Thanks!


>
> >      if (hdev->vhost_ops) {
> >          hdev->vhost_ops->vhost_backend_cleanup(hdev);
> >      }
> > diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> > index 5e8183f64a..376bc8446d 100644
> > --- a/include/hw/virtio/vhost.h
> > +++ b/include/hw/virtio/vhost.h
> > @@ -118,6 +118,12 @@ struct vhost_dev {
> >       */
> >      uint64_t protocol_features;
> >
> > +    /**
> > +     * @shared_uuids: contains the UUIDs of all the exported
> > +     * virtio objects owned by the vhost device.
> > +     */
> > +    GSList *shared_uuids;
> > +
> >      uint64_t max_queues;
> >      uint64_t backend_cap;
> >      /* @started: is the vhost device started? */
> > --
> > 2.41.0
> >
>
>
> --
> Marc-André Lureau
>
>

[PATCH 1/3] hw/virtio: check owner for removing objects
[PATCH 2/3] hw/virtio: cleanup shared resources
[PATCH 3/3] hw/virtio: rename virtio dmabuf API