virtio-net: Make virtio queue sizes configurable

PATCH In review Patrik Hermansson 3 participants 8 messages Nov 27, 2015 – Mar 15, 2016

Share & tools

Patrik Hermansson

author

2015-11-27 15:03 UTC #0 lore raw

PATCH 1/1 virtio-net: Make virtio queue sizes configurable

This patch adds the option to specify virtio queue sizes. Currently the queue sizes is hard coded to 256, which might not be suitable for all types of applications. This patch makes it possible to specify the queue size between 256 to 1024.

The minimum value is chosen based on the current sizes of the virtio queues. The maximum queue size is based upon VIRTQUEUE_SIZE_MAX.

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

hw/net/virtio-net.c +28 -3

index a877614..c4fcb39 100644
@@ -1326,20 +1326,40 @@ static void virtio_net_tx_bh(void *opaque)
     }
 }
 
+static void virtio_net_validate_queue_limit(uint16_t *queue_size)
+{
+    if (*queue_size > VIRTIO_QUEUE_SIZE_MAX) {
+        error_report("queue-size: %d, exceeds maximum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MAX,
+                      VIRTIO_QUEUE_SIZE_MAX);
+        *queue_size = VIRTIO_QUEUE_SIZE_MAX;
+    } else if (*queue_size < VIRTIO_QUEUE_SIZE_MIN) {
+        error_report("queue-size: %d, below minimum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MIN,
+                      VIRTIO_QUEUE_SIZE_MIN);
+        *queue_size = VIRTIO_QUEUE_SIZE_MIN;
+    }
+}
+
 static void virtio_net_add_queue(VirtIONet *n, int index)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
 
-    n->vqs[index].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx);
+    virtio_net_validate_queue_limit(&n->net_conf.rx_virtqueue_sz);
+    virtio_net_validate_queue_limit(&n->net_conf.tx_virtqueue_sz);
+    n->vqs[index].rx_vq = virtio_add_queue(vdev, n->net_conf.rx_virtqueue_sz,
+                                           virtio_net_handle_rx);
     if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer);
+            virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz,
+                             virtio_net_handle_tx_timer);
         n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                               virtio_net_tx_timer,
                                               &n->vqs[index]);
     } else {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh);
+            virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz,
+                             virtio_net_handle_tx_bh);
         n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]);
     }

@@ -1826,6 +1846,11 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer,
                        TX_TIMER_INTERVAL),
     DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST),
+    DEFINE_PROP_UINT16("rx_virtqueue_sz", VirtIONet, net_conf.rx_virtqueue_sz,
+                       RX_VIRTQUEUE_SIZE),
+    DEFINE_PROP_UINT16("tx_virtqueue_sz", VirtIONet, net_conf.tx_virtqueue_sz,
+                       TX_VIRTQUEUE_SIZE),
+
     DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx),
     DEFINE_PROP_END_OF_LIST(),
 };

include/hw/virtio/virtio-net.h +7 -1

index f3cc25f..dbcabbf 100644
@@ -28,13 +28,19 @@
  * ensures fairness in the io path.  256 conveniently matches the
  * length of the TX queue and shows a good balance of performance
  * and latency. */
-#define TX_BURST 256
+#define VIRTIO_QUEUE_SIZE_MIN 256
+#define VIRTIO_QUEUE_SIZE_MAX VIRTQUEUE_MAX_SIZE
+#define RX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN
+#define TX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN
+#define TX_BURST TX_VIRTQUEUE_SIZE
 
 typedef struct virtio_net_conf
 {
     uint32_t txtimer;
     int32_t txburst;
     char *tx;
+    uint16_t rx_virtqueue_sz;
+    uint16_t tx_virtqueue_sz;
 } virtio_net_conf;
 
 /* Maximum packet size we can receive from tap device: header + 64k */

Signature · 1 line

1.9.1

Greg Kurz

2016-03-14 11:38 UTC #1 lore raw

Quoted 8 lines · On Fri, 27 Nov 2015 16:02:39 +0100 Patrik Hermansson <[email protected]>…

This patch adds the option to specify virtio queue sizes. Currently the queue sizes is hard coded to 256, which might not be suitable for all types of applications. This patch makes it possible to specify the queue size between 256 to 1024.
The minimum value is chosen based on the current sizes of the virtio queues. The maximum queue size is based upon VIRTQUEUE_SIZE_MAX.
---

