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

[Eugenio Perez Martin]

On Thu, Aug 25, 2022 at 9:16 AM Guo Zhi <qtxuning1999@sjtu.edu.cn> wrote:
>
>
>
> ----- Original Message -----
> > From: "eperezma" <eperezma@redhat.com>
> > To: "Guo Zhi" <qtxuning1999@sjtu.edu.cn>
> > Cc: "jasowang" <jasowang@redhat.com>, "sgarzare" <sgarzare@redhat.com>, 
> > "Michael Tsirkin" <mst@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 <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.
>
> 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);
>

Oh, right, good point!

Thanks!