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

[Eugenio Perez Martin]

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.

>
> >
> >> 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? It's definitely closer to the kernel so I guess
it's an advantage.

Thanks!