Michael, any thoughts on this patch ?

Patrik, I have a few remarks. See below.

SoB tag is missing.

Quoted 9 lines

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

hw/net/virtio-net.c +0 -0

index a877614..c4fcb39 100644

hw/net/virtio-net.c static void virtio_net_tx_bh(void *opaque) · line 1333

Quoted 8 lines

hw/net/virtio-net.c +5 -0

@@ -1326,20 +1326,40 @@ static void virtio_net_tx_bh(void *opaque)
     }
 }
 
+static void virtio_net_validate_queue_limit(uint16_t *queue_size)
+{
+    if (*queue_size > VIRTIO_QUEUE_SIZE_MAX) {
+        error_report("queue-size: %d, exceeds maximum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MAX,

It is encouraged to keep format strings passed to error_report() on 1 line. It is easier to grep.

hw/net/virtio-net.c static void virtio_net_tx_bh(void *opaque) · line 1338

Quoted 5 lines

hw/net/virtio-net.c +5 -0

+                      VIRTIO_QUEUE_SIZE_MAX);
+        *queue_size = VIRTIO_QUEUE_SIZE_MAX;
+    } else if (*queue_size < VIRTIO_QUEUE_SIZE_MIN) {
+        error_report("queue-size: %d, below minimum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MIN,

Same remark.

Quoted 47 lines

hw/net/virtio-net.c +18 -3

+                      VIRTIO_QUEUE_SIZE_MIN);
+        *queue_size = VIRTIO_QUEUE_SIZE_MIN;
+    }
+}
+
 static void virtio_net_add_queue(VirtIONet *n, int index)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
 
-    n->vqs[index].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx);
+    virtio_net_validate_queue_limit(&n->net_conf.rx_virtqueue_sz);
+    virtio_net_validate_queue_limit(&n->net_conf.tx_virtqueue_sz);
+    n->vqs[index].rx_vq = virtio_add_queue(vdev, n->net_conf.rx_virtqueue_sz,
+                                           virtio_net_handle_rx);
     if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer);
+            virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz,
+                             virtio_net_handle_tx_timer);
         n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                               virtio_net_tx_timer,
                                               &n->vqs[index]);
     } else {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh);
+            virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz,
+                             virtio_net_handle_tx_bh);
         n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]);
     }

@@ -1826,6 +1846,11 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer,
                        TX_TIMER_INTERVAL),
     DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST),
+    DEFINE_PROP_UINT16("rx_virtqueue_sz", VirtIONet, net_conf.rx_virtqueue_sz,
+                       RX_VIRTQUEUE_SIZE),
+    DEFINE_PROP_UINT16("tx_virtqueue_sz", VirtIONet, net_conf.tx_virtqueue_sz,
+                       TX_VIRTQUEUE_SIZE),
+
     DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx),
     DEFINE_PROP_END_OF_LIST(),
 };

include/hw/virtio/virtio-net.h +0 -0

index f3cc25f..dbcabbf 100644

include/hw/virtio/virtio-net.h line 31

Quoted 4 lines

include/hw/virtio/virtio-net.h +0 -1

@@ -28,13 +28,19 @@
  * ensures fairness in the io path.  256 conveniently matches the
  * length of the TX queue and shows a good balance of performance
  * and latency. */
-#define TX_BURST 256

TX_BURST is still used by:

DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST)

Quoted 16 lines

include/hw/virtio/virtio-net.h +7 -0

+#define VIRTIO_QUEUE_SIZE_MIN 256
+#define VIRTIO_QUEUE_SIZE_MAX VIRTQUEUE_MAX_SIZE
+#define RX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN
+#define TX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN
+#define TX_BURST TX_VIRTQUEUE_SIZE
 
 typedef struct virtio_net_conf
 {
     uint32_t txtimer;
     int32_t txburst;
     char *tx;
+    uint16_t rx_virtqueue_sz;
+    uint16_t tx_virtqueue_sz;
 } virtio_net_conf;
 
 /* Maximum packet size we can receive from tap device: header + 64k */

Greg Kurz

2016-03-15 07:59 UTC #2 lore raw

Quoted 101 lines · On Mon, 14 Mar 2016 12:27:55 +0100 Greg Kurz <[email protected]> wrote:

