https://bugzilla.redhat.com/show_bug.cgi?id=1462653

Just like I've added support for setting rx_queue_size (in
c56cdf259 and friends), qemu just gained support for setting tx
ring size.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---

diff to v1:
- Resend after 2e7d4916967 which introduced yet another qemu capability and
  thus made my patch not apply cleanly.

 docs/formatdomain.html.in                                | 16 +++++++++++++++-
 docs/schemas/domaincommon.rng                            |  5 +++++
 src/conf/domain_conf.c                                   | 16 ++++++++++++++++
 src/conf/domain_conf.h                                   |  1 +
 src/qemu/qemu_capabilities.c                             |  4 ++++
 src/qemu/qemu_capabilities.h                             |  3 +++
 src/qemu/qemu_command.c                                  |  8 ++++++++
 src/qemu/qemu_domain.c                                   | 16 +++++++++++-----
 ...e.args => qemuxml2argv-net-virtio-rxtxqueuesize.args} |  4 ++--
 ...ize.xml => qemuxml2argv-net-virtio-rxtxqueuesize.xml} |  2 +-
 tests/qemuxml2argvtest.c                                 |  5 +++--
 ...e.xml => qemuxml2xmlout-net-virtio-rxtxqueuesize.xml} |  2 +-
 tests/qemuxml2xmltest.c                                  |  2 +-
 13 files changed, 71 insertions(+), 13 deletions(-)
 rename tests/qemuxml2argvdata/{qemuxml2argv-net-virtio-rxqueuesize.args => qemuxml2argv-net-virtio-rxtxqueuesize.args} (85%)
 rename tests/qemuxml2argvdata/{qemuxml2argv-net-virtio-rxqueuesize.xml => qemuxml2argv-net-virtio-rxtxqueuesize.xml} (93%)
 rename tests/qemuxml2xmloutdata/{qemuxml2xmlout-net-virtio-rxqueuesize.xml => qemuxml2xmlout-net-virtio-rxtxqueuesize.xml} (96%)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index c12efcf78..58662cf48 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -5071,7 +5071,7 @@ qemu-kvm -net nic,model=? /dev/null
     &lt;source network='default'/&gt;
     &lt;target dev='vnet1'/&gt;
     &lt;model type='virtio'/&gt;
-    <b>&lt;driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rx_queue_size='256'&gt;
+    <b>&lt;driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rx_queue_size='256' tx_queue_size='256'&gt;
       &lt;host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off' mrg_rxbuf='off'/&gt;
       &lt;guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/&gt;
     &lt;/driver&gt;
@@ -5201,6 +5201,20 @@ qemu-kvm -net nic,model=? /dev/null
         <b>In general you should leave this option alone, unless you
         are very certain you know what you are doing.</b>
       </dd>
+      <dt><code>tx_queue_size</code></dt>
+      <dd>
+        The optional <code>tx_queue_size</code> attribute controls
+        the size of virtio ring for each queue as described above.
+        The default value is hypervisor dependent and may change
+        across its releases. Moreover, some hypervisors may pose
+        some restrictions on actual value. For instance, latest
+        QEMU (as of 2017-07-13) requires value to be a power of two
+        from [256, 1024] range.
+        <span class="since">Since 3.6.0 (QEMU and KVM only)</span><br/><br/>
+
+        <b>In general you should leave this option alone, unless you
+        are very certain you know what you are doing.</b>
+      </dd>
       <dt>virtio options</dt>
       <dd>
         For virtio interfaces,
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index fc1a40f96..6558a1b2d 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2709,6 +2709,11 @@
                   <ref name='positiveInteger'/>
                 </attribute>
               </optional>
+              <optional>
+                <attribute name='tx_queue_size'>
+                  <ref name='positiveInteger'/>
+                </attribute>
+              </optional>
               <optional>
                 <attribute name="txmode">
                   <choice>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3feeccb9f..c525936cb 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9920,6 +9920,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
     char *event_idx = NULL;
     char *queues = NULL;
     char *rx_queue_size = NULL;
+    char *tx_queue_size = NULL;
     char *str = NULL;
     char *filter = NULL;
     char *internal = NULL;
@@ -10093,6 +10094,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
                 event_idx = virXMLPropString(cur, "event_idx");
                 queues = virXMLPropString(cur, "queues");
                 rx_queue_size = virXMLPropString(cur, "rx_queue_size");
