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++) {
+ g_free(elems[j]);
+ }
+
+ virtqueue_flush(q->tx_vq, num_packets);
+ virtio_notify(vdev, q->tx_vq);
+ }
+
return num_packets;
}
--
2.17.1On Thu, Aug 18, 2022 at 11: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.
Yes, but I don't see anything that is related to the "exposing used
buffers", it looks more like batching used buffers etc.
>
> 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);
So virtqueue_fill will call virtqueue_unmap_sg(), won't this cause
mapping to be leaked?
And I think playing batching in device specific code seems
sub-optimal, e.g if we want to implement in-order in block we need
duplicate the logic here.
Thanks
> + for (j = 0; j < num_packets; j++) {
> + g_free(elems[j]);
> + }
> +
> + virtqueue_flush(q->tx_vq, num_packets);
> + virtio_notify(vdev, q->tx_vq);
> + }
> +
> return num_packets;
> }
>
> --
> 2.17.1
>
----- Original Message -----
> From: "jasowang" <jasowang@redhat.com>
> To: "Guo Zhi" <qtxuning1999@sjtu.edu.cn>
> Cc: "eperezma" <eperezma@redhat.com>, "sgarzare" <sgarzare@redhat.com>, "Michael Tsirkin" <mst@redhat.com>,
> "qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>
> Sent: Thursday, August 25, 2022 2:06:11 PM
> Subject: Re: [RFC 1/2] virtio: expose used buffers
> On Thu, Aug 18, 2022 at 11: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.
>
> Yes, but I don't see anything that is related to the "exposing used
> buffers", it looks more like batching used buffers etc.
>
>>
>> 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);
>
> So virtqueue_fill will call virtqueue_unmap_sg(), won't this cause
> mapping to be leaked?
>
Virtqueue_push was virtqueue_vill + virtqueue_flush already, and I will call vq_detach_element for exposed buffers.
> And I think playing batching in device specific code seems
> sub-optimal, e.g if we want to implement in-order in block we need
> duplicate the logic here.
>
Only the device knows if the batching is possible,
We have the same problem as in vhost-kernel in this regard, and it was proposed to do per-device there.
> Thanks
>
>> + for (j = 0; j < num_packets; j++) {
>> + g_free(elems[j]);
>> + }
>> +
>> + virtqueue_flush(q->tx_vq, num_packets);
>> + virtio_notify(vdev, q->tx_vq);
>> + }
>> +
>> return num_packets;
>> }
>>
>> --
>> 2.17.1
>>
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
>
----- 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);
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
>>
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!
© 2016 - 2025 Red Hat, Inc.