Quoted 8 lines · On Fri, 27 Nov 2015 16:02:39 +0100 Patrik Hermansson <[email protected]>…

This patch adds the option to specify virtio queue sizes. Currently the queue sizes is hard coded to 256, which might not be suitable for all types of applications. This patch makes it possible to specify the queue size between 256 to 1024.
The minimum value is chosen based on the current sizes of the virtio queues. The maximum queue size is based upon VIRTQUEUE_SIZE_MAX.
---

Michael, any thoughts on this patch ?

Patrik, I have a few remarks. See below.

SoB tag is missing.

Quoted 9 lines

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

hw/net/virtio-net.c +0 -0

index a877614..c4fcb39 100644

hw/net/virtio-net.c static void virtio_net_tx_bh(void *opaque) · line 1333

Quoted 8 lines

hw/net/virtio-net.c +5 -0

@@ -1326,20 +1326,40 @@ static void virtio_net_tx_bh(void *opaque)
     }
 }
 
+static void virtio_net_validate_queue_limit(uint16_t *queue_size)
+{
+    if (*queue_size > VIRTIO_QUEUE_SIZE_MAX) {
+        error_report("queue-size: %d, exceeds maximum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MAX,

It is encouraged to keep format strings passed to error_report() on 1 line. It is easier to grep.

hw/net/virtio-net.c static void virtio_net_tx_bh(void *opaque) · line 1338

Quoted 5 lines

hw/net/virtio-net.c +5 -0

+                      VIRTIO_QUEUE_SIZE_MAX);
+        *queue_size = VIRTIO_QUEUE_SIZE_MAX;
+    } else if (*queue_size < VIRTIO_QUEUE_SIZE_MIN) {
+        error_report("queue-size: %d, below minimum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MIN,

Same remark.

Quoted 47 lines

hw/net/virtio-net.c +18 -3

+                      VIRTIO_QUEUE_SIZE_MIN);
+        *queue_size = VIRTIO_QUEUE_SIZE_MIN;
+    }
+}
+
 static void virtio_net_add_queue(VirtIONet *n, int index)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
 
-    n->vqs[index].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx);
+    virtio_net_validate_queue_limit(&n->net_conf.rx_virtqueue_sz);
+    virtio_net_validate_queue_limit(&n->net_conf.tx_virtqueue_sz);
+    n->vqs[index].rx_vq = virtio_add_queue(vdev, n->net_conf.rx_virtqueue_sz,
+                                           virtio_net_handle_rx);
     if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer);
+            virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz,
+                             virtio_net_handle_tx_timer);
         n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                               virtio_net_tx_timer,
                                               &n->vqs[index]);
     } else {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh);
+            virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz,
+                             virtio_net_handle_tx_bh);
         n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]);
     }

@@ -1826,6 +1846,11 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer,
                        TX_TIMER_INTERVAL),
     DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST),
+    DEFINE_PROP_UINT16("rx_virtqueue_sz", VirtIONet, net_conf.rx_virtqueue_sz,
+                       RX_VIRTQUEUE_SIZE),
+    DEFINE_PROP_UINT16("tx_virtqueue_sz", VirtIONet, net_conf.tx_virtqueue_sz,
+                       TX_VIRTQUEUE_SIZE),
+
     DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx),
     DEFINE_PROP_END_OF_LIST(),
 };

include/hw/virtio/virtio-net.h +0 -0

index f3cc25f..dbcabbf 100644

include/hw/virtio/virtio-net.h line 31

Quoted 4 lines

include/hw/virtio/virtio-net.h +0 -1

@@ -28,13 +28,19 @@
  * ensures fairness in the io path.  256 conveniently matches the
  * length of the TX queue and shows a good balance of performance
  * and latency. */
-#define TX_BURST 256

TX_BURST is still used by:

Quoted 2 lines

DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST)

/me found his missing eye and read one line further...

Quoted 6 lines

include/hw/virtio/virtio-net.h line 36

Quoted 6 lines

include/hw/virtio/virtio-net.h +5 -0

+#define VIRTIO_QUEUE_SIZE_MIN 256
+#define VIRTIO_QUEUE_SIZE_MAX VIRTQUEUE_MAX_SIZE
+#define RX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN
+#define TX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN
+#define TX_BURST TX_VIRTQUEUE_SIZE

