[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 06/15/2017 10:17 AM, sferdjao redhat com wrote:
> From: Sahid Orentino Ferdjaoui <sahid ferdjaoui redhat com>
> 
> 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 redhat com>
> ---
>  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",
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]