[libvirt] [PATCHv2 0/3] exposing busy polling support for vhost-net

sferdjao@redhat.com posted 3 patches 6 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20170717112727.28348-1-sferdjao@redhat.com
docs/formatdomain.html.in                          | 12 +++++++-
docs/schemas/domaincommon.rng                      |  5 ++++
src/conf/domain_conf.c                             | 16 +++++++++++
src/conf/domain_conf.h                             |  1 +
src/qemu/qemu_capabilities.c                       |  5 ++++
src/qemu/qemu_capabilities.h                       |  1 +
src/qemu/qemu_command.c                            | 28 ++++++++++++++++++
src/qemu/qemu_hotplug.c                            | 12 ++++++++
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 +
.../qemuxml2argv-net-virtio-netdev-pollus-fail.xml | 23 +++++++++++++++
...xml2argv-net-virtio-netdev-pollus-qemu-fail.xml | 23 +++++++++++++++
.../qemuxml2argv-net-virtio-netdev-pollus.args     | 25 ++++++++++++++++
.../qemuxml2argv-net-virtio-netdev-pollus.xml      | 23 +++++++++++++++
.../qemuxml2argv-net-virtio-poll-us.xml            | 23 +++++++++++++++
tests/qemuxml2argvtest.c                           |  9 ++++++
.../qemuxml2xmlout-net-virtio-poll-us.xml          | 33 ++++++++++++++++++++++
tests/qemuxml2xmltest.c                            |  1 +
21 files changed, 244 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
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-poll-us.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-net-virtio-poll-us.xml
[libvirt] [PATCHv2 0/3] exposing busy polling support for vhost-net
Posted by sferdjao@redhat.com 6 years, 9 months ago
From: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@redhat.com>

In version 2.7.0, QEMU introduced support of busy polling for
vhost-net [0]. To avoid paraphrasing original authors of that feature
and the purpose it I prefer to share a pointer [1].

This patch serie exposes throught the NIC driver-specific element a
new option 'poll_us'. That option is only available with the backend
driver 'vhost' and that because libvirt automatically fallback to QEMU
if the driver is not specified where that option is not available.

The option 'poll_us' takes a positive. 0 means that the option
is not going to be exposed.

[0] 69e87b32680a41d9761191443587c595b6f5fc3f
[1] https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg04595.html

Sahid Orentino Ferdjaoui (3):
  qemu: detect support for vhost-net busy polling
  conf: introduce 'poll_us' attribute for domain interface
  This patch finalizes support of 'poll_us' attribute as an option of
    the NIC driver-specific element, the commit also takes care of
    hotplug.

 docs/formatdomain.html.in                          | 12 +++++++-
 docs/schemas/domaincommon.rng                      |  5 ++++
 src/conf/domain_conf.c                             | 16 +++++++++++
 src/conf/domain_conf.h                             |  1 +
 src/qemu/qemu_capabilities.c                       |  5 ++++
 src/qemu/qemu_capabilities.h                       |  1 +
 src/qemu/qemu_command.c                            | 28 ++++++++++++++++++
 src/qemu/qemu_hotplug.c                            | 12 ++++++++
 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 +
 .../qemuxml2argv-net-virtio-netdev-pollus-fail.xml | 23 +++++++++++++++
 ...xml2argv-net-virtio-netdev-pollus-qemu-fail.xml | 23 +++++++++++++++
 .../qemuxml2argv-net-virtio-netdev-pollus.args     | 25 ++++++++++++++++
 .../qemuxml2argv-net-virtio-netdev-pollus.xml      | 23 +++++++++++++++
 .../qemuxml2argv-net-virtio-poll-us.xml            | 23 +++++++++++++++
 tests/qemuxml2argvtest.c                           |  9 ++++++
 .../qemuxml2xmlout-net-virtio-poll-us.xml          | 33 ++++++++++++++++++++++
 tests/qemuxml2xmltest.c                            |  1 +
 21 files changed, 244 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
 create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-virtio-poll-us.xml
 create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-net-virtio-poll-us.xml

-- 
2.9.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 0/3] exposing busy polling support for vhost-net
Posted by Peter Krempa 6 years, 9 months ago
On Mon, Jul 17, 2017 at 07:27:24 -0400, sferdjao@redhat.com wrote:
> From: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@redhat.com>
> 
> In version 2.7.0, QEMU introduced support of busy polling for
> vhost-net [0]. To avoid paraphrasing original authors of that feature
> and the purpose it I prefer to share a pointer [1].
> 
> This patch serie exposes throught the NIC driver-specific element a
> new option 'poll_us'. That option is only available with the backend
> driver 'vhost' and that because libvirt automatically fallback to QEMU
> if the driver is not specified where that option is not available.
> 
> The option 'poll_us' takes a positive. 0 means that the option
> is not going to be exposed.

We had a similar attempt to do this for disk polling, but that was
rejected since it's not very straightforward for the users to tune this
variable. I think this falls into the same category.

Here's the discussion for iothread polling:

https://www.redhat.com/archives/libvir-list/2017-February/msg01048.html
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 0/3] exposing busy polling support for vhost-net
Posted by Laine Stump 6 years, 9 months ago
On 07/17/2017 07:39 AM, Peter Krempa wrote:
> On Mon, Jul 17, 2017 at 07:27:24 -0400, sferdjao@redhat.com wrote:
>> From: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@redhat.com>
>>
>> In version 2.7.0, QEMU introduced support of busy polling for
>> vhost-net [0]. To avoid paraphrasing original authors of that feature
>> and the purpose it I prefer to share a pointer [1].
>>
>> This patch serie exposes throught the NIC driver-specific element a
>> new option 'poll_us'. That option is only available with the backend
>> driver 'vhost' and that because libvirt automatically fallback to QEMU
>> if the driver is not specified where that option is not available.
>>
>> The option 'poll_us' takes a positive. 0 means that the option
>> is not going to be exposed.
> We had a similar attempt to do this for disk polling, but that was
> rejected since it's not very straightforward for the users to tune this
> variable. I think this falls into the same category.
>
> Here's the discussion for iothread polling:
>
> https://www.redhat.com/archives/libvir-list/2017-February/msg01048.html

The problem is there are already many similar driver-specific
odd/not-straightforward tuning parameters present in the XML, so it's
difficult to see where to draw the line. I think this may be due to the
fact that now whenever a new option is added to qemu, a bugzilla record
is semi-automatically opened (by a human, but they almost always do it)
requesting libvirt to support that new option, and libvirt developers
like to be accommodating (and while some reviews/reviewers base the
conversation on "should we even do this?", many/most are simply based on
"assuming we want to do this, is this a proper way to do it?", or even
"assuming we want to do this, and this is the proper way to do it, has
all allocated memory been freed, and error conditions handled?" (e.g. my
own review of V1 of these patches :-P).

In one of the followups to the iothread polling above, Stefan pointed
out a way to accomplish the tuning via qemu commandline passthrough,
using qemu's "-set" commandline argument. If libvirt doesn't add support
for this attribute in the XML, that's how it will need to be set if
someone requires it. And it *is* possible to set poll-us for an
interface in this way, e.g.:


  <qemu:commandline>
    <qemu:arg value='-set'/>
    <qemu:arg value='netdev.hostnet1.poll-us=20'/>
  </qemu:commandline>


which results in this addition to the commandline:

  -set netdev.hostnet1.poll-us=20

As long as the commandline generated for the interface was something
like this:

  -netdev tap,fd=30,id=hostnet1,vhost=on,vhostfd=32

everything will work out properly.

There are 3 issues with this though:

1) it depends on the specific interface you want to control having the
id ("alias" in libvirt-speak) "hostnet0". If the config is modified to
add/remove any interfaces prior to this one in the config, the alias
will change, and the qemu:commandline will silently begin modifying the
wrong interface. (although we've discussed it, I think it is still not
possible to explicitly set the alias for a device - they are always
automatically determined when the domain is started / device is hotplugged)

2) libvirt documentation explicitly says that use of <qemu:commandline>
is not supported, and any use of it will taint the domain config. This
means that nobody will be able to use it in a supported commercial setting.

....


Well, I forget what the 3rd reason was, but it was a *good* one, and it
made a lot of sense 3 days ago when I started writing this message :-P.

Oh, wait, now I remember:

3) It's not possible to set options using <qemu:commandline> when
hotplugging a device, so the functionality would only be available for
interfaces that were present when the domain was started.


Anyway, I'm sympathetic to the sentiment of "don't add frivolous new
knobs to libvirt config just because someone asked for it and it's easy
to do". I've made that same argument before myself. I just think that if
we're going to be restrictive like that, we need to be consistent about
it, and need to have a story for how to accommodate the request for the
functionality in a different way that doesn't have limited
functionality, and doesn't render the software unsupportable. I think
that in this case none of those is true.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 0/3] exposing busy polling support for vhost-net
Posted by Daniel P. Berrange 6 years, 8 months ago
On Mon, Jul 17, 2017 at 01:39:13PM +0200, Peter Krempa wrote:
> On Mon, Jul 17, 2017 at 07:27:24 -0400, sferdjao@redhat.com wrote:
> > From: Sahid Orentino Ferdjaoui <sahid.ferdjaoui@redhat.com>
> > 
> > In version 2.7.0, QEMU introduced support of busy polling for
> > vhost-net [0]. To avoid paraphrasing original authors of that feature
> > and the purpose it I prefer to share a pointer [1].
> > 
> > This patch serie exposes throught the NIC driver-specific element a
> > new option 'poll_us'. That option is only available with the backend
> > driver 'vhost' and that because libvirt automatically fallback to QEMU
> > if the driver is not specified where that option is not available.
> > 
> > The option 'poll_us' takes a positive. 0 means that the option
> > is not going to be exposed.
> 
> We had a similar attempt to do this for disk polling, but that was
> rejected since it's not very straightforward for the users to tune this
> variable. I think this falls into the same category.
> 
> Here's the discussion for iothread polling:
> 
> https://www.redhat.com/archives/libvir-list/2017-February/msg01048.html

Yes, thanks for pointing that out. I do have the same objections to this
patch, as for the previous disk polling patch. I just don't think they
are practical for a appliction to use - they're too low level to expose
to sysadmins, and there's no obvious way for an application to pick
the right settings automatically. So I think we're best served by letting
QEMU pick defaults

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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