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

[Jason Wang]

在 2022/7/11 17:56, 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.


For the ones that requires more metadata, we can store it in elem->opaque?


> > 2) store pointers to ring_id_maps
> I think you mean to keep storing VirtQueueElement at ring_id_maps?


Yes and introduce a pointer to metadata in VirtQueueElement


> 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"?


Or let's having a dedicated arrays (like desc_state/desc_extra in 
kernel) instead of trying to reuse ring_id_maps.

Thanks


> 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);
> > >            }