... please ignore the comment above :)

Quoted 13 lines

Quoted 10 lines

include/hw/virtio/virtio-net.h +2 -0

 typedef struct virtio_net_conf
 {
     uint32_t txtimer;
     int32_t txburst;
     char *tx;
+    uint16_t rx_virtqueue_sz;
+    uint16_t tx_virtqueue_sz;
 } virtio_net_conf;
 
 /* Maximum packet size we can receive from tap device: header + 64k */

Patrik Hermansson

author

2016-03-15 09:21 UTC #3 lore raw

In reply to Greg Kurz

Hi Greg,
Thanks for your comments and for taking the time.

2016-03-14 12:27 GMT+01:00 Greg Kurz <[email protected]>:

Quoted 2 lines

SoB tag is missing.

I will add this in the future.

Quoted 8 lines

hw/net/virtio-net.c static void virtio_net_tx_bh(void *opaque) · line 1333
Quoted 5 lines
hw/net/virtio-net.c +5 -0
+static void virtio_net_validate_queue_limit(uint16_t *queue_size)
+{
+    if (*queue_size > VIRTIO_QUEUE_SIZE_MAX) {
+        error_report("queue-size: %d, exceeds maximum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MAX,
It is encouraged to keep format strings passed to error_report() on 1 line. It is easier to grep.

Makes sense, I will fix this.

Quoted 7 lines

hw/net/virtio-net.c static void virtio_net_tx_bh(void *opaque) · line 1338

Quoted 5 lines

hw/net/virtio-net.c +5 -0

+                      VIRTIO_QUEUE_SIZE_MAX);
+        *queue_size = VIRTIO_QUEUE_SIZE_MAX;
+    } else if (*queue_size < VIRTIO_QUEUE_SIZE_MIN) {
+        error_report("queue-size: %d, below minimum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MIN,

Same remark.

See previous comment.

Thanks,
Patrik

Michael S. Tsirkin

2016-03-15 04:50 UTC #4 lore raw

In reply to Patrik Hermansson

Quoted 3 lines · Patrik Hermansson

This patch adds the option to specify virtio queue sizes. Currently the queue sizes is hard coded to 256, which might not be suitable for all types of applications.

Any more data here? 256 packets seems plenty. Is there a chance your application does not support indirect descriptors? That's a better approach I think.

Quoted 5 lines

This patch makes it possible to specify the queue size between 256 to 1024.
The minimum value is chosen based on the current sizes of the virtio queues. The maximum queue size is based upon VIRTQUEUE_SIZE_MAX.

To me, this seems excessive. For example, 1024 will break if we have to add a virtio net header entry (on old kernels).

Quoted 92 lines

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

hw/net/virtio-net.c +28 -3

index a877614..c4fcb39 100644
@@ -1326,20 +1326,40 @@ static void virtio_net_tx_bh(void *opaque)
     }
 }
 
+static void virtio_net_validate_queue_limit(uint16_t *queue_size)
+{
+    if (*queue_size > VIRTIO_QUEUE_SIZE_MAX) {
+        error_report("queue-size: %d, exceeds maximum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MAX,
+                      VIRTIO_QUEUE_SIZE_MAX);
+        *queue_size = VIRTIO_QUEUE_SIZE_MAX;
+    } else if (*queue_size < VIRTIO_QUEUE_SIZE_MIN) {
+        error_report("queue-size: %d, below minimum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MIN,
+                      VIRTIO_QUEUE_SIZE_MIN);
+        *queue_size = VIRTIO_QUEUE_SIZE_MIN;
+    }
+}
+
 static void virtio_net_add_queue(VirtIONet *n, int index)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
 
-    n->vqs[index].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx);
+    virtio_net_validate_queue_limit(&n->net_conf.rx_virtqueue_sz);
+    virtio_net_validate_queue_limit(&n->net_conf.tx_virtqueue_sz);
+    n->vqs[index].rx_vq = virtio_add_queue(vdev, n->net_conf.rx_virtqueue_sz,
+                                           virtio_net_handle_rx);
     if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer);
+            virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz,
+                             virtio_net_handle_tx_timer);
         n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                               virtio_net_tx_timer,
                                               &n->vqs[index]);
     } else {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh);