+                tx_queue_size = virXMLPropString(cur, "tx_queue_size");
             } else if (xmlStrEqual(cur->name, BAD_CAST "filterref")) {
                 if (filter) {
                     virReportError(VIR_ERR_XML_ERROR, "%s",
@@ -10490,6 +10492,16 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
             }
             def->driver.virtio.rx_queue_size = q;
         }
+        if (tx_queue_size) {
+            unsigned int q;
+            if (virStrToLong_uip(tx_queue_size, NULL, 10, &q) < 0) {
+                virReportError(VIR_ERR_XML_DETAIL,
+                               _("'tx_queue_size' attribute must be positive number: %s"),
+                               tx_queue_size);
+                goto error;
+            }
+            def->driver.virtio.tx_queue_size = q;
+        }
         if ((str = virXPathString("string(./driver/host/@csum)", ctxt))) {
             if ((val = virTristateSwitchTypeFromString(str)) <= 0) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -10687,6 +10699,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
     VIR_FREE(event_idx);
     VIR_FREE(queues);
     VIR_FREE(rx_queue_size);
+    VIR_FREE(tx_queue_size);
     VIR_FREE(str);
     VIR_FREE(filter);
     VIR_FREE(type);
@@ -22558,6 +22571,9 @@ virDomainVirtioNetDriverFormat(char **outstr,
     if (def->driver.virtio.rx_queue_size)
         virBufferAsprintf(&buf, " rx_queue_size='%u'",
                           def->driver.virtio.rx_queue_size);
+    if (def->driver.virtio.tx_queue_size)
+        virBufferAsprintf(&buf, " tx_queue_size='%u'",
+                          def->driver.virtio.tx_queue_size);
 
     virDomainVirtioOptionsFormat(&buf, def->virtio);
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index af15ee8b5..e3c736dc7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -969,6 +969,7 @@ struct _virDomainNetDef {
             virTristateSwitch event_idx;
             unsigned int queues; /* Multiqueue virtio-net */
             unsigned int rx_queue_size;
+            unsigned int tx_queue_size;
             struct {
                 virTristateSwitch csum;
                 virTristateSwitch gso;
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 04aa8d53c..5a060ae98 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -431,6 +431,9 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
               "virtio.ats",
               "loadparm",
               "spapr-pci-host-bridge",
+
+              /* 265 */
+              "virtio-net.tx_queue_size",
     );
 
 
@@ -1697,6 +1700,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioNet[] = {
     { "tx", QEMU_CAPS_VIRTIO_TX_ALG },
     { "event_idx", QEMU_CAPS_VIRTIO_NET_EVENT_IDX },
     { "rx_queue_size", QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE },
+    { "tx_queue_size", QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE },
     { "host_mtu", QEMU_CAPS_VIRTIO_NET_HOST_MTU },
 };
 
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 8250b57b4..6e094e92e 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -418,6 +418,9 @@ typedef enum {
     QEMU_CAPS_LOADPARM, /* -machine loadparm */
     QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, /* -device spapr-pci-host-bridge */
 
+    /* 265 */
+    QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE, /* virtio-net-*.tx_queue_size */
+
     QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 6ac26af3e..8ff6d2176 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -3733,6 +3733,14 @@ qemuBuildNicDevStr(virDomainDefPtr def,
         }
         virBufferAsprintf(&buf, ",rx_queue_size=%u", net->driver.virtio.rx_queue_size);
     }
+    if (usingVirtio && net->driver.virtio.tx_queue_size) {
+        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE)) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("virtio tx_queue_size option is not supported with this QEMU binary"));
+            goto error;
+        }
+        virBufferAsprintf(&buf, ",tx_queue_size=%u", net->driver.virtio.tx_queue_size);
+    }
 
     if (usingVirtio && net->mtu) {
         if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_HOST_MTU)) {
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 464d3a1f9..50095b7df 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3169,11 +3169,17 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev,
             goto cleanup;
         }
 
-        if (STREQ_NULLABLE(net->model, "virtio") &&
-            net->driver.virtio.rx_queue_size & (net->driver.virtio.rx_queue_size - 1)) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("rx_queue_size has to be a power of two"));
-            goto cleanup;
+        if (STREQ_NULLABLE(net->model, "virtio")) {
+            if (net->driver.virtio.rx_queue_size & (net->driver.virtio.rx_queue_size - 1)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("rx_queue_size has to be a power of two"));
+                goto cleanup;
+            }
+            if (net->driver.virtio.tx_queue_size & (net->driver.virtio.tx_queue_size - 1)) {
+                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                               _("tx_queue_size has to be a power of two"));
+                goto cleanup;
+            }
         }
 
         if (net->mtu &&
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.args b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxtxqueuesize.args
similarity index 85%
rename from tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.args
rename to tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxtxqueuesize.args
index 07c358a02..c78da3d17 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxtxqueuesize.args
@@ -21,7 +21,7 @@ server,nowait \
 -usb \
 -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
 -device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
--device virtio-net-pci,rx_queue_size=512,vlan=0,id=net0,mac=00:11:22:33:44:55,\
-bus=pci.0,addr=0x3 \
+-device virtio-net-pci,rx_queue_size=512,tx_queue_size=1024,vlan=0,id=net0,\
+mac=00:11:22:33:44:55,bus=pci.0,addr=0x3 \
 -net user,vlan=0,name=hostnet0 \
 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxtxqueuesize.xml
similarity index 93%
rename from tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml
rename to tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxtxqueuesize.xml
index d64e31df2..b51931d52 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml
+++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxtxqueuesize.xml
@@ -22,7 +22,7 @@
     <interface type='user'>
       <mac address='00:11:22:33:44:55'/>
       <model type='virtio'/>
-      <driver rx_queue_size='512'/>
+      <driver rx_queue_size='512' tx_queue_size='1024'/>
     </interface>
     <memballoon model='virtio'/>
   </devices>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index b95ea46be..e496ac468 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1160,8 +1160,9 @@ mymain(void)
             QEMU_CAPS_VIRTIO_S390);
     DO_TEST("net-virtio-ccw",
             QEMU_CAPS_VIRTIO_CCW, QEMU_CAPS_VIRTIO_S390);
