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

Re: [libvirt] [PATCH 2/2] Add tx_alg attribute to interface XML for virtio backend

On 02/07/2011 12:06 PM, Eric Blake wrote:
On 02/04/2011 02:00 PM, Laine Stump wrote:
qemu's virtio-net-pci driver allows setting the algorithm used for tx
packets to either "bh" or "timer". I don't know exactly how these
algorithms differ, but someone has requested the ability to choose
between them in libvirt's domain XML. See:

Same as qemu aio - we don't have to know exactly what the difference is,
to know that someone else who does know the difference wants to be able
to choose :)

     <interface ...>
       <model type='virtio'/>
       <driver tx_alg='bh'/>

I chose to put this setting as an attribute to<driver>  rather than as
a sub-element to<tune>  because it is specific to the virtio-net
driver, not something that is generally usable by all network drivers.
(note that this is the same placement as the "driver name=..."
attribute used to choose kernel vs. userland backend for the
virtio-net driver.)
I take it that tx_alg applies to both options of driver name=... when
using a virtio interface, right?

I assume so, but know little beyond the name of the option :-)

This is a bit troublesome to me, because I can see
lots of new virtio options that could potentially be requested (just
run "qemu-kvm -device virtio-net-pci,?" on a qemu that's version
0.13.0 or newer, and compare that output to potential tunable items in
"-device e1000,?" or "-net tap,..."), so the attribute list could
potentially get quite long (which is something I was concerned about
when I first added the option to select kernel vs. userland backend,
but didn't realize just how many virtio-specific options there were).
I'd like feedback from danpb or DV, since this might be a long-term XML

Me too! :-)

Maybe it makes sense to introduce sub-elements of<driver>,
according to the driver chosen?

   <interface ...>
       <tunable name='tx_alg'>bh</tunable>

But I'm not sure if that is any better.

Well, there is already a <tune> element inside <interface> that works similar to <memtune> and <cputune>:

<interface ...>

One alternative would be to place tx_alg there, and ignore it when model type != 'virtio'. That seems a bit cheesy, though. Another alternative would be to put a "<tune> element inside <driver> that looks just like memtune, cputune, and interface/tune.

Again, the issue here is concern over the attribute list of <driver> getting excessively long. If that's not an issue, then leaving tx_alg as a simple attribute will probably be fine.

If the option isn't listed there, the config item is ignored (should
it instead generate a warning log? error out and prevent the domain
from starting?)
I would lean towards erroring out, the same way that we error out if:

   <driver name='vhost'/>

is explicitly requested when the kernel module is not present.

Okay. I'll make it that way in the next revision.


+            virBufferVSprintf(&buf, ",tx=%s",
+                              virDomainNetVirtioTxAlgTypeToString(net->driver.virtio.tx_alg));
+        } else {
+            /* What should we do if the option isn't available? log a
+             * warning? prevent the domain from starting? Ignore it?
+             * Right now we're ignoring it.
+             */
This would be the perfect place to error out with

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