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

[Jason Wang <jasowang@redhat.com>]

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