-    DO_TEST("net-virtio-rxqueuesize",
-            QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE);
+    DO_TEST("net-virtio-rxtxqueuesize",
+            QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE,
+            QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE);
     DO_TEST_PARSE_ERROR("net-virtio-rxqueuesize-invalid-size", NONE);
     DO_TEST("net-eth", NONE);
     DO_TEST("net-eth-ifname", NONE);
diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-virtio-rxqueuesize.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-virtio-rxtxqueuesize.xml
similarity index 96%
rename from tests/qemuxml2xmloutdata/qemuxml2xmlout-net-virtio-rxqueuesize.xml
rename to tests/qemuxml2xmloutdata/qemuxml2xmlout-net-virtio-rxtxqueuesize.xml
index 78433026c..5c33a58ad 100644
--- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-virtio-rxqueuesize.xml
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-virtio-rxtxqueuesize.xml
@@ -29,7 +29,7 @@
     <interface type='user'>
       <mac address='00:11:22:33:44:55'/>
       <model type='virtio'/>
-      <driver rx_queue_size='512'/>
+      <driver rx_queue_size='512' tx_queue_size='1024'/>
       <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
     </interface>
     <input type='mouse' bus='ps2'/>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 5e4b1d17f..724c8efd0 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -537,7 +537,7 @@ mymain(void)
     DO_TEST("net-eth-ifname", NONE);
     DO_TEST("net-eth-hostip", NONE);
     DO_TEST("net-virtio-network-portgroup", NONE);
-    DO_TEST("net-virtio-rxqueuesize", NONE);
+    DO_TEST("net-virtio-rxtxqueuesize", NONE);
     DO_TEST("net-hostdev", NONE);
     DO_TEST("net-hostdev-vfio", NONE);
     DO_TEST("net-midonet", NONE);
-- 
2.13.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Jul 17, 2017 at 15:39:56 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1462653

This bugzilla is not public.

> 
> Just like I've added support for setting rx_queue_size (in
> c56cdf259 and friends), qemu just gained support for setting tx
> ring size.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
> 
> diff to v1:
> - Resend after 2e7d4916967 which introduced yet another qemu capability and
>   thus made my patch not apply cleanly.
> 
>  docs/formatdomain.html.in                                | 16 +++++++++++++++-
>  docs/schemas/domaincommon.rng                            |  5 +++++
>  src/conf/domain_conf.c                                   | 16 ++++++++++++++++
>  src/conf/domain_conf.h                                   |  1 +
>  src/qemu/qemu_capabilities.c                             |  4 ++++
>  src/qemu/qemu_capabilities.h                             |  3 +++
>  src/qemu/qemu_command.c                                  |  8 ++++++++
>  src/qemu/qemu_domain.c                                   | 16 +++++++++++-----
>  ...e.args => qemuxml2argv-net-virtio-rxtxqueuesize.args} |  4 ++--
>  ...ize.xml => qemuxml2argv-net-virtio-rxtxqueuesize.xml} |  2 +-
>  tests/qemuxml2argvtest.c                                 |  5 +++--
>  ...e.xml => qemuxml2xmlout-net-virtio-rxtxqueuesize.xml} |  2 +-
>  tests/qemuxml2xmltest.c                                  |  2 +-
>  13 files changed, 71 insertions(+), 13 deletions(-)
>  rename tests/qemuxml2argvdata/{qemuxml2argv-net-virtio-rxqueuesize.args => qemuxml2argv-net-virtio-rxtxqueuesize.args} (85%)
>  rename tests/qemuxml2argvdata/{qemuxml2argv-net-virtio-rxqueuesize.xml => qemuxml2argv-net-virtio-rxtxqueuesize.xml} (93%)
>  rename tests/qemuxml2xmloutdata/{qemuxml2xmlout-net-virtio-rxqueuesize.xml => qemuxml2xmlout-net-virtio-rxtxqueuesize.xml} (96%)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index c12efcf78..58662cf48 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in

