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

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



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.


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