Re: [libvirt] [PATCH 3/3] qemu: add busy polling support

On 06/15/2017 10:17 AM, sferdjao(a)redhat.com wrote: > From: Sahid Orentino Ferdjaoui <sahid.ferdjaoui(a)redhat.com&gt; > > This patch finalizes support of 'poll_us' attribute as an option of > the NIC driver-specific element, the commit also takes care of > hotplug. > > <devices> > <interface type='ethernet'> > <mac address='52:54:00:23:bc:ba'/> > <model type='virtio'/> > <driver name="vhost" poll_us='50'/> > </interface> > </devices> > > That option is supported for all networks which are using a tap > device. > > To be used the backend should be specificly set to use vhost. Actually, libvirt's qemu driver will use vhost-net automatically as long as the qemu binary supports it (unless 1) the domain is using TCG instead of KVM, or 2) the interface configuration specifically says "<driver name='qemu" .../>). > > Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui(a)redhat.com&gt; > --- > docs/formatdomain.html.in | 12 ++++++++++- > docs/schemas/domaincommon.rng | 5 +++++ > src/qemu/qemu_command.c | 22 +++++++++++++++++++++ > src/qemu/qemu_hotplug.c | 12 +++++++++++ > .../qemuxml2argv-net-virtio-netdev-pollus-fail.xml | 23 ++++++++++++++++++++++ > ...xml2argv-net-virtio-netdev-pollus-qemu-fail.xml | 23 ++++++++++++++++++++++ > .../qemuxml2argv-net-virtio-netdev-pollus.args | 23 ++++++++++++++++++++++ > .../qemuxml2argv-net-virtio-netdev-pollus.xml | 23 ++++++++++++++++++++++ > tests/qemuxml2argvtest.c | 9 +++++++++ The successful tests should all be done in qemuxml2xmltest.c as well > 9 files changed, 151 insertions(+), 1 deletion(-) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus-fail.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus-qemu-fail.xml > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus.args > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus.xml > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 52119f0..7bfeabc 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -5043,7 +5043,7 @@ qemu-kvm -net nic,model=? /dev/null > &lt;model type='virtio'/&gt; > <b>&lt;driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rx_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;guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off' poll_us='50'/&gt; > &lt;/driver&gt; > </b> > &lt;/interface&gt; > @@ -5171,6 +5171,16 @@ 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> > + <dd> > + The optional <code>poll_us</code> attribute configure the > + maximum number of microseconds that could be spent on busy > + polling. It improves the performance, but requires more > + CPU. This option is only available with vhost backend driver. > + <span class="since">Since 2.7.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 4950ddc..f304385 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -2687,6 +2687,11 @@ > <optional> > <ref name="event_idx"/> > </optional> > + <optional> > + <attribute name='poll_us'> > + <ref name='positiveInteger'/> > + </attribute> > + </optional> > </group> > </choice> > <ref name="virtioOptions"/> As mentioned in the previous patch, the RNG and docs changes should be in the previous patch. (In this patch you're going to want to add an entry to docs/news.xml) > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 8c12b2b..412496e 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -3955,6 +3955,9 @@ qemuBuildHostNetStr(virDomainNetDefPtr net, > } > if (net->tune.sndbuf_specified) > virBufferAsprintf(&buf, "sndbuf=%lu,", net->tune.sndbuf); > + if (net->driver.virtio.pollus) > + virBufferAsprintf(&buf, "poll-us=%u,", > + net->driver.virtio.pollus); > } > > virObjectUnref(cfg); > @@ -8451,6 +8454,25 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, > return -1; > } > > + if (net->driver.virtio.pollus > 0) { > + if (net->driver.virtio.name != VIR_DOMAIN_NET_BACKEND_TYPE_VHOST) { I haven't checked - does this get set automatically when the vhost-net FD's are successfully opened? > + virReportError( > + VIR_ERR_CONFIG_UNSUPPORTED, > + _("Busy polling is only supported with vhost backend driver")); I wonder if anyone would be confused by the lack of the exact option name "poll_us" in these error messages. > + return -1; > + } > + /* Nothing besides TAP devices supports busy polling. */ > + if (!(actualType == VIR_DOMAIN_NET_TYPE_NETWORK || > + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || > + actualType == VIR_DOMAIN_NET_TYPE_DIRECT || > + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Busy polling is not supported for: %s"), > + virDomainNetTypeToString(actualType)); > + return -1; > + } > + } > + > /* and only TAP devices support nwfilter rules */ > if (net->filter && > !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK || > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 0b8d3d8..33884b7 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -995,6 +995,18 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver, > return -1; > } > > + /* and nothing besides TAP devices supports busy polling. */ > + if (net->driver.virtio.pollus > 0 && > + !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK || > + actualType == VIR_DOMAIN_NET_TYPE_BRIDGE || > + actualType == VIR_DOMAIN_NET_TYPE_DIRECT || > + actualType == VIR_DOMAIN_NET_TYPE_ETHERNET)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Busy polling is not supported for: %s"), > + virDomainNetTypeToString(actualType)); > + return -1; > + } Huh. I just noticed there's a lot of duplicated code between qemuBuildInterfaceCommandLine() and qemuDomainAttachNetDevice()... (not your problem, but it seems like this is just *screaming* to be vombined/consolidated to eliminate behavioral differences / half-fixed bugs etc. > + > /* and only TAP devices support nwfilter rules */ > if (net->filter && > !(actualType == VIR_DOMAIN_NET_TYPE_NETWORK || > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus-fail.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus-fail.xml > new file mode 100644 > index 0000000..3b664b9 > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus-fail.xml > @@ -0,0 +1,23 @@ > +<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-system-i686</emulator> > + <interface type='ethernet'> > + <mac address='52:54:00:23:bc:ba'/> > + <model type='virtio'/> > + <driver poll_us='50'/> So this is supposed to fail because the config doesn't explicitly say "driver name='vhost'". But as I said in the review of patch 2, vhost-net is used by default. So this would normally be a correct config.

