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

[Alex Bennée]

Albert Esteve <aesteve@redhat.com> writes:

> On Tue, Feb 6, 2024 at 12:11 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
>  Albert Esteve <aesteve@redhat.com> writes:
>
>  > Ensure that we cleanup all virtio shared
>  > resources when the vhost devices is cleaned
>  > up (after a hot unplug, or a crash).
>  >
>  > To do so, we add a new function to the virtio_dmabuf
>  > API called `virtio_dmabuf_vhost_cleanup`, which
>  > loop through the table and removes all
>  > resources owned by the vhost device parameter.
>  >
>  > Also, add a test to verify that the new
>  > function in the API behaves as expected.
>  >
>  > Signed-off-by: Albert Esteve <aesteve@redhat.com>
>  > Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>  > ---
>  >  hw/display/virtio-dmabuf.c        | 22 +++++++++++++++++++++
>  >  hw/virtio/vhost.c                 |  3 +++
>  >  include/hw/virtio/virtio-dmabuf.h | 10 ++++++++++
>  >  tests/unit/test-virtio-dmabuf.c   | 33 +++++++++++++++++++++++++++++++
>  >  4 files changed, 68 insertions(+)
>  >
>  > diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
>  > index 3dba4577ca..6688809777 100644
>  > --- a/hw/display/virtio-dmabuf.c
>  > +++ b/hw/display/virtio-dmabuf.c
>  > @@ -136,6 +136,28 @@ SharedObjectType virtio_object_type(const QemuUUID 
> *uuid)
>  >      return vso->type;
>  >  }
>  >  
>  > +static bool virtio_dmabuf_resource_is_owned(gpointer key,
>  > +                                            gpointer value,
>  > +                                            gpointer dev)
>  > +{
>  > +    VirtioSharedObject *vso;
>  > +
>  > +    vso = (VirtioSharedObject *) value;
>  > +    return vso->type == TYPE_VHOST_DEV && vso->value == dev;
>
>  It's a bit surprising to see vso->value being an anonymous gpointer
>  rather than the proper type and a bit confusing between value and
>  vso->value.
>
> It is the signature required for this to be used with 
> `g_hash_table_foreach_remove`.
> For the naming, the HashMap stores gpointers, that point to 
> `VirtioSharedObject`, and
> these point to the underlying type (stored at `vso->value`). It may sound a 
> bit confusing,
> but is a byproduct of the VirtioSharedObject indirection. Not sure which 
> names could be
> more fit for this, but I'm open to change them.

This is the problem without overloading value and vso->value. I
appreciate that virtio_dmabuf_resource_is_owned() has to follow the
signature for g_hash_table_foreach_remove but usually the compare
function then casts gpointer to the underlying type for any comparison.

So something like:

  typedef struct VirtioSharedObject {
      SharedObjectType type;
      union {
            vhost_dev *dev; /* TYPE_VHOST_DEV */
            int udma_buf;   /* TYPE_DMABUF */
      } value;
  } VirtioSharedObject;

and then you would have:

  VirtioSharedObject *vso = value;
  if (vso->type == TYPE_VHOST_DEV) {
     vhost_dev *dev = dev;
     return vso->value.dev == dev;
  }

In fact I think you can skip the cast so have:

  VirtioSharedObject *vso = value;
  return vso->type == TYPE_VHOST_DEV && vso->value.dev == dev;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro