[v1] exposing busy polling support for vhost-net

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

Posted by sferdjao@redhat.com 8 years, 7 months ago

From: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@redhat.com>

QEMU 2.7.0 adds support of busy polling on vhost-net.

  69e87b32680a41d9761191443587c595b6f5fc3f

Signed-off-by: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@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",
     );
 
 
@@ -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);
+
     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= */
 
     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>
-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Laine Stump 8 years, 7 months ago

(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@redhat.com wrote:
> From: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@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@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>
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Eduardo Habkost 8 years, 7 months ago

(CCing qemu-devel and the QAPI maintainers.  I have a question
about introspection below.)

On Sat, Jun 17, 2017 at 11:48:10AM -0400, Laine Stump wrote:
[...]
> > +    /* 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.

query-command-line-options is incomplete and useless on many
cases (including but not limited to -device, -object, and
-netdev).  It is not capable to model cases where the set of
accepted options depends on a discriminator option (e.g. a
"driver" or "type" option).

Fortunately, netdev options are modelled in the QAPI schema as
union Netdev.  However, 'query-qmp-schema' doesn't seem to
include union Netdev because it is not referenced by any QMP
command or event.

Markus, Eric, Michael: is there any way libvirt can query the
definition of union Netdev from the schema with current QEMU?

-- 
Eduardo

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

Posted by Eric Blake 8 years, 7 months ago

On 06/17/2017 09:15 PM, Eduardo Habkost wrote:
> (CCing qemu-devel and the QAPI maintainers.  I have a question
> about introspection below.)
> 

> 
> Fortunately, netdev options are modelled in the QAPI schema as
> union Netdev.  However, 'query-qmp-schema' doesn't seem to
> include union Netdev because it is not referenced by any QMP
> command or event.

I've posted patches in the past (qemu 2.6 timeframe, if I recall) that
changed netdev_add into a fully-advertised interface, but we didn't take
it then because we weren't sure how to handle the fact that netdev_add
can currently accept both an integer (1) and a string ("1") as
identical, and we didn't want to risk breaking clients that passed a
string when the promoted command would only accept integers.  I guess we
should revive that.

> 
> Markus, Eric, Michael: is there any way libvirt can query the
> definition of union Netdev from the schema with current QEMU?

Really, we need to promote netdev_add to a full-fledged QMP command.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

Posted by Eduardo Habkost 8 years, 7 months ago

On Mon, Jun 26, 2017 at 07:23:50AM -0600, Eric Blake wrote:
> On 06/17/2017 09:15 PM, Eduardo Habkost wrote:
> > (CCing qemu-devel and the QAPI maintainers.  I have a question
> > about introspection below.)
> > 
> 
> > 
> > Fortunately, netdev options are modelled in the QAPI schema as
> > union Netdev.  However, 'query-qmp-schema' doesn't seem to
> > include union Netdev because it is not referenced by any QMP
> > command or event.
> 
> I've posted patches in the past (qemu 2.6 timeframe, if I recall) that
> changed netdev_add into a fully-advertised interface, but we didn't take
> it then because we weren't sure how to handle the fact that netdev_add
> can currently accept both an integer (1) and a string ("1") as
> identical, and we didn't want to risk breaking clients that passed a
> string when the promoted command would only accept integers.  I guess we
> should revive that.
> 
> > 
> > Markus, Eric, Michael: is there any way libvirt can query the
> > definition of union Netdev from the schema with current QEMU?
> 
> Really, we need to promote netdev_add to a full-fledged QMP command.

This may be enough for -netdev, but do we want to make command-line
option introspection always depend on having the same struct being used
in a QMP command to work?  What if the arguments for the command-line
option don't match precisely the input of a QMP command?

-- 
Eduardo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/3] qemu: add capability for vhost-net busy polling
[libvirt] [PATCH 2/3] conf: introduce 'poll_us' attribute for domain interface
[libvirt] [PATCH 3/3] qemu: add busy polling support