> + </interface> > + </devices> > +</domain> > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus-qemu-fail.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus-qemu-fail.xml > new file mode 100644 > index 0000000..18999c3 > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus-qemu-fail.xml > @@ -0,0 +1,23 @@ > +<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-system-i686</emulator> > + <interface type='ethernet'> > + <mac address='52:54:00:23:bc:ba'/> > + <model type='virtio'/> > + <driver name="qemu" poll_us='50'/> This one is okay - it *should* fail. > + </interface> > + </devices> > +</domain> > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus.args b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus.args > new file mode 100644 > index 0000000..8d44134 > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus.args > @@ -0,0 +1,23 @@ > +LC_ALL=C \ > +PATH=/bin \ > +HOME=/home/test \ > +USER=test \ > +LOGNAME=test \ > +QEMU_AUDIO_DRV=none \ > +/usr/bin/qemu-system-i686 \ > +-name QEMUGuest1 \ > +-S \ > +-M pc \ > +-m 214 \ > +-smp 1,sockets=1,cores=1,threads=1 \ > +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ > +-nographic \ > +-nodefaults \ > +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ > +-no-acpi \ > +-boot c \ > +-usb \ > +-netdev tap,fd=3,id=hostnet0,poll-us=50 \ > +-device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:23:bc:ba,bus=pci.0,\ > +addr=0x3 \ > +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus.xml > new file mode 100644 > index 0000000..46ef7d6 > --- /dev/null > +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-virtio-netdev-pollus.xml > @@ -0,0 +1,23 @@ > +<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-system-i686</emulator> > + <interface type='ethernet'> > + <mac address='52:54:00:23:bc:ba'/> > + <model type='virtio'/> > + <driver name="vhost" poll_us='50'/> > + </interface> > + </devices> > +</domain> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c > index 799aea9..54d1b07 100644 > --- a/tests/qemuxml2argvtest.c > +++ b/tests/qemuxml2argvtest.c > @@ -1133,6 +1133,15 @@ mymain(void) > QEMU_CAPS_NODEFCONFIG); > DO_TEST("net-virtio-netdev", > QEMU_CAPS_NETDEV, QEMU_CAPS_NODEFCONFIG); > + DO_TEST("net-virtio-netdev-pollus", > + QEMU_CAPS_NETDEV, > + QEMU_CAPS_VHOST_NET_POLL_US); > + DO_TEST_FAILURE("net-virtio-netdev-pollus-fail", > + QEMU_CAPS_NETDEV, > + QEMU_CAPS_VHOST_NET_POLL_US); > + DO_TEST_FAILURE("net-virtio-netdev-pollus-qemu-fail", > + QEMU_CAPS_NETDEV, > + QEMU_CAPS_VHOST_NET_POLL_US); > DO_TEST("net-virtio-s390", > QEMU_CAPS_VIRTIO_S390); > DO_TEST("net-virtio-ccw", >