+            virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz,
+                             virtio_net_handle_tx_bh);
         n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]);
     }

@@ -1826,6 +1846,11 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer,
                        TX_TIMER_INTERVAL),
     DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST),
+    DEFINE_PROP_UINT16("rx_virtqueue_sz", VirtIONet, net_conf.rx_virtqueue_sz,
+                       RX_VIRTQUEUE_SIZE),
+    DEFINE_PROP_UINT16("tx_virtqueue_sz", VirtIONet, net_conf.tx_virtqueue_sz,
+                       TX_VIRTQUEUE_SIZE),
+
     DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx),
     DEFINE_PROP_END_OF_LIST(),
 };

include/hw/virtio/virtio-net.h +7 -1

index f3cc25f..dbcabbf 100644
@@ -28,13 +28,19 @@
  * ensures fairness in the io path.  256 conveniently matches the
  * length of the TX queue and shows a good balance of performance
  * and latency. */
-#define TX_BURST 256
+#define VIRTIO_QUEUE_SIZE_MIN 256
+#define VIRTIO_QUEUE_SIZE_MAX VIRTQUEUE_MAX_SIZE
+#define RX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN
+#define TX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN
+#define TX_BURST TX_VIRTQUEUE_SIZE
 
 typedef struct virtio_net_conf
 {
     uint32_t txtimer;
     int32_t txburst;
     char *tx;
+    uint16_t rx_virtqueue_sz;
+    uint16_t tx_virtqueue_sz;
 } virtio_net_conf;
 
 /* Maximum packet size we can receive from tap device: header + 64k */

Signature · 1 line

1.9.1

Greg Kurz

2016-03-15 08:00 UTC #5 lore raw

Quoted 9 lines · On Tue, 15 Mar 2016 06:50:38 +0200 "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 3 lines · Patrik Hermansson
This patch adds the option to specify virtio queue sizes. Currently the queue sizes is hard coded to 256, which might not be suitable for all types of applications.
Any more data here? 256 packets seems plenty. Is there a chance your application does not support indirect descriptors? That's a better approach I think.

Quoted 9 lines

Quoted 5 lines
This patch makes it possible to specify the queue size between 256 to 1024.
The minimum value is chosen based on the current sizes of the virtio queues. The maximum queue size is based upon VIRTQUEUE_SIZE_MAX.
To me, this seems excessive. For example, 1024 will break if we have to add a virtio net header entry (on old kernels).

Hi Michael,

Can you elaborate on what/how will break ?

Thanks.

--
Greg

Quoted 93 lines

Quoted 92 lines

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

hw/net/virtio-net.c +28 -3

index a877614..c4fcb39 100644
@@ -1326,20 +1326,40 @@ static void virtio_net_tx_bh(void *opaque)
     }
 }
 
+static void virtio_net_validate_queue_limit(uint16_t *queue_size)
+{
+    if (*queue_size > VIRTIO_QUEUE_SIZE_MAX) {
+        error_report("queue-size: %d, exceeds maximum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MAX,
+                      VIRTIO_QUEUE_SIZE_MAX);
+        *queue_size = VIRTIO_QUEUE_SIZE_MAX;
+    } else if (*queue_size < VIRTIO_QUEUE_SIZE_MIN) {
+        error_report("queue-size: %d, below minimum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MIN,
+                      VIRTIO_QUEUE_SIZE_MIN);
+        *queue_size = VIRTIO_QUEUE_SIZE_MIN;
+    }
+}
+
 static void virtio_net_add_queue(VirtIONet *n, int index)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
 
-    n->vqs[index].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx);
+    virtio_net_validate_queue_limit(&n->net_conf.rx_virtqueue_sz);
+    virtio_net_validate_queue_limit(&n->net_conf.tx_virtqueue_sz);
+    n->vqs[index].rx_vq = virtio_add_queue(vdev, n->net_conf.rx_virtqueue_sz,
+                                           virtio_net_handle_rx);
     if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer);
+            virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz,
+                             virtio_net_handle_tx_timer);
         n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                               virtio_net_tx_timer,
                                               &n->vqs[index]);
     } else {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh);
+            virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz,
+                             virtio_net_handle_tx_bh);
         n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]);
     }

