On Sat, Jun 17, 2017 at 12:30:29PM -0400, Laine Stump wrote:
> On 06/15/2017 10:17 AM, sferdjao@xxxxxxxxxx wrote:
> > From: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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
> > <model type='virtio'/>
> > <b><driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rx_queue_size='256'>
> > <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'/>
> > + <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off' poll_us='50'/>
> > </driver>
> > </b>
> > </interface>
> > @@ -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.
If the driver name is not explicitly set it seems that libvirt is
going to use vhost but can fallback to qemu. Since that option is not
available for qemu I prefered to make this explicit.
>
> > + </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",
> >
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
