[RFC 0/2] Virtio in order feature support for virtio-net device.

[RFC 0/2] Virtio in order feature support for virtio-net device.

[Guo Zhi]

In virtio-spec 1.1, new feature bit VIRTIO_F_IN_ORDER was introduced.
When this feature has been negotiated, virtio driver will use
descriptors in ring order: starting from offset 0 in the table, and
wrapping around at the end of the table. Virtio devices will always use
descriptors in the same order in which they have been made available.
This can reduce virtio accesses to used ring.

Based on updated virtio-spec, this series realized IN_ORDER prototype
for virtio-net device in QEMU.

Some work haven't been done in this patch series:
1. Virtio device in_order support for packed vq is left for the future.

Related patches:
In order feature in Linux(support virtio driver, vhost_test and vsock device): https://lkml.org/lkml/2022/8/17/643

Guo Zhi (2):
  virtio: expose used buffers
  virtio: enable f_in_order feature for virtio-net

 hw/net/virtio-net.c        | 30 +++++++++++++++++++++++++++---
 include/hw/virtio/virtio.h |  4 +++-
 2 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.17.1

[RFC 1/2] virtio: expose used buffers

[Guo Zhi]

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

[RFC 2/2] virtio: enable f_in_order feature for virtio-net

[Guo Zhi]

In order feature is not a transparent feature in QEMU, only specific
devices(eg, virtio-net) support it.

Signed-off-by: Guo Zhi <qtxuning1999@sjtu.edu.cn>
---
 hw/net/virtio-net.c        | 1 +
 include/hw/virtio/virtio.h | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index c8e83921..cf0b23d8 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -719,6 +719,7 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
     features |= n->host_features;
 
     virtio_add_feature(&features, VIRTIO_NET_F_MAC);
+    virtio_add_feature(&features, VIRTIO_F_IN_ORDER);
 
     if (!peer_has_vnet_hdr(n)) {
         virtio_clear_feature(&features, VIRTIO_NET_F_CSUM);
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index db1c0ddf..578f22c8 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -291,7 +291,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
     DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
                       VIRTIO_F_IOMMU_PLATFORM, false), \
     DEFINE_PROP_BIT64("packed", _state, _field, \
-                      VIRTIO_F_RING_PACKED, false)
+                      VIRTIO_F_RING_PACKED, false), \
+    DEFINE_PROP_BIT64("in_order", _state, _field, \
+                      VIRTIO_F_IN_ORDER, false)
 
 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
 bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
-- 
2.17.1

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
>

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

[Jason Wang]

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?

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
>

Re: [RFC 0/2] Virtio in order feature support for virtio-net device.

[Jason Wang]

On Thu, Aug 18, 2022 at 11:13 PM Guo Zhi <qtxuning1999@sjtu.edu.cn> wrote:
>
> In virtio-spec 1.1, new feature bit VIRTIO_F_IN_ORDER was introduced.
> When this feature has been negotiated, virtio driver will use
> descriptors in ring order: starting from offset 0 in the table, and
> wrapping around at the end of the table. Virtio devices will always use
> descriptors in the same order in which they have been made available.
> This can reduce virtio accesses to used ring.

So the bached used buffer seems like an optimization, I'd like
re-order the patches

1) Just enable virtio-net in-order without the batching, to see the performance
2) Add the batching optimization on top.

>
> Based on updated virtio-spec, this series realized IN_ORDER prototype
> for virtio-net device in QEMU.

Please share with us some performance numbers. E.g pps.

>
> Some work haven't been done in this patch series:
> 1. Virtio device in_order support for packed vq is left for the future.

Anything makes packed virtqueue special?

Thanks

>
> Related patches:
> In order feature in Linux(support virtio driver, vhost_test and vsock device): https://lkml.org/lkml/2022/8/17/643
>
> Guo Zhi (2):
>   virtio: expose used buffers
>   virtio: enable f_in_order feature for virtio-net
>
>  hw/net/virtio-net.c        | 30 +++++++++++++++++++++++++++---
>  include/hw/virtio/virtio.h |  4 +++-
>  2 files changed, 30 insertions(+), 4 deletions(-)
>
> --
> 2.17.1
>

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

[Guo Zhi]

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

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!

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

[Guo Zhi]

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