[...]

> @@ -5201,6 +5201,20 @@ qemu-kvm -net nic,model=? /dev/null
>          <b>In general you should leave this option alone, unless you
>          are very certain you know what you are doing.</b>
>        </dd>
> +      <dt><code>tx_queue_size</code></dt>
> +      <dd>
> +        The optional <code>tx_queue_size</code> attribute controls
> +        the size of virtio ring for each queue as described above.
> +        The default value is hypervisor dependent and may change
> +        across its releases. Moreover, some hypervisors may pose
> +        some restrictions on actual value. For instance, latest
> +        QEMU (as of 2017-07-13) requires value to be a power of two
> +        from [256, 1024] range.
> +        <span class="since">Since 3.6.0 (QEMU and KVM only)</span><br/><br/>

This is ridiculous. Since we can't figure out how to set this, how are
users supposed to figure this out?

Is it really needed? How should it be configured? Can't we or qemu pick
a sane value?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 07/18/2017 08:23 AM, Peter Krempa wrote:
> On Mon, Jul 17, 2017 at 15:39:56 +0200, Michal Privoznik wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1462653
> 
> This bugzilla is not public.

Okay, I'll drop it from the commit message.

> 
>>
>> Just like I've added support for setting rx_queue_size (in
>> c56cdf259 and friends), qemu just gained support for setting tx
>> ring size.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>
>> diff to v1:
>> - Resend after 2e7d4916967 which introduced yet another qemu capability and
>>   thus made my patch not apply cleanly.
>>
>>  docs/formatdomain.html.in                                | 16 +++++++++++++++-
>>  docs/schemas/domaincommon.rng                            |  5 +++++
>>  src/conf/domain_conf.c                                   | 16 ++++++++++++++++
>>  src/conf/domain_conf.h                                   |  1 +
>>  src/qemu/qemu_capabilities.c                             |  4 ++++
>>  src/qemu/qemu_capabilities.h                             |  3 +++
>>  src/qemu/qemu_command.c                                  |  8 ++++++++
>>  src/qemu/qemu_domain.c                                   | 16 +++++++++++-----
>>  ...e.args => qemuxml2argv-net-virtio-rxtxqueuesize.args} |  4 ++--
>>  ...ize.xml => qemuxml2argv-net-virtio-rxtxqueuesize.xml} |  2 +-
>>  tests/qemuxml2argvtest.c                                 |  5 +++--
>>  ...e.xml => qemuxml2xmlout-net-virtio-rxtxqueuesize.xml} |  2 +-
>>  tests/qemuxml2xmltest.c                                  |  2 +-
>>  13 files changed, 71 insertions(+), 13 deletions(-)
>>  rename tests/qemuxml2argvdata/{qemuxml2argv-net-virtio-rxqueuesize.args => qemuxml2argv-net-virtio-rxtxqueuesize.args} (85%)
>>  rename tests/qemuxml2argvdata/{qemuxml2argv-net-virtio-rxqueuesize.xml => qemuxml2argv-net-virtio-rxtxqueuesize.xml} (93%)
>>  rename tests/qemuxml2xmloutdata/{qemuxml2xmlout-net-virtio-rxqueuesize.xml => qemuxml2xmlout-net-virtio-rxtxqueuesize.xml} (96%)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index c12efcf78..58662cf48 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
> 
> [...]
> 
>> @@ -5201,6 +5201,20 @@ qemu-kvm -net nic,model=? /dev/null
>>          <b>In general you should leave this option alone, unless you
>>          are very certain you know what you are doing.</b>
>>        </dd>
>> +      <dt><code>tx_queue_size</code></dt>
>> +      <dd>
>> +        The optional <code>tx_queue_size</code> attribute controls
>> +        the size of virtio ring for each queue as described above.
>> +        The default value is hypervisor dependent and may change
>> +        across its releases. Moreover, some hypervisors may pose
>> +        some restrictions on actual value. For instance, latest
>> +        QEMU (as of 2017-07-13) requires value to be a power of two
>> +        from [256, 1024] range.
>> +        <span class="since">Since 3.6.0 (QEMU and KVM only)</span><br/><br/>
> 
> This is ridiculous. Since we can't figure out how to set this, how are
> users supposed to figure this out?

Well, you've cut off the line that reads;

  <b>In general you should leave this option alone, unless you
  are very certain you know what you are doing.</b>

So only users that know how virtio works under the hood are expected to
also know what rx/tx queue size is and how to set it. But frankly, I
think users setting this are always gonna go with the highest value
avaliable (1024). Such detailed description is a copy of rx_virtio_queue
size description which is result of review.

> 
> Is it really needed? How should it be configured? Can't we or qemu pick
> a sane value?
> 

