On 31.08.2016 14:48, John Ferlan wrote: > > > On 08/19/2016 07:54 AM, Michal Privoznik wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1366989 >> >> Qemu grew yet another virtio-net tunable [1]. It basically allows > > s/Qemu grew yet/QEMU added/ > >> users to set the size of RX virtio ring. But because virtio-net >> uses two separate ring buffers to pass data from/to guest they >> named it explicitly rx_queue_size. We should expose it in our XML >> too. >> >> 1: http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg02029.html > > Has this been merged yet? I see Jason Wang has it queue for net-next > (for 2.8), but I don't see the code in the mainline qemu I just updated to. No, I had to apply the patch locally and work over patched qemu. QEMU devels are currently in freeze for 2.7 release (not sure what their release date is planned on though). > >> >> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> >> --- >> docs/formatdomain.html.in | 7 +++++- >> docs/schemas/domaincommon.rng | 5 ++++ >> src/conf/domain_conf.c | 16 ++++++++++++ >> src/conf/domain_conf.h | 1 + >> src/qemu/qemu_domain.c | 7 ++++++ >> .../qemuxml2argv-net-virtio-rxqueuesize.xml | 29 ++++++++++++++++++++++ >> 6 files changed, 64 insertions(+), 1 deletion(-) >> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml >> >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >> index bfbb0f2..6642dc8 100644 >> --- a/docs/formatdomain.html.in >> +++ b/docs/formatdomain.html.in >> @@ -4604,7 +4604,7 @@ qemu-kvm -net nic,model=? /dev/null >> <source network='default'/> >> <target dev='vnet1'/> >> <model type='virtio'/> >> - <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5'> >> + <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rxqueuesize='128'> >> <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off' mrg_rxbuf='off'/> >> <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/> >> </driver> >> @@ -4720,6 +4720,11 @@ qemu-kvm -net nic,model=? /dev/null >> <span class="since">virtio-net since 1.0.6 (QEMU and KVM only)</span> >> <span class="since">vhost-user since 1.2.17 (QEMU and KVM only)</span> >> </dd> >> + <dt><code>rxqueuesize</code></dt> >> + <dd> >> + The optional <code>rxqueuesize</code> attribute controls >> + the size of virtio ring for each queue as described above. > > Need a <span class="since"> (I assume something similar to queues with > the QEMU and KVM only condition) > > Also why not "rx_queue_size" - there's already event_idx or mrg_rxbuf, > so adding the "_" at least mimics qemu's argument. That's what I wanted to prevent. Copying name from qemu. But I've struggled with the naming too (as can be seen - look how far I got :D). I don't have a strong opinion on either of those, so whatever we feel like I'll stick with that. > > Furthermore, the bz has quite a bit of discussion regarding an > "appropriate value", so while I don't think it's something we want to > provide (that is suggested values), perhaps we could create a pointer or > at least a few hints. At the very least a this value needs to be a power > of 2 value... If not provided, the QEMU default of 256 is used. A > larger size gives you what advantage. In general some guidance on > setting or where to look could be helpful. Well, as you point out later in the review too, qemu might change these limitation anytime and we would advice untrue. But if I'm careful with wording we might be safe. > >> + </dd> >> </dl> >> <p> >> Offloading options for the host and guest can be configured using >> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng >> index 052f28c..4b89a86 100644 >> --- a/docs/schemas/domaincommon.rng >> +++ b/docs/schemas/domaincommon.rng >> @@ -2529,6 +2529,11 @@ >> </attribute> >> </optional> >> <optional> >> + <attribute name='rxqueuesize'> >> + <ref name='positiveInteger'/> >> + </attribute> >> + </optional> >> + <optional> >> <attribute name="txmode"> >> <choice> >> <value>iothread</value> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index 14d4f7d..08cde19 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -8911,6 +8911,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, >> char *ioeventfd = NULL; >> char *event_idx = NULL; >> char *queues = NULL; >> + char *rxqueuesize = NULL; >> char *str = NULL; >> char *filter = NULL; >> char *internal = NULL; >> @@ -9083,6 +9084,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, >> ioeventfd = virXMLPropString(cur, "ioeventfd"); >> event_idx = virXMLPropString(cur, "event_idx"); >> queues = virXMLPropString(cur, "queues"); >> + rxqueuesize = virXMLPropString(cur, "rxqueuesize"); >> } else if (xmlStrEqual(cur->name, BAD_CAST "filterref")) { >> if (filter) { >> virReportError(VIR_ERR_XML_ERROR, "%s", >> @@ -9466,6 +9468,17 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, >> if (q > 1) >> def->driver.virtio.queues = q; >> } >> + if (rxqueuesize) { >> + unsigned int q; >> + if (virStrToLong_uip(rxqueuesize, NULL, 10, &q) < 0) { >> + virReportError(VIR_ERR_XML_DETAIL, >> + _("'rxqueuesize' attribute must be positive number: %s"), >> + rxqueuesize); >> + goto error; >> + } >> + if (q > 1) >> + def->driver.virtio.rxqueuesize = q; >> + } >> if ((str = virXPathString("string(./driver/host/@csum)", ctxt))) { >> if ((val = virTristateSwitchTypeFromString(str)) <= 0) { >> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> @@ -9646,6 +9659,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, >> VIR_FREE(ioeventfd); >> VIR_FREE(event_idx); >> VIR_FREE(queues); >> + VIR_FREE(rxqueuesize); >> VIR_FREE(str); >> VIR_FREE(filter); >> VIR_FREE(type); >> @@ -20768,6 +20782,8 @@ virDomainVirtioNetDriverFormat(char **outstr, >> } >> if (def->driver.virtio.queues) >> virBufferAsprintf(&buf, "queues='%u' ", def->driver.virtio.queues); >> + if (def->driver.virtio.rxqueuesize) >> + virBufferAsprintf(&buf, "rxqueuesize='%u' ", def->driver.virtio.rxqueuesize); >> >> virBufferTrim(&buf, " ", -1); >> >> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h >> index 8b26724..9197d70 100644 >> --- a/src/conf/domain_conf.h >> +++ b/src/conf/domain_conf.h >> @@ -906,6 +906,7 @@ struct _virDomainNetDef { >> virTristateSwitch ioeventfd; >> virTristateSwitch event_idx; >> unsigned int queues; /* Multiqueue virtio-net */ >> + unsigned int rxqueuesize; /* rx_queue_size */ >> struct { >> virTristateSwitch csum; >> virTristateSwitch gso; >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index 6c0f261..ee24086 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -2372,6 +2372,13 @@ qemuDomainDeviceDefValidate(const virDomainDeviceDef *dev, >> "not supported by QEMU")); >> goto cleanup; >> } >> + >> + if (STREQ_NULLABLE(net->model, "virtio") && >> + net->driver.virtio.rxqueuesize & (net->driver.virtio.rxqueuesize -1)) { > > - 1 ? > > e.g. extra space (although I do see the line is at 80 chars... could > change the internal name to rxqsz ;-)). > >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("rxqueuesize has to be a power of two")); > > ^^ hence why I think it should be documented. > > The bz also indicates there's a maximum of 1024. Should we check for > that? Although if the maximum increases we'd have to follow suit. > Naturally there isn't a way to get that maximum. If something larger > than 1024 is passed, then qemu will fail. Ugh, the hazards of adding > these 1-off things that have limits and rules, but we're not given all > the details necessary. Nope. Moreover, this feature is going to be used by expert users who know exactly what they are doing. So I'd say we're okay with just trying to start qemu and see if it fails or not. IOW too much work on our side not worth it. > >> + goto cleanup; >> + } >> } >> >> ret = 0; >> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml >> new file mode 100644 >> index 0000000..e23d3e3 >> --- /dev/null >> +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-rxqueuesize.xml >> @@ -0,0 +1,29 @@ >> +<domain type='qemu'> >> + <name>QEMUGuest1</name> >> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> >> + <memory unit='KiB'>219100</memory> >> + <currentMemory unit='KiB'>219100</currentMemory> >> + <vcpu placement='static'>1</vcpu> >> + <os> >> + <type arch='i686' machine='pc'>hvm</type> >> + <boot dev='hd'/> >> + </os> >> + <clock offset='utc'/> >> + <on_poweroff>destroy</on_poweroff> >> + <on_reboot>restart</on_reboot> >> + <on_crash>destroy</on_crash> >> + <devices> >> + <emulator>/usr/bin/qemu</emulator> >> + <disk type='block' device='disk'> >> + <source dev='/dev/HostVG/QEMUGuest1'/> >> + <target dev='hda' bus='ide'/> >> + </disk> >> + <controller type='usb' index='0'/> >> + <interface type='user'> >> + <mac address='00:11:22:33:44:55'/> >> + <model type='virtio'/> >> + <driver rxqueuesize='128'/> >> + </interface> >> + <memballoon model='virtio'/> >> + </devices> >> +</domain> >> > > Shouldn't qemuxml2xmltest.c be updated to add this new test? Good catch. > > Should there be a "FAIL" test with an invalid value? Sure, I can introduce such test. > > I think by rule we have to wait for QEMU to merge this so we cannot push > until then. In the meantime, we should probably find out if it's felt we > should check a maximum of 1024 or not. I'd hate to see that value change > and then we have problems, but the way it is now - qemu would fail with > a 2048 value from libvirt. I don't think we need to. We should probably check whether it is a power of two (this looks like invariant requirement) = who would want a ring size of a non-power of two size? But the maximum/minimum size can change. Currently the limits are [256;1024]. Moreover, qemu fails with sensible message: error: internal error: process exited while connecting to monitor: 2016-08-31T14:23:16.583247Z qemu-system-x86_64: -device virtio-net-pci,mq=on,vectors=42,rx_queue_size=2048,netdev=hostnet1,id=net1,mac=52:54:00:6e:0e:72,bus=pci.2,addr=0x2: Invalid rx_queue_size (= 2048), must be a power of 2 between 256 and 1024. I'll post v2 which will is rebased onto current HEAD and address your findings. Thank you! Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list