[libvirt] [PATCH] qemu: Expose rx/tx_queue_size in qemu.conf too
In 2074ef6cd4a2 and c56cdf259 (and friends) we've added two attributes to virtio NICs: rx_queue_size and tx_queue_size. However, sysadmins might want to set these on per-host basis but don't necessarily have an access to domain XML (e.g. because they are generated by some other app). So let's expose them under qemu.conf (the settings from domain XML still take precedence as they are more specific ones). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 12 +++++++++-- src/qemu/libvirtd_qemu.aug | 4 ++++ src/qemu/qemu.conf | 7 +++++++ src/qemu/qemu_command.c | 42 ++++++++++++++++++++++++++++++-------- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 3 +++ src/qemu/qemu_hotplug.c | 2 +- src/qemu/test_libvirtd_qemu.aug.in | 2 ++ 9 files changed, 66 insertions(+), 13 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d272cc1ba..c0107ab4b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5373,7 +5373,11 @@ qemu-kvm -net nic,model=? /dev/null some restrictions on actual value. For instance, latest QEMU (as of 2016-09-01) requires value to be a power of two from [256, 1024] range. - <span class="since">Since 2.3.0 (QEMU and KVM only)</span><br/><br/> + <span class="since">Since 2.3.0 (QEMU and KVM only)</span> + Then, <span class="since">Since 4.1.0</span> the default value can be + set in <code>qemu.conf</code> file and thus overrides hypervisor + default. + <br/><br/> <b>In general you should leave this option alone, unless you are very certain you know what you are doing.</b> @@ -5389,7 +5393,11 @@ qemu-kvm -net nic,model=? /dev/null range. In addition to that, this may work only for a subset of interface types, e.g. aforementioned QEMU enables this option only for <code>vhostuser</code> type. - <span class="since">Since 3.7.0 (QEMU and KVM only)</span><br/><br/> + <span class="since">Since 3.7.0 (QEMU and KVM only)</span> + Then, <span class="since">Since 4.1.0</span> the default value can be + set in <code>qemu.conf</code> file and thus overrides hypervisor + default. + <br/><br/> <b>In general you should leave this option alone, unless you are very certain you know what you are doing.</b> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index c19bf3a43..084290296 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -118,6 +118,9 @@ module Libvirtd_qemu = let vxhs_entry = bool_entry "vxhs_tls" | str_entry "vxhs_tls_x509_cert_dir" + let virtio_entry = int_entry "rx_queue_size" + | int_entry "tx_queue_size" + (* Each entry in the config is one of the following ... *) let entry = default_tls_entry | vnc_entry @@ -137,6 +140,7 @@ module Libvirtd_qemu = | gluster_debug_level_entry | memory_entry | vxhs_entry + | virtio_entry let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 43dd561cc..a945ebdd5 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -775,3 +775,10 @@ # This directory is used for memoryBacking source if configured as file. # NOTE: big files will be stored here #memory_backing_dir = "/var/lib/libvirt/qemu/ram" + +# The following two values set the default RX/TX ring buffer size for virtio +# interfaces. These values are taken unless overridden in domain XML. Please +# note that QEMU accepts 256, 512 and 1024 only. These values correspond to +# those from domain XML. +#rx_queue_size = 1024 +#tx_queue_size = 1024 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b8aede32d..771c12445 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3693,7 +3693,8 @@ qemuBuildNicStr(virDomainNetDefPtr net, char * -qemuBuildNicDevStr(virDomainDefPtr def, +qemuBuildNicDevStr(virQEMUDriverConfigPtr cfg, + virDomainDefPtr def, virDomainNetDefPtr net, int vlan, unsigned int bootindex, @@ -3813,21 +3814,40 @@ qemuBuildNicDevStr(virDomainDefPtr def, virBufferAsprintf(&buf, ",mq=on,vectors=%zu", 2 * vhostfdSize + 2); } } - if (usingVirtio && net->driver.virtio.rx_queue_size) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) { + if (usingVirtio) { + unsigned int rx_queue_size = net->driver.virtio.rx_queue_size; + + if (rx_queue_size == 0 && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) + rx_queue_size = cfg->rx_queue_size; + + + if (rx_queue_size && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) { + net->driver.virtio.rx_queue_size = rx_queue_size; + virBufferAsprintf(&buf, ",rx_queue_size=%u", rx_queue_size); + } else if (rx_queue_size) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("virtio rx_queue_size option is not supported with this QEMU binary")); goto error; } - 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)) { + if (usingVirtio) { + unsigned int tx_queue_size = net->driver.virtio.tx_queue_size; + + if (tx_queue_size == 0 && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE)) + tx_queue_size = cfg->tx_queue_size; + + if (tx_queue_size && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE)) { + net->driver.virtio.tx_queue_size = tx_queue_size; + virBufferAsprintf(&buf, ",tx_queue_size=%u", tx_queue_size); + } else if (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) { @@ -8489,7 +8509,7 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, virCommandAddArg(cmd, netdev); VIR_FREE(netdev); - if (!(nic = qemuBuildNicDevStr(def, net, -1, bootindex, + if (!(nic = qemuBuildNicDevStr(cfg, def, net, -1, bootindex, queues, qemuCaps))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error generating NIC -device string")); @@ -8526,6 +8546,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, int **nicindexes, bool chardevStdioLogd) { + virQEMUDriverConfigPtr cfg; int ret = -1; char *nic = NULL, *host = NULL; int *tapfd = NULL; @@ -8587,6 +8608,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, return -1; } + cfg = virQEMUDriverGetConfig(driver); + switch (actualType) { case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_BRIDGE: @@ -8782,7 +8805,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, virCommandAddArgList(cmd, "-netdev", host, NULL); } if (qemuDomainSupportsNicdev(def, net)) { - if (!(nic = qemuBuildNicDevStr(def, net, vlan, bootindex, + if (!(nic = qemuBuildNicDevStr(cfg, def, net, vlan, bootindex, vhostfdSize, qemuCaps))) goto cleanup; virCommandAddArgList(cmd, "-device", nic, NULL); @@ -8826,6 +8849,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, VIR_FREE(host); VIR_FREE(tapfdName); VIR_FREE(vhostfdName); + virObjectUnref(cfg); return ret; } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index bdde6f918..85f7bd1b4 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -90,7 +90,8 @@ char *qemuBuildNicStr(virDomainNetDefPtr net, int vlan); /* Current, best practice */ -char *qemuBuildNicDevStr(virDomainDefPtr def, +char *qemuBuildNicDevStr(virQEMUDriverConfigPtr cfg, + virDomainDefPtr def, virDomainNetDefPtr net, int vlan, unsigned int bootindex, diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index af503d31c..2fa96431f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -912,6 +912,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueString(conf, "memory_backing_dir", &cfg->memoryBackingDir) < 0) goto cleanup; + if (virConfGetValueUInt(conf, "rx_queue_size", &cfg->rx_queue_size) < 0 || + virConfGetValueUInt(conf, "tx_queue_size", &cfg->tx_queue_size) < 0) + goto cleanup; + ret = 0; cleanup: diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index a553e30e2..3f38a76c2 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -206,6 +206,9 @@ struct _virQEMUDriverConfig { bool vxhsTLS; char *vxhsTLSx509certdir; + + unsigned int rx_queue_size; + unsigned int tx_queue_size; }; /* Main driver state */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6b245bd6a..18b460704 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1116,7 +1116,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, for (i = 0; i < vhostfdSize; i++) VIR_FORCE_CLOSE(vhostfd[i]); - if (!(nicstr = qemuBuildNicDevStr(vm->def, net, vlan, 0, + if (!(nicstr = qemuBuildNicDevStr(cfg, vm->def, net, vlan, 0, queueSize, priv->qemuCaps))) goto try_remove; diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 688e5b9fd..4fc4e2f4e 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -100,3 +100,5 @@ module Test_libvirtd_qemu = { "1" = "mount" } } { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" } +{ "rx_queue_size" = "1024" } +{ "tx_queue_size" = "1024" } -- 2.13.6
On 01/19/2018 07:50 AM, Michal Privoznik wrote:
In 2074ef6cd4a2 and c56cdf259 (and friends) we've added two attributes to virtio NICs: rx_queue_size and tx_queue_size. However, sysadmins might want to set these on per-host basis but don't necessarily have an access to domain XML (e.g. because they are generated by some other app). So let's expose them under qemu.conf (the settings from domain XML still take precedence as they are more specific ones).
This wording says to me domain XML takes precedence; however,... [1]
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 12 +++++++++-- src/qemu/libvirtd_qemu.aug | 4 ++++ src/qemu/qemu.conf | 7 +++++++ src/qemu/qemu_command.c | 42 ++++++++++++++++++++++++++++++-------- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 3 +++ src/qemu/qemu_hotplug.c | 2 +- src/qemu/test_libvirtd_qemu.aug.in | 2 ++ 9 files changed, 66 insertions(+), 13 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d272cc1ba..c0107ab4b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5373,7 +5373,11 @@ qemu-kvm -net nic,model=? /dev/null some restrictions on actual value. For instance, latest QEMU (as of 2016-09-01) requires value to be a power of two from [256, 1024] range. - <span class="since">Since 2.3.0 (QEMU and KVM only)</span><br/><br/> + <span class="since">Since 2.3.0 (QEMU and KVM only)</span> + Then, <span class="since">Since 4.1.0</span> the default value can be + set in <code>qemu.conf</code> file and thus overrides hypervisor + default.
[1] ...this and...
+ <br/><br/>
<b>In general you should leave this option alone, unless you are very certain you know what you are doing.</b> @@ -5389,7 +5393,11 @@ qemu-kvm -net nic,model=? /dev/null range. In addition to that, this may work only for a subset of interface types, e.g. aforementioned QEMU enables this option only for <code>vhostuser</code> type. - <span class="since">Since 3.7.0 (QEMU and KVM only)</span><br/><br/> + <span class="since">Since 3.7.0 (QEMU and KVM only)</span> + Then, <span class="since">Since 4.1.0</span> the default value can be + set in <code>qemu.conf</code> file and thus overrides hypervisor + default. + <br/><br/>
[1] ... this seems to imply the conf value overrides XML (or as written hypervisor default)... Then the code does something even more interesting... [2] BTW: If you look at the generated output you see "Then, Since 4.1.0...." Personally, "Then," is probably not the best transition "word"... Also it's not clear that "and thus overrides hypervisor default." is actually what's being done in the code. My suggestion: "Additionally, since 4.1.0 the value can be set in the qemu.conf file in order to override the domain XML setting."
<b>In general you should leave this option alone, unless you are very certain you know what you are doing.</b> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index c19bf3a43..084290296 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -118,6 +118,9 @@ module Libvirtd_qemu = let vxhs_entry = bool_entry "vxhs_tls" | str_entry "vxhs_tls_x509_cert_dir"
+ let virtio_entry = int_entry "rx_queue_size" + | int_entry "tx_queue_size" + (* Each entry in the config is one of the following ... *) let entry = default_tls_entry | vnc_entry @@ -137,6 +140,7 @@ module Libvirtd_qemu = | gluster_debug_level_entry | memory_entry | vxhs_entry + | virtio_entry
let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 43dd561cc..a945ebdd5 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -775,3 +775,10 @@ # This directory is used for memoryBacking source if configured as file. # NOTE: big files will be stored here #memory_backing_dir = "/var/lib/libvirt/qemu/ram" + +# The following two values set the default RX/TX ring buffer size for virtio +# interfaces. These values are taken unless overridden in domain XML. Please +# note that QEMU accepts 256, 512 and 1024 only. These values correspond to +# those from domain XML.
[1]... and this says, XML overrides values - which to a degree is what the code does. Interesting to note the QEMU valid values... I think we'd be better off indicating that valid values are described in formatdomain.html, but a hyperlink to the docs isn't something that's already in the qemu.conf file and I'm not sure/clear if it's "good" or "valid" to put it there. In the long run, I think it'd be nice to have one place to describe when the feature is supported and what the valid values are so that if/when they change in the future it's only one place to change. Someone could set this, but if their QEMU isn't new enough, then domains will fail to start and it may not be obvious why.
+#rx_queue_size = 1024 +#tx_queue_size = 1024 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b8aede32d..771c12445 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3693,7 +3693,8 @@ qemuBuildNicStr(virDomainNetDefPtr net,
char * -qemuBuildNicDevStr(virDomainDefPtr def, +qemuBuildNicDevStr(virQEMUDriverConfigPtr cfg, + virDomainDefPtr def, virDomainNetDefPtr net, int vlan, unsigned int bootindex, @@ -3813,21 +3814,40 @@ qemuBuildNicDevStr(virDomainDefPtr def, virBufferAsprintf(&buf, ",mq=on,vectors=%zu", 2 * vhostfdSize + 2); } } - if (usingVirtio && net->driver.virtio.rx_queue_size) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) { + if (usingVirtio) { + unsigned int rx_queue_size = net->driver.virtio.rx_queue_size; + + if (rx_queue_size == 0 && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) + rx_queue_size = cfg->rx_queue_size; + + + if (rx_queue_size && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) { + net->driver.virtio.rx_queue_size = rx_queue_size;
[2] ...if someone sets the qemu.conf variable it overwrites the domain value? Is that really something we want to do? Is this is only the running XML or is it the config XML?
+ virBufferAsprintf(&buf, ",rx_queue_size=%u", rx_queue_size); + } else if (rx_queue_size) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("virtio rx_queue_size option is not supported with this QEMU binary")); goto error; } - 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)) { + if (usingVirtio) { + unsigned int tx_queue_size = net->driver.virtio.tx_queue_size; + + if (tx_queue_size == 0 && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE)) + tx_queue_size = cfg->tx_queue_size; + + if (tx_queue_size && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE)) { + net->driver.virtio.tx_queue_size = tx_queue_size; + virBufferAsprintf(&buf, ",tx_queue_size=%u", tx_queue_size); + } else if (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) { @@ -8489,7 +8509,7 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, virCommandAddArg(cmd, netdev); VIR_FREE(netdev);
- if (!(nic = qemuBuildNicDevStr(def, net, -1, bootindex, + if (!(nic = qemuBuildNicDevStr(cfg, def, net, -1, bootindex, queues, qemuCaps))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error generating NIC -device string")); @@ -8526,6 +8546,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, int **nicindexes, bool chardevStdioLogd) { + virQEMUDriverConfigPtr cfg;
Today this is fine, worried about some future adjustment which goes to cleanup before cfg is set... So if we set it to NULL we avoid that adjustment causing some unforeseen issue.
int ret = -1; char *nic = NULL, *host = NULL; int *tapfd = NULL; @@ -8587,6 +8608,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, return -1; }
+ cfg = virQEMUDriverGetConfig(driver); + switch (actualType) { case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_BRIDGE: @@ -8782,7 +8805,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, virCommandAddArgList(cmd, "-netdev", host, NULL); } if (qemuDomainSupportsNicdev(def, net)) { - if (!(nic = qemuBuildNicDevStr(def, net, vlan, bootindex, + if (!(nic = qemuBuildNicDevStr(cfg, def, net, vlan, bootindex, vhostfdSize, qemuCaps))) goto cleanup; virCommandAddArgList(cmd, "-device", nic, NULL); @@ -8826,6 +8849,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, VIR_FREE(host); VIR_FREE(tapfdName); VIR_FREE(vhostfdName); + virObjectUnref(cfg); return ret; }
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index bdde6f918..85f7bd1b4 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -90,7 +90,8 @@ char *qemuBuildNicStr(virDomainNetDefPtr net, int vlan);
/* Current, best practice */ -char *qemuBuildNicDevStr(virDomainDefPtr def, +char *qemuBuildNicDevStr(virQEMUDriverConfigPtr cfg, + virDomainDefPtr def, virDomainNetDefPtr net, int vlan, unsigned int bootindex, diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index af503d31c..2fa96431f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -912,6 +912,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueString(conf, "memory_backing_dir", &cfg->memoryBackingDir) < 0) goto cleanup;
+ if (virConfGetValueUInt(conf, "rx_queue_size", &cfg->rx_queue_size) < 0 || + virConfGetValueUInt(conf, "tx_queue_size", &cfg->tx_queue_size) < 0) + goto cleanup; +
Once the domain capabilities are read, can/should we then check if the rx/tx values are set and cause a failure at startup time? Probably reduces some complexity in qemuBuildNicDevStr too. John
ret = 0;
cleanup: diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index a553e30e2..3f38a76c2 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -206,6 +206,9 @@ struct _virQEMUDriverConfig {
bool vxhsTLS; char *vxhsTLSx509certdir; + + unsigned int rx_queue_size; + unsigned int tx_queue_size; };
/* Main driver state */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 6b245bd6a..18b460704 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1116,7 +1116,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, for (i = 0; i < vhostfdSize; i++) VIR_FORCE_CLOSE(vhostfd[i]);
- if (!(nicstr = qemuBuildNicDevStr(vm->def, net, vlan, 0, + if (!(nicstr = qemuBuildNicDevStr(cfg, vm->def, net, vlan, 0, queueSize, priv->qemuCaps))) goto try_remove;
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 688e5b9fd..4fc4e2f4e 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -100,3 +100,5 @@ module Test_libvirtd_qemu = { "1" = "mount" } } { "memory_backing_dir" = "/var/lib/libvirt/qemu/ram" } +{ "rx_queue_size" = "1024" } +{ "tx_queue_size" = "1024" }
On 01/31/2018 03:28 PM, John Ferlan wrote:
On 01/19/2018 07:50 AM, Michal Privoznik wrote:
In 2074ef6cd4a2 and c56cdf259 (and friends) we've added two attributes to virtio NICs: rx_queue_size and tx_queue_size. However, sysadmins might want to set these on per-host basis but don't necessarily have an access to domain XML (e.g. because they are generated by some other app). So let's expose them under qemu.conf (the settings from domain XML still take precedence as they are more specific ones).
This wording says to me domain XML takes precedence; however,... [1]
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 12 +++++++++-- src/qemu/libvirtd_qemu.aug | 4 ++++ src/qemu/qemu.conf | 7 +++++++ src/qemu/qemu_command.c | 42 ++++++++++++++++++++++++++++++-------- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 3 +++ src/qemu/qemu_hotplug.c | 2 +- src/qemu/test_libvirtd_qemu.aug.in | 2 ++ 9 files changed, 66 insertions(+), 13 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index d272cc1ba..c0107ab4b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5373,7 +5373,11 @@ qemu-kvm -net nic,model=? /dev/null some restrictions on actual value. For instance, latest QEMU (as of 2016-09-01) requires value to be a power of two from [256, 1024] range. - <span class="since">Since 2.3.0 (QEMU and KVM only)</span><br/><br/> + <span class="since">Since 2.3.0 (QEMU and KVM only)</span> + Then, <span class="since">Since 4.1.0</span> the default value can be + set in <code>qemu.conf</code> file and thus overrides hypervisor + default.
[1] ...this and...
+ <br/><br/>
<b>In general you should leave this option alone, unless you are very certain you know what you are doing.</b> @@ -5389,7 +5393,11 @@ qemu-kvm -net nic,model=? /dev/null range. In addition to that, this may work only for a subset of interface types, e.g. aforementioned QEMU enables this option only for <code>vhostuser</code> type. - <span class="since">Since 3.7.0 (QEMU and KVM only)</span><br/><br/> + <span class="since">Since 3.7.0 (QEMU and KVM only)</span> + Then, <span class="since">Since 4.1.0</span> the default value can be + set in <code>qemu.conf</code> file and thus overrides hypervisor + default. + <br/><br/>
[1] ... this seems to imply the conf value overrides XML (or as written hypervisor default)...
No. hypervisor default has the least precedence. It's like this: hv default < qemu.conf < domain XML.
Then the code does something even more interesting... [2]
BTW: If you look at the generated output you see "Then, Since 4.1.0...."
Personally, "Then," is probably not the best transition "word"... Also it's not clear that "and thus overrides hypervisor default." is actually what's being done in the code.
My suggestion:
"Additionally, since 4.1.0 the value can be set in the qemu.conf file in order to override the domain XML setting."
Okay, I'm no good with documentation.
<b>In general you should leave this option alone, unless you are very certain you know what you are doing.</b> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index c19bf3a43..084290296 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -118,6 +118,9 @@ module Libvirtd_qemu = let vxhs_entry = bool_entry "vxhs_tls" | str_entry "vxhs_tls_x509_cert_dir"
+ let virtio_entry = int_entry "rx_queue_size" + | int_entry "tx_queue_size" + (* Each entry in the config is one of the following ... *) let entry = default_tls_entry | vnc_entry @@ -137,6 +140,7 @@ module Libvirtd_qemu = | gluster_debug_level_entry | memory_entry | vxhs_entry + | virtio_entry
let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 43dd561cc..a945ebdd5 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -775,3 +775,10 @@ # This directory is used for memoryBacking source if configured as file. # NOTE: big files will be stored here #memory_backing_dir = "/var/lib/libvirt/qemu/ram" + +# The following two values set the default RX/TX ring buffer size for virtio +# interfaces. These values are taken unless overridden in domain XML. Please +# note that QEMU accepts 256, 512 and 1024 only. These values correspond to +# those from domain XML.
[1]... and this says, XML overrides values - which to a degree is what the code does.
Interesting to note the QEMU valid values... I think we'd be better off indicating that valid values are described in formatdomain.html, but a hyperlink to the docs isn't something that's already in the qemu.conf file and I'm not sure/clear if it's "good" or "valid" to put it there.
Sure. I can drop the sentence mentioning values completely. The next one refers to the docs anyway.
In the long run, I think it'd be nice to have one place to describe when the feature is supported and what the valid values are so that if/when they change in the future it's only one place to change.
Someone could set this, but if their QEMU isn't new enough, then domains will fail to start and it may not be obvious why.
No, if qemu doesn't support the feature these settings don't get applied. Starting fails only if the values are requested in domain XML, not qemu.conf.
+#rx_queue_size = 1024 +#tx_queue_size = 1024 diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b8aede32d..771c12445 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3693,7 +3693,8 @@ qemuBuildNicStr(virDomainNetDefPtr net,
char * -qemuBuildNicDevStr(virDomainDefPtr def, +qemuBuildNicDevStr(virQEMUDriverConfigPtr cfg, + virDomainDefPtr def, virDomainNetDefPtr net, int vlan, unsigned int bootindex, @@ -3813,21 +3814,40 @@ qemuBuildNicDevStr(virDomainDefPtr def, virBufferAsprintf(&buf, ",mq=on,vectors=%zu", 2 * vhostfdSize + 2); } } - if (usingVirtio && net->driver.virtio.rx_queue_size) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) { + if (usingVirtio) { + unsigned int rx_queue_size = net->driver.virtio.rx_queue_size; + + if (rx_queue_size == 0 && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) + rx_queue_size = cfg->rx_queue_size; + + + if (rx_queue_size && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE)) { + net->driver.virtio.rx_queue_size = rx_queue_size;
[2] ...if someone sets the qemu.conf variable it overwrites the domain value? Is that really something we want to do? Is this is only the running XML or is it the config XML?
It is just live XML and we need to do this to keep ABI stability during migration. Also, it's nice to see what values were applied, therefore you can see them in 'dumpxml' for a live domain. It's not unusual that we do this, for instance: qemuDomainPrepareChardevSourceTLS().
+ virBufferAsprintf(&buf, ",rx_queue_size=%u", rx_queue_size); + } else if (rx_queue_size) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("virtio rx_queue_size option is not supported with this QEMU binary")); goto error; } - 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)) { + if (usingVirtio) { + unsigned int tx_queue_size = net->driver.virtio.tx_queue_size; + + if (tx_queue_size == 0 && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE)) + tx_queue_size = cfg->tx_queue_size; + + if (tx_queue_size && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE)) { + net->driver.virtio.tx_queue_size = tx_queue_size; + virBufferAsprintf(&buf, ",tx_queue_size=%u", tx_queue_size); + } else if (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) { @@ -8489,7 +8509,7 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, virCommandAddArg(cmd, netdev); VIR_FREE(netdev);
- if (!(nic = qemuBuildNicDevStr(def, net, -1, bootindex, + if (!(nic = qemuBuildNicDevStr(cfg, def, net, -1, bootindex, queues, qemuCaps))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error generating NIC -device string")); @@ -8526,6 +8546,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, int **nicindexes, bool chardevStdioLogd) { + virQEMUDriverConfigPtr cfg;
Today this is fine, worried about some future adjustment which goes to cleanup before cfg is set... So if we set it to NULL we avoid that adjustment causing some unforeseen issue.
Okay, consider done.
int ret = -1; char *nic = NULL, *host = NULL; int *tapfd = NULL; @@ -8587,6 +8608,8 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, return -1; }
+ cfg = virQEMUDriverGetConfig(driver); + switch (actualType) { case VIR_DOMAIN_NET_TYPE_NETWORK: case VIR_DOMAIN_NET_TYPE_BRIDGE: @@ -8782,7 +8805,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, virCommandAddArgList(cmd, "-netdev", host, NULL); } if (qemuDomainSupportsNicdev(def, net)) { - if (!(nic = qemuBuildNicDevStr(def, net, vlan, bootindex, + if (!(nic = qemuBuildNicDevStr(cfg, def, net, vlan, bootindex, vhostfdSize, qemuCaps))) goto cleanup; virCommandAddArgList(cmd, "-device", nic, NULL); @@ -8826,6 +8849,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, VIR_FREE(host); VIR_FREE(tapfdName); VIR_FREE(vhostfdName); + virObjectUnref(cfg); return ret; }
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index bdde6f918..85f7bd1b4 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -90,7 +90,8 @@ char *qemuBuildNicStr(virDomainNetDefPtr net, int vlan);
/* Current, best practice */ -char *qemuBuildNicDevStr(virDomainDefPtr def, +char *qemuBuildNicDevStr(virQEMUDriverConfigPtr cfg, + virDomainDefPtr def, virDomainNetDefPtr net, int vlan, unsigned int bootindex, diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index af503d31c..2fa96431f 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -912,6 +912,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueString(conf, "memory_backing_dir", &cfg->memoryBackingDir) < 0) goto cleanup;
+ if (virConfGetValueUInt(conf, "rx_queue_size", &cfg->rx_queue_size) < 0 || + virConfGetValueUInt(conf, "tx_queue_size", &cfg->tx_queue_size) < 0) + goto cleanup; +
Once the domain capabilities are read, can/should we then check if the rx/tx values are set and cause a failure at startup time? Probably reduces some complexity in qemuBuildNicDevStr too.
Well, I don't know. I mean, currently this patch is written in fault-tolerant fashion - if qemu doesn't support these two settings, domain can start just fine. If it does support them, they are applied. On one hand, this is very user friendly. On the other it doesn't follow how we treat other settings. For instance, the aforementioned chardev_tls - if qemu doesn't support it starting a domain fails. So maybe after all I need to change this behaviour I've implemented. Either don't enable it in the config or install newer qemu. I'll send v2 shortly. Michal
participants (2)
-
John Ferlan -
Michal Privoznik