No. Some users need bigger virtio rings otherwise they see a packet
drop. So this is a fine tuning that heavily depends on the use case.
Thus libvirt should not try to come up with some value.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Jul 18, 2017 at 09:01:31 +0200, Michal Privoznik wrote:
> On 07/18/2017 08:23 AM, Peter Krempa wrote:
> > On Mon, Jul 17, 2017 at 15:39:56 +0200, Michal Privoznik wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1462653
> > 
> > This bugzilla is not public.
> 
> Okay, I'll drop it from the commit message.

Also add proper explanation what the benefits are, since upstream can't
read the motivation from the bugzilla.

> >> Just like I've added support for setting rx_queue_size (in
> >> c56cdf259 and friends), qemu just gained support for setting tx
> >> ring size.

[...]

> >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> >> index c12efcf78..58662cf48 100644
> >> --- a/docs/formatdomain.html.in
> >> +++ b/docs/formatdomain.html.in
> > 
> > [...]
> > 
> >> @@ -5201,6 +5201,20 @@ qemu-kvm -net nic,model=? /dev/null
> >>          <b>In general you should leave this option alone, unless you
> >>          are very certain you know what you are doing.</b>
> >>        </dd>
> >> +      <dt><code>tx_queue_size</code></dt>
> >> +      <dd>
> >> +        The optional <code>tx_queue_size</code> attribute controls
> >> +        the size of virtio ring for each queue as described above.
> >> +        The default value is hypervisor dependent and may change
> >> +        across its releases. Moreover, some hypervisors may pose
> >> +        some restrictions on actual value. For instance, latest
> >> +        QEMU (as of 2017-07-13) requires value to be a power of two

Refer to a proper version of qemu when this is supported, not a date.

> >> +        from [256, 1024] range.
> >> +        <span class="since">Since 3.6.0 (QEMU and KVM only)</span><br/><br/>
> > 
> > This is ridiculous. Since we can't figure out how to set this, how are
> > users supposed to figure this out?
> 
> Well, you've cut off the line that reads;
> 
>   <b>In general you should leave this option alone, unless you
>   are very certain you know what you are doing.</b>
> 
> So only users that know how virtio works under the hood are expected to
> also know what rx/tx queue size is and how to set it. But frankly, I

This statement is ridiculous by itself.

> think users setting this are always gonna go with the highest value
> avaliable (1024). Such detailed description is a copy of rx_virtio_queue
> size description which is result of review.
> 
> > 
> > Is it really needed? How should it be configured? Can't we or qemu pick
> > a sane value?
> > 
> 
> No. Some users need bigger virtio rings otherwise they see a packet
> drop. So this is a fine tuning that heavily depends on the use case.
> Thus libvirt should not try to come up with some value.

That's why I think it's wrong.  What's the drawback of setting it to
maximum? Which workloads will hit it? Why is the default not sufficient?

And most notably, how do the users figure out that they need it?

In this case, there are no anchor points that users can use to figure
out if they need a setting like this. In addition putting in a warning
that a setting should not be touched makes it rather useless.

Is there any writeup that we could point users to which would explain
this feature?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[Adding MST who wrote qemu patches]

On 07/18/2017 12:21 PM, Peter Krempa wrote:
> On Tue, Jul 18, 2017 at 09:01:31 +0200, Michal Privoznik wrote:
>> On 07/18/2017 08:23 AM, Peter Krempa wrote:
>>> On Mon, Jul 17, 2017 at 15:39:56 +0200, Michal Privoznik wrote:
>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1462653
>>>
>>> This bugzilla is not public.
>>
>> Okay, I'll drop it from the commit message.
> 
> Also add proper explanation what the benefits are, since upstream can't
> read the motivation from the bugzilla.
> 
>>>> Just like I've added support for setting rx_queue_size (in
>>>> c56cdf259 and friends), qemu just gained support for setting tx
>>>> ring size.
> 
> [...]
> 
>>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>>> index c12efcf78..58662cf48 100644
>>>> --- a/docs/formatdomain.html.in
>>>> +++ b/docs/formatdomain.html.in
>>>
>>> [...]
>>>
>>>> @@ -5201,6 +5201,20 @@ qemu-kvm -net nic,model=? /dev/null
>>>>          <b>In general you should leave this option alone, unless you
>>>>          are very certain you know what you are doing.</b>
>>>>        </dd>
>>>> +      <dt><code>tx_queue_size</code></dt>
>>>> +      <dd>
>>>> +        The optional <code>tx_queue_size</code> attribute controls
>>>> +        the size of virtio ring for each queue as described above.
>>>> +        The default value is hypervisor dependent and may change
>>>> +        across its releases. Moreover, some hypervisors may pose
>>>> +        some restrictions on actual value. For instance, latest
>>>> +        QEMU (as of 2017-07-13) requires value to be a power of two
> 
> Refer to a proper version of qemu when this is supported, not a date.
> 
>>>> +        from [256, 1024] range.
>>>> +        <span class="since">Since 3.6.0 (QEMU and KVM only)</span><br/><br/>
>>>
>>> This is ridiculous. Since we can't figure out how to set this, how are
>>> users supposed to figure this out?
>>
>> Well, you've cut off the line that reads;
>>
>>   <b>In general you should leave this option alone, unless you
>>   are very certain you know what you are doing.</b>
>>
>> So only users that know how virtio works under the hood are expected to
>> also know what rx/tx queue size is and how to set it. But frankly, I
> 
> This statement is ridiculous by itself.

