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

[Jason Wang]

On Tue, Jul 12, 2022 at 4:33 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Tue, Jul 12, 2022 at 9:53 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > 在 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?
> >
>
> But there is no VirtQueueElem there. VirtQueueElem is allocated by
> virtqueue_pop, but state restoring messages are not received by this
> function. If we allocate an artificial one, a lot of members do not
> make sense (like in_addr / out_addr), and we should never use them
> with virtqueue_push / fill / flush and similar.

Ok.

>
> >
> > >
> > >> 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.
> >
>
> Sure, it looks to me like:
> * renaming ring_id_maps to desc_state/desc_extra/something similar,
> since now it's used to store more state that only the guest mapping
> * Rename "opaque" to "data"
> * Forget the wrapper and assume data == NULL is an invalid head /
> empty. To me they serve as a doc, but I guess it's fine to use them
> directly. The kernel works that way anyway.
>
> Does this look better?

Yes.

> It's definitely closer to the kernel so I guess
> it's an advantage.

I think the advantage is that it decouples the dynamic allocated
metadata (VirtQueueElem) out of the static allocated ones.

>
> Thanks!
>