Re: [RFC PATCH v9 12/23] vhost: Add opaque member to SVQElement

[Eugenio Perez Martin]

On Mon, Jul 11, 2022 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/7/7 02:39, Eugenio Pérez 写道:
> > When qemu injects buffers to the vdpa device it will be used to maintain
> > contextual data. If SVQ has no operation, it will be used to maintain
> > the VirtQueueElement pointer.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-shadow-virtqueue.h |  3 ++-
> >   hw/virtio/vhost-shadow-virtqueue.c | 13 +++++++------
> >   2 files changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.h 
> > b/hw/virtio/vhost-shadow-virtqueue.h
> > index 0e434e9fd0..a811f90e01 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.h
> > +++ b/hw/virtio/vhost-shadow-virtqueue.h
> > @@ -16,7 +16,8 @@
> >   #include "hw/virtio/vhost-iova-tree.h"
> >
> >   typedef struct SVQElement {
> > -    VirtQueueElement *elem;
> > +    /* Opaque data */
> > +    void *opaque;
>
>
> So I wonder if we can simply:
>
> 1) introduce a opaque to VirtQueueElement

(answered in other thread, pasting here for completion)

It does not work for messages that are not generated by the guest. For
example, the ones used to restore the device state at live migration
destination.

> 2) store pointers to ring_id_maps
>

I think you mean to keep storing VirtQueueElement at ring_id_maps?
Otherwise, looking for them will not be immediate.

> Since
>
> 1) VirtQueueElement's member looks general

Not general enough :).

> 2) help to reduce the tricky codes like vhost_svq_empty_elem() and
> vhost_svq_empty_elem().
>

I'm ok to change to whatever other method, but to allocate them
individually seems worse to me. Both performance wise and because
error paths are more complicated. Maybe it would be less tricky if I
try to move the use of them less "by value" and more "as pointers"?

Thanks!

> Thanks
>
>
> >
> >       /* Last descriptor of the chain */
> >       uint32_t last_chain_id;
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> > b/hw/virtio/vhost-shadow-virtqueue.c
> > index c5e49e51c5..492bb12b5f 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -237,7 +237,7 @@ static uint16_t vhost_svq_last_desc_of_chain(const 
> > VhostShadowVirtqueue *svq,
> >    */
> >   static bool vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec 
> > *out_sg,
> >                             size_t out_num, const struct iovec *in_sg,
> > -                          size_t in_num, VirtQueueElement *elem)
> > +                          size_t in_num, void *opaque)
> >   {
> >       SVQElement *svq_elem;
> >       unsigned qemu_head;
> > @@ -245,13 +245,12 @@ static bool vhost_svq_add(VhostShadowVirtqueue *svq, 
> > const struct iovec *out_sg,
> >       bool ok = vhost_svq_add_split(svq, out_sg, out_num, in_sg, in_num,
> >                                     &qemu_head);
> >       if (unlikely(!ok)) {
> > -        g_free(elem);
> >           return false;
> >       }
> >
> >       n = out_num + in_num;
> >       svq_elem = &svq->ring_id_maps[qemu_head];
> > -    svq_elem->elem = elem;
> > +    svq_elem->opaque = opaque;
> >       svq_elem->last_chain_id = vhost_svq_last_desc_of_chain(svq, n, 
> > qemu_head);
> >       return true;
> >   }
> > @@ -277,6 +276,8 @@ static bool vhost_svq_add_element(VhostShadowVirtqueue 
> > *svq,
> >                               elem->in_num, elem);
> >       if (ok) {
> >           vhost_svq_kick(svq);
> > +    } else {
> > +        g_free(elem);
> >       }
> >
> >       return ok;
> > @@ -392,7 +393,7 @@ static void 
> > vhost_svq_disable_notification(VhostShadowVirtqueue *svq)
> >
> >   static bool vhost_svq_is_empty_elem(SVQElement elem)
> >   {
> > -    return elem.elem == NULL;
> > +    return elem.opaque == NULL;
> >   }
> >
> >   static SVQElement vhost_svq_empty_elem(void)
> > @@ -483,7 +484,7 @@ static void vhost_svq_flush(VhostShadowVirtqueue *svq,
> >                   break;
> >               }
> >
> > -            elem = g_steal_pointer(&svq_elem.elem);
> > +            elem = g_steal_pointer(&svq_elem.opaque);
> >               virtqueue_fill(vq, elem, len, i++);
> >           }
> >
> > @@ -651,7 +652,7 @@ void vhost_svq_stop(VhostShadowVirtqueue *svq)
> >
> >       for (unsigned i = 0; i < svq->vring.num; ++i) {
> >           g_autofree VirtQueueElement *elem = NULL;
> > -        elem = g_steal_pointer(&svq->ring_id_maps[i].elem);
> > +        elem = g_steal_pointer(&svq->ring_id_maps[i].opaque);
> >           if (elem) {
> >               virtqueue_detach_element(svq->vq, elem, 0);
> >           }
>