Why? There are more experienced users (for whom libvirt's just a stable
API) and less experienced ones (for whom libvirt's and tools on the top
of it are great for their automatic chose of parameters, e.g. gnome boxes).

> 
>> think users setting this are always gonna go with the highest value
>> avaliable (1024). Such detailed description is a copy of rx_virtio_queue
>> size description which is result of review.
>>
>>>
>>> Is it really needed? How should it be configured? Can't we or qemu pick
>>> a sane value?
>>>
>>
>> No. Some users need bigger virtio rings otherwise they see a packet
>> drop. So this is a fine tuning that heavily depends on the use case.
>> Thus libvirt should not try to come up with some value.
> 
> That's why I think it's wrong.  What's the drawback of setting it to
> maximum? Which workloads will hit it? Why is the default not sufficient?
> 
> And most notably, how do the users figure out that they need it?

I'll leave this for MST to answer.

> 
> In this case, there are no anchor points that users can use to figure
> out if they need a setting like this. In addition putting in a warning
> that a setting should not be touched makes it rather useless.

Well, it can be viewed as counter part to rx_queue_size which we already
have.

> 
> Is there any writeup that we could point users to which would explain
> this feature?
> 

I'm afraid there's nothing else than BZ I've linked and qemu patches.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 07/18/2017 07:12 AM, Michal Privoznik wrote:
> [Adding MST who wrote qemu patches]
>
> On 07/18/2017 12:21 PM, Peter Krempa wrote:
>> On Tue, Jul 18, 2017 at 09:01:31 +0200, Michal Privoznik wrote:
>>> On 07/18/2017 08:23 AM, Peter Krempa wrote:
>>>> On Mon, Jul 17, 2017 at 15:39:56 +0200, Michal Privoznik wrote:
>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1462653
>>>> This bugzilla is not public.
>>> Okay, I'll drop it from the commit message.
>> Also add proper explanation what the benefits are, since upstream can't
>> read the motivation from the bugzilla.
>>
>>>>> Just like I've added support for setting rx_queue_size (in
>>>>> c56cdf259 and friends), qemu just gained support for setting tx
>>>>> ring size.
>> [...]
>>
>>>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>>>> index c12efcf78..58662cf48 100644
>>>>> --- a/docs/formatdomain.html.in
>>>>> +++ b/docs/formatdomain.html.in
>>>> [...]
>>>>
>>>>> @@ -5201,6 +5201,20 @@ qemu-kvm -net nic,model=? /dev/null
>>>>>          <b>In general you should leave this option alone, unless you
>>>>>          are very certain you know what you are doing.</b>
>>>>>        </dd>
>>>>> +      <dt><code>tx_queue_size</code></dt>
>>>>> +      <dd>
>>>>> +        The optional <code>tx_queue_size</code> attribute controls
>>>>> +        the size of virtio ring for each queue as described above.
>>>>> +        The default value is hypervisor dependent and may change
>>>>> +        across its releases. Moreover, some hypervisors may pose
>>>>> +        some restrictions on actual value. For instance, latest
>>>>> +        QEMU (as of 2017-07-13) requires value to be a power of two
>> Refer to a proper version of qemu when this is supported, not a date.
>>
>>>>> +        from [256, 1024] range.
>>>>> +        <span class="since">Since 3.6.0 (QEMU and KVM only)</span><br/><br/>
>>>> This is ridiculous. Since we can't figure out how to set this, how are
>>>> users supposed to figure this out?
>>> Well, you've cut off the line that reads;
>>>
>>>   <b>In general you should leave this option alone, unless you
>>>   are very certain you know what you are doing.</b>
>>>
>>> So only users that know how virtio works under the hood are expected to
>>> also know what rx/tx queue size is and how to set it. But frankly, I
>> This statement is ridiculous by itself.
> Why? There are more experienced users (for whom libvirt's just a stable
> API) and less experienced ones (for whom libvirt's and tools on the top
> of it are great for their automatic chose of parameters, e.g. gnome boxes).

Beyond that, that same statement (or something nearly identical) is
repeated in multiple places throughout the XML documentation. There are
at least two classes of these attributes that I can think of:

