| Message ID | 20231107093744.388099-3-aesteve@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Virtio dmabuf improvements | expand |
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 >
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 > >
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? */
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(+)