[libvirt] [PATCH 1/3] qemu: add capability for vhost-net busy polling

Sat Jun 17 15:48:10 UTC 2017

(Eduardo - I Cc'ed you because there's a question you may be able to
answer about halfway down the message...)

On 06/15/2017 10:17 AM, sferdjao at redhat.com wrote:
> From: Sahid Orentino Ferdjaoui <sahid.ferdjaoui at redhat.com>
> 
> QEMU 2.7.0 adds support of busy polling on vhost-net.
> 
>   69e87b32680a41d9761191443587c595b6f5fc3f

I guess this is the upstream commit ID of the feature in qemu? I haven't
noticed people referencing qemu commit ID's in libvirt in the past, but
there's nothing wrong with it either. If you're going to put it in
though, you should make it clear what the number is, maybe something like:

   Detect support for vhost-net busy polling (the "poll-us"
   commandline option). This was added to upstream qemu in
   version 2.7.0, commit 69e87b32....

> 
> Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c                     | 6 ++++++
>  src/qemu/qemu_capabilities.h                     | 1 +
>  tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml  | 1 +
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 1 +
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 +
>  7 files changed, 12 insertions(+)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index c0c39bd..faf6d82 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -376,6 +376,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>                "intel-iommu.device-iotlb", /* 260 */
>                "virtio.iommu_platform",
>                "virtio.ats",
> +              "vhost-net.poll_us",

It's not 100% done in existing code, but generally people have tried to
reproduce qemu option names exactly in the capability name (I think less
so in the past, more so now). So this should be vhost-net.poll-us (dash
instead of underscore), or even more exactly "netdev.poll-us" or
"netdev.tap.poll-us".

>      );
>  
>  
> @@ -4773,6 +4774,11 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
>      if (qemuCaps->version >= 2006000)
>          virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT);
>  
> +    /* Support for busy polling on vhost-net devices ("-netdev
> +     * tap,...,poll-us=n") */
> +    if (qemuCaps->version >= 2007000)
> +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_VHOST_NET_POLL_US);
> +

Enabling a capability based purely on the qemu binary version is
something that's only done as a last resort if it can't be determined by
QMP probes. This is because the version number is an inaccurate
indicator - it often happens that a new capability is added to upstream
in qemu version "Y", but then that feature is backported to qemu version
"X" in some downstream distro package; if we based the capability on
version number, then libvirt wouldn't take advantage of the feature even
though qemu had it.

What *should* get us the info we need is to add a table entry to
virQEMUCapsCommandLine to check whether or not the poll-us option exists
in the current qemu binary. Something like this:

static struct virQEMUCapsCommandLineProps virQEMUCapsCommandLine[] = {
    ...
    { "netdev", "poll-us", QEMU_CAPS_NETDEV_POLL_US },

Unfortunately when I add that and regenerate the .replies file for qemu
2.9.0 (with the command "tests/qemucapsprobe
/usr/bin/qemu-system-x86_64"), this is the info it finds for netdev:

    {
      "parameters": [
      ],
      "option": "netdev"
    },

My understanding was that the possible parameters to the netdev
commandline option should be listed, but instead the parameters list is
empty :-/.

Maybe Eduardo can help me understand why the parameters are empty, and
(more importantly) what needs to be done to get the supported parameters
for -netdev - basically we need to know what are the available
parameters to "-netdev tap,...". I hope we don't end up needing to base
the check purely on qemu version.

>      if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0)
>          goto cleanup;
>  
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index e57cae9..33f8ed0 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -415,6 +415,7 @@ typedef enum {
>      QEMU_CAPS_INTEL_IOMMU_DEVICE_IOTLB, /* intel-iommu.device-iotlb */
>      QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM, /* virtio-*-pci.iommu_platform */
>      QEMU_CAPS_VIRTIO_PCI_ATS, /* virtio-*-pci.ats */
> +    QEMU_CAPS_VHOST_NET_POLL_US, /* vhost-net with -netdev poll-us= */

As implied up above, I think the capability should be called
QEMU_CAPS_NETDEV_POLL_US, since that's an exact description of that goes
on the commandline (and even though it currently only has an effect when
vhost-net is turned on, that could possibly change in the future).

>  
>      QEMU_CAPS_LAST /* this must always be the last item */
>  } virQEMUCapsFlags;
> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
> index 70cce64..f833d4a 100644
> --- a/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.7.0.s390x.xml
> @@ -132,6 +132,7 @@
>    <flag name='query-named-block-nodes'/>
>    <flag name='kernel-irqchip'/>
>    <flag name='kernel-irqchip.split'/>
> +  <flag name='vhost-net.poll_us'/>
>    <version>2007000</version>
>    <kvmVersion>0</kvmVersion>
>    <package></package>
> diff --git a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
> index 49c0462..2ab44cf 100644
> --- a/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml
> @@ -205,6 +205,7 @@
>    <flag name='kernel-irqchip'/>
>    <flag name='kernel-irqchip.split'/>
>    <flag name='intel-iommu.intremap'/>
> +  <flag name='vhost-net.poll_us'/>
>    <version>2007000</version>
>    <kvmVersion>0</kvmVersion>
>    <package> (v2.7.0)</package>
> diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml
> index 51be9bc..98f763d 100644
> --- a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml
> @@ -134,6 +134,7 @@
>    <flag name='query-named-block-nodes'/>
>    <flag name='kernel-irqchip'/>
>    <flag name='kernel-irqchip.split'/>
> +  <flag name='vhost-net.poll_us'/>
>    <version>2007093</version>
>    <kvmVersion>0</kvmVersion>
>    <package></package>
> diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml
> index 01edbc8..bafe08f 100644
> --- a/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml
> @@ -207,6 +207,7 @@
>    <flag name='kernel-irqchip.split'/>
>    <flag name='intel-iommu.intremap'/>
>    <flag name='intel-iommu.eim'/>
> +  <flag name='vhost-net.poll_us'/>
>    <version>2008000</version>
>    <kvmVersion>0</kvmVersion>
>    <package> (v2.8.0)</package>
> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
> index 58dd9f6..dc93b5a 100644
> --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
> +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml
> @@ -218,6 +218,7 @@
>    <flag name='intel-iommu.device-iotlb'/>
>    <flag name='virtio.iommu_platform'/>
>    <flag name='virtio.ats'/>
> +  <flag name='vhost-net.poll_us'/>
>    <version>2009000</version>
>    <kvmVersion>0</kvmVersion>
>    <package> (v2.9.0)</package>
> 