@@ -1826,6 +1846,11 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer,
                        TX_TIMER_INTERVAL),
     DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST),
+    DEFINE_PROP_UINT16("rx_virtqueue_sz", VirtIONet, net_conf.rx_virtqueue_sz,
+                       RX_VIRTQUEUE_SIZE),
+    DEFINE_PROP_UINT16("tx_virtqueue_sz", VirtIONet, net_conf.tx_virtqueue_sz,
+                       TX_VIRTQUEUE_SIZE),
+
     DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx),
     DEFINE_PROP_END_OF_LIST(),
 };

include/hw/virtio/virtio-net.h +7 -1

index f3cc25f..dbcabbf 100644
@@ -28,13 +28,19 @@
  * ensures fairness in the io path.  256 conveniently matches the
  * length of the TX queue and shows a good balance of performance
  * and latency. */
-#define TX_BURST 256
+#define VIRTIO_QUEUE_SIZE_MIN 256
+#define VIRTIO_QUEUE_SIZE_MAX VIRTQUEUE_MAX_SIZE
+#define RX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN
+#define TX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN
+#define TX_BURST TX_VIRTQUEUE_SIZE
 
 typedef struct virtio_net_conf
 {
     uint32_t txtimer;
     int32_t txburst;
     char *tx;
+    uint16_t rx_virtqueue_sz;
+    uint16_t tx_virtqueue_sz;
 } virtio_net_conf;
 
 /* Maximum packet size we can receive from tap device: header + 64k */

Signature · 1 line

1.9.1

Michael S. Tsirkin

2016-03-15 09:44 UTC #6 lore raw

Quoted 18 lines · Greg Kurz

Quoted 15 lines · On Tue, 15 Mar 2016 06:50:38 +0200 "Michael S. Tsirkin" <[email protected]> wrote:
Quoted 3 lines · Patrik Hermansson
This patch adds the option to specify virtio queue sizes. Currently the queue sizes is hard coded to 256, which might not be suitable for all types of applications.
Any more data here? 256 packets seems plenty. Is there a chance your application does not support indirect descriptors? That's a better approach I think.
Quoted 5 lines
This patch makes it possible to specify the queue size between 256 to 1024.
The minimum value is chosen based on the current sizes of the virtio queues. The maximum queue size is based upon VIRTQUEUE_SIZE_MAX.

Quoted 7 lines

Quoted 3 lines
To me, this seems excessive. For example, 1024 will break if we have to add a virtio net header entry (on old kernels).
Hi Michael,
Can you elaborate on what/how will break ?

At least on linux, we add a header and then pass the scatter list to writev as is. Linux doesn't support io vectors > 1024 so if we allow 1024 entries for the guest, adding even a single entry will make the write fail.

Quoted 98 lines

Thanks.

--
Greg

Quoted 93 lines

Quoted 92 lines

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

hw/net/virtio-net.c +28 -3

index a877614..c4fcb39 100644
@@ -1326,20 +1326,40 @@ static void virtio_net_tx_bh(void *opaque)
     }
 }
 
+static void virtio_net_validate_queue_limit(uint16_t *queue_size)
+{
+    if (*queue_size > VIRTIO_QUEUE_SIZE_MAX) {
+        error_report("queue-size: %d, exceeds maximum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MAX,
+                      VIRTIO_QUEUE_SIZE_MAX);
+        *queue_size = VIRTIO_QUEUE_SIZE_MAX;
+    } else if (*queue_size < VIRTIO_QUEUE_SIZE_MIN) {
+        error_report("queue-size: %d, below minimum allowed queue-size(%d),"
+                     "queue-size set to %d", *queue_size, VIRTIO_QUEUE_SIZE_MIN,
+                      VIRTIO_QUEUE_SIZE_MIN);
+        *queue_size = VIRTIO_QUEUE_SIZE_MIN;
+    }
+}
+
 static void virtio_net_add_queue(VirtIONet *n, int index)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(n);
 
-    n->vqs[index].rx_vq = virtio_add_queue(vdev, 256, virtio_net_handle_rx);
+    virtio_net_validate_queue_limit(&n->net_conf.rx_virtqueue_sz);
+    virtio_net_validate_queue_limit(&n->net_conf.tx_virtqueue_sz);
+    n->vqs[index].rx_vq = virtio_add_queue(vdev, n->net_conf.rx_virtqueue_sz,
+                                           virtio_net_handle_rx);
     if (n->net_conf.tx && !strcmp(n->net_conf.tx, "timer")) {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_timer);
