----- Original Message -----
> From: "eperezma" <epere...@redhat.com>
> To: "Guo Zhi" <qtxuning1...@sjtu.edu.cn>
> Cc: "jasowang" <jasow...@redhat.com>, "sgarzare" <sgarz...@redhat.com>,
> "Michael Tsirkin" <m...@redhat.com>,
> "qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>
> Sent: Monday, August 22, 2022 10:08:32 PM
> Subject: Re: [RFC 1/2] virtio: expose used buffers
> On Thu, Aug 18, 2022 at 5:13 PM Guo Zhi <qtxuning1...@sjtu.edu.cn> wrote:
>>
>> Follow VIRTIO 1.1 spec, we can only writing out a single used ring for a
>> batch of descriptors, and only notify guest when the batch of
>> descriptors have all been used.
>>
>> We do that batch for tx, because the driver doesn't need to know the
>> length of tx buffer, while for rx, we don't apply the batch strategy.
>>
>> Signed-off-by: Guo Zhi <qtxuning1...@sjtu.edu.cn>
>> ---
>> hw/net/virtio-net.c | 29 ++++++++++++++++++++++++++---
>> 1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index dd0d056f..c8e83921 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -2542,8 +2542,10 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>> VirtIONet *n = q->n;
>> VirtIODevice *vdev = VIRTIO_DEVICE(n);
>> VirtQueueElement *elem;
>> + VirtQueueElement *elems[VIRTQUEUE_MAX_SIZE];
>> int32_t num_packets = 0;
>> int queue_index = vq2q(virtio_get_queue_index(q->tx_vq));
>> + size_t j;
>> if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
>> return num_packets;
>> }
>> @@ -2621,14 +2623,35 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>> }
>>
>> drop:
>> - virtqueue_push(q->tx_vq, elem, 0);
>> - virtio_notify(vdev, q->tx_vq);
>> - g_free(elem);
>> + if (!virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER)) {
>> + virtqueue_push(q->tx_vq, elem, 0);
>> + virtio_notify(vdev, q->tx_vq);
>> + g_free(elem);
>> + } else {
>> + elems[num_packets] = elem;
>> + }
>>
>> if (++num_packets >= n->tx_burst) {
>> break;
>> }
>> }
>> +
>> + if (virtio_vdev_has_feature(vdev, VIRTIO_F_IN_ORDER) && num_packets) {
>> + /**
>> + * If in order feature negotiated, devices can notify the use of a
>> batch
>> + * of buffers to the driver by only writing out a single used ring
>> entry
>> + * with the id corresponding to the head entry of the descriptor
>> chain
>> + * describing the last buffer in the batch.
>> + */
>> + virtqueue_fill(q->tx_vq, elems[num_packets - 1], 0, 0);
>> + for (j = 0; j < num_packets; j++) {
>
> There are a few calls on virtqueue_pop that we need to keep cleaning
> here. For example, the increment on vq->inuse or dma_memory_map/unmap.
> Maybe it is ok to call virtqueue_detach_element here for all skipped
> buffers of the batch, but I haven't reviewed it in depth.
Yeah, I think I should call virtqueue_detach_element for skipped buffers.
>
> Also, if we want to batch, we must increment used idx accordingly.
> From the standard, "The device then skips forward in the [used] ring
> according to the size of the batch. Accordingly, it increments the
> used idx by the size of the batch."
>
used_idx has been added by the size of the batch, because of this:
virtqueue_flush(q->tx_vq, num_packets);
Thanks!
> If we are sure virtio-net device will use tx virtqueue in order, maybe
> it is better to enable the in order feature bit before and then do the
> batching on top.
>
> Thanks!
>
>> + g_free(elems[j]);
>> + }
>> +
>> + virtqueue_flush(q->tx_vq, num_packets);
>> + virtio_notify(vdev, q->tx_vq);
>> + }
>> +
>> return num_packets;
>> }
>>
>> --
>> 2.17.1
>>