Re: [RFC 1/2] virtio: expose used buffers

[Eugenio Perez Martin]

On Thu, Aug 18, 2022 at 5:13 PM Guo Zhi <qtxuning1999@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 <qtxuning1999@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.

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

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
>