+            virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz,
+                             virtio_net_handle_tx_timer);
         n->vqs[index].tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                               virtio_net_tx_timer,
                                               &n->vqs[index]);
     } else {
         n->vqs[index].tx_vq =
-            virtio_add_queue(vdev, 256, virtio_net_handle_tx_bh);
+            virtio_add_queue(vdev, n->net_conf.tx_virtqueue_sz,
+                             virtio_net_handle_tx_bh);
         n->vqs[index].tx_bh = qemu_bh_new(virtio_net_tx_bh, &n->vqs[index]);
     }

@@ -1826,6 +1846,11 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer,
                        TX_TIMER_INTERVAL),
     DEFINE_PROP_INT32("x-txburst", VirtIONet, net_conf.txburst, TX_BURST),
+    DEFINE_PROP_UINT16("rx_virtqueue_sz", VirtIONet, net_conf.rx_virtqueue_sz,
+                       RX_VIRTQUEUE_SIZE),
+    DEFINE_PROP_UINT16("tx_virtqueue_sz", VirtIONet, net_conf.tx_virtqueue_sz,
+                       TX_VIRTQUEUE_SIZE),
+
     DEFINE_PROP_STRING("tx", VirtIONet, net_conf.tx),
     DEFINE_PROP_END_OF_LIST(),
 };

include/hw/virtio/virtio-net.h +7 -1

index f3cc25f..dbcabbf 100644
@@ -28,13 +28,19 @@
  * ensures fairness in the io path.  256 conveniently matches the
  * length of the TX queue and shows a good balance of performance
  * and latency. */
-#define TX_BURST 256
+#define VIRTIO_QUEUE_SIZE_MIN 256
+#define VIRTIO_QUEUE_SIZE_MAX VIRTQUEUE_MAX_SIZE
+#define RX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN
+#define TX_VIRTQUEUE_SIZE VIRTIO_QUEUE_SIZE_MIN
+#define TX_BURST TX_VIRTQUEUE_SIZE
 
 typedef struct virtio_net_conf
 {
     uint32_t txtimer;
     int32_t txburst;
     char *tx;
+    uint16_t rx_virtqueue_sz;
+    uint16_t tx_virtqueue_sz;
 } virtio_net_conf;
 
 /* Maximum packet size we can receive from tap device: header + 64k */

Signature · 1 line

1.9.1

Patrik Hermansson

author

2016-03-15 19:29 UTC #7 lore raw

In reply to Michael S. Tsirkin

Hi Michael

Thanks for looking into the patch and for your comments.

2016-03-15 5:50 GMT+01:00 Michael S. Tsirkin <[email protected]>:

Quoted 8 lines · Patrik Hermansson

Quoted 3 lines
This patch adds the option to specify virtio queue sizes. Currently the queue sizes is hard coded to 256, which might not be suitable for all types of applications.
Any more data here? 256 packets seems plenty. Is there a chance your application does not support indirect descriptors? That's a better approach I think.

My application within the VM is using DPDK(2.0) virtio PMD and even if it seems to have been attempts
getting support for indirect descriptors it does not seems it have reached mainline.
To be honest I hadn't heard about indirect descriptors in virtio until you mentioned it so
I still have some homework to do, but it seems to be a much more attractive solution
if such support exists in my case.

With 256 descriptors I see some problems with sporadic packet drops from time to time
which I believe have to do with qemu getting rescheduled, increasing the queue to 1024
seems to remedy the problem. My beliefs is that it is due the process getting some more
headroom until packets are getting dropped.

Quoted 10 lines

Quoted 6 lines
This patch makes it possible to specify the queue size between 256 to 1024.
The minimum value is chosen based on the current sizes of the virtio queues. The maximum queue size is based upon VIRTQUEUE_SIZE_MAX.
To me, this seems excessive. For example, 1024 will break if we have to add a virtio net header entry (on old kernels).

1024 were chosen due to assertion that the queue were not allowed to greater than VIRTQUEUE_SIZE_MAX. I can lower the upper limit and do some more measurements, but maybe it is the wrong path to go regardless?

Thanks,
Patrik