1) attributes that are automatically set to a sane value by libvirt when
not specified (and they usually *aren't* specified), and saved in the
config XML in order to assure they are set the same every time the
domain is started (to preserve guest ABI). These are intended to record
whatever was the default setting for the attribute at the time the
domain was created. For example, a pcie-root-port controller might have
<model name='ioh3420'/> set, if that was the only type of pcie-root-port
available at the time a domain was created; this comes in handy now that
there is a generic pcie-root-port (which also has <model
name='pcie-root-port'/>) - existing domains don't get their ABI screwed
up when they're migrated to a newer host.

2) knobs that have been added in over the years at the request/demand
from below (qemu) and above (ovirt / openstack), many of them obscure,
difficult to explain with any amount of useful detail, and almost never
worthy of being manually set (and if you "don't know what you're doing",
you're just as likely to make things worse as to make them better).

tx_queue_size is one of the latter.

For either of these types of attributes, they need to be documented so
that someone encountering them (or actively searching them out) will at
least have a basic idea what the attribute is named / used for, but we
also need to warn the casual user to not mess with them. I don't see
anything ridiculous about that.

>
>>> think users setting this are always gonna go with the highest value
>>> avaliable (1024). Such detailed description is a copy of rx_virtio_queue
>>> size description which is result of review.
>>>
>>>> Is it really needed? How should it be configured? Can't we or qemu pick
>>>> a sane value?
>>>>
>>> No. Some users need bigger virtio rings otherwise they see a packet
>>> drop. So this is a fine tuning that heavily depends on the use case.
>>> Thus libvirt should not try to come up with some value.
>> That's why I think it's wrong.  What's the drawback of setting it to
>> maximum? Which workloads will hit it? Why is the default not sufficient?
>>
>> And most notably, how do the users figure out that they need it?


I think it's a bit too much burden on libvirt to expect that much detail
to be included in formatdomain.html. Heck, I don't know if anyone even
*knows* that much detail right now.


> I'll leave this for MST to answer.
>
>> In this case, there are no anchor points that users can use to figure
>> out if they need a setting like this. In addition putting in a warning
>> that a setting should not be touched makes it rather useless.

I disagree. The setting is documented so that people can know that it
exists. Anyone *really* interested in performance can look up
information about it directly from the source and/or run their own
performance tests. My understanding is that's what's done with settings
like this, not just in libvirt and qemu, but in the Linux kernel too -
people like Red Hat's performance testing groups will run a bunch of
benchmark tests with different loads, comparing the test results with
different settings of the tuning attributes, and publish white papers
with recommendations for settings based on what loads a customer will be
running on their systems (over time, these performance tests may
discover that one particular setting is the best in *all* conditions,
and in that case either qemu or libvirt can begin setting that as the
default). But it's impractical to expect the person adding the knob to
perform such performance testing and document the proper setting for the
knob before it's even been pushed upstream. Instead, we add the knobs
and let the perf testing teams know about them, then they run their
barrage of tests and tell everyone what (if anything) to do with those
knobs.


> Well, it can be viewed as counter part to rx_queue_size which we already
> have.
>
>> Is there any writeup that we could point users to which would explain
>> this feature?
>>
> I'm afraid there's nothing else than BZ I've linked and qemu patches.
>

I think the documentation Peter is looking for will come later, from
someone else.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Jul 20, 2017 at 21:36:28 -0400, Laine Stump wrote:
> On 07/18/2017 07:12 AM, Michal Privoznik wrote:

[...]

> >>>>> @@ -5201,6 +5201,20 @@ qemu-kvm -net nic,model=? /dev/null
> >>>>>          <b>In general you should leave this option alone, unless you
> >>>>>          are very certain you know what you are doing.</b>
> >>>>>        </dd>
> >>>>> +      <dt><code>tx_queue_size</code></dt>
> >>>>> +      <dd>
> >>>>> +        The optional <code>tx_queue_size</code> attribute controls
> >>>>> +        the size of virtio ring for each queue as described above.
> >>>>> +        The default value is hypervisor dependent and may change
> >>>>> +        across its releases. Moreover, some hypervisors may pose
> >>>>> +        some restrictions on actual value. For instance, latest
> >>>>> +        QEMU (as of 2017-07-13) requires value to be a power of two
> >> Refer to a proper version of qemu when this is supported, not a date.
> >>
> >>>>> +        from [256, 1024] range.
> >>>>> +        <span class="since">Since 3.6.0 (QEMU and KVM only)</span><br/><br/>
> >>>> This is ridiculous. Since we can't figure out how to set this, how are
> >>>> users supposed to figure this out?
> >>> Well, you've cut off the line that reads;
> >>>
> >>>   <b>In general you should leave this option alone, unless you
> >>>   are very certain you know what you are doing.</b>
> >>>
> >>> So only users that know how virtio works under the hood are expected to
> >>> also know what rx/tx queue size is and how to set it. But frankly, I
> >> This statement is ridiculous by itself.
> > Why? There are more experienced users (for whom libvirt's just a stable
> > API) and less experienced ones (for whom libvirt's and tools on the top
> > of it are great for their automatic chose of parameters, e.g. gnome boxes).
> 
> Beyond that, that same statement (or something nearly identical) is
> repeated in multiple places throughout the XML documentation. There are
> at least two classes of these attributes that I can think of:
> 
> 1) attributes that are automatically set to a sane value by libvirt when
> not specified (and they usually *aren't* specified), and saved in the
> config XML in order to assure they are set the same every time the
> domain is started (to preserve guest ABI). These are intended to record
> whatever was the default setting for the attribute at the time the
> domain was created. For example, a pcie-root-port controller might have
> <model name='ioh3420'/> set, if that was the only type of pcie-root-port
> available at the time a domain was created; this comes in handy now that
> there is a generic pcie-root-port (which also has <model
> name='pcie-root-port'/>) - existing domains don't get their ABI screwed
> up when they're migrated to a newer host.
> 
> 2) knobs that have been added in over the years at the request/demand
> from below (qemu) and above (ovirt / openstack), many of them obscure,
> difficult to explain with any amount of useful detail, and almost never
> worthy of being manually set (and if you "don't know what you're doing",
> you're just as likely to make things worse as to make them better).
> 
> tx_queue_size is one of the latter.
> 
> For either of these types of attributes, they need to be documented so
> that someone encountering them (or actively searching them out) will at
> least have a basic idea what the attribute is named / used for, but we
> also need to warn the casual user to not mess with them. I don't see
> anything ridiculous about that.

[1]

> >>> think users setting this are always gonna go with the highest value
> >>> avaliable (1024). Such detailed description is a copy of rx_virtio_queue
> >>> size description which is result of review.
> >>>
> >>>> Is it really needed? How should it be configured? Can't we or qemu pick
> >>>> a sane value?
> >>>>
> >>> No. Some users need bigger virtio rings otherwise they see a packet
> >>> drop. So this is a fine tuning that heavily depends on the use case.
> >>> Thus libvirt should not try to come up with some value.
> >> That's why I think it's wrong.  What's the drawback of setting it to
> >> maximum? Which workloads will hit it? Why is the default not sufficient?
> >>
> >> And most notably, how do the users figure out that they need it?
> 
> 
> I think it's a bit too much burden on libvirt to expect that much detail
> to be included in formatdomain.html. Heck, I don't know if anyone even
> *knows* that much detail right now.

That proves my point kind of. If there isn't knowledge how to set up
this, then why even add the option. Is there really a point?

> > I'll leave this for MST to answer.
> >
> >> In this case, there are no anchor points that users can use to figure
> >> out if they need a setting like this. In addition putting in a warning
> >> that a setting should not be touched makes it rather useless.
> 
> I disagree. The setting is documented so that people can know that it
> exists. Anyone *really* interested in performance can look up
> information about it directly from the source and/or run their own
> performance tests. My understanding is that's what's done with settings
> like this, not just in libvirt and qemu, but in the Linux kernel too -
> people like Red Hat's performance testing groups will run a bunch of
> benchmark tests with different loads, comparing the test results with
> different settings of the tuning attributes, and publish white papers
> with recommendations for settings based on what loads a customer will be
> running on their systems (over time, these performance tests may
> discover that one particular setting is the best in *all* conditions,
> and in that case either qemu or libvirt can begin setting that as the
> default). But it's impractical to expect the person adding the knob to
> perform such performance testing and document the proper setting for the
> knob before it's even been pushed upstream. Instead, we add the knobs
> and let the perf testing teams know about them, then they run their
> barrage of tests and tell everyone what (if anything) to do with those
> knobs.

I'd expect that there is a very clearly defined benefit when adding such
thing. Adding a knob not doing anything is hardly worth the time.

> > Well, it can be viewed as counter part to rx_queue_size which we already
> > have.
> >
> >> Is there any writeup that we could point users to which would explain
> >> this feature?
> >>
> > I'm afraid there's nothing else than BZ I've linked and qemu patches.
> >
> 
> I think the documentation Peter is looking for will come later, from
> someone else.


The problem is that we are using the 'prior art' argument too often
without thinking whether something even makes sense. And then we have to
support it.

When adding a feature, there should be some clear benefit from it. When
fixing a bug it also should have some clear statement why it's
happening.

In this case we have a knob, which is not really clear how to set it
which may add performance benefits (that's the non-clear feature part)
and it may fix packets being lost (that's the bug part, but in default
configuration it's not fixed, nor clear when it occurs).

This is the ridiculous part [1].

I'm not going to object to adding this any more. I just wanted some
justification for adding this and I did not get a clear one.

Please at least add proper version information as requested in my first
reply when reposting.

No-longer-complained-against-by: Peter Krempa <pkrempa@redhat.com>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list