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

[Eric Blake]

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:
> 
>    https://bugzilla.redhat.com/show_bug.cgi?id=629662

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'/>
>       ...
>     </interface>
> 
> 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?

> 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
commitment.  Maybe it makes sense to introduce sub-elements of <driver>,
according to the driver chosen?

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

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

> 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.

> @@ -6808,12 +6828,16 @@ virDomainNetDefFormat(virBufferPtr buf,
>          virBufferEscapeString(buf, "      <model type='%s'/>\n",
>                                def->model);
>          if (STREQ(def->model, "virtio") &&
> -            def->driver.virtio.name) {
> +            (def->driver.virtio.name || def->driver.virtio.tx_alg)) {
>              virBufferAddLit(buf, "      <driver");
>              if (def->driver.virtio.name) {
>                  virBufferVSprintf(buf, " name='%s'",
>                                    virDomainNetBackendTypeToString(def->driver.virtio.name));
>              }
> +            if (def->driver.virtio.tx_alg) {
> +                virBufferVSprintf(buf, " tx_alg='%s'",
> +                                  virDomainNetVirtioTxAlgTypeToString(def->driver.virtio.tx_alg));
> +            }
>              virBufferAddLit(buf, "/>\n");

Obviously, this would change if we settle on a different XML representation.

> +++ b/src/qemu/qemu_capabilities.c
> @@ -1063,6 +1063,7 @@ qemuCapsExtractDeviceStr(const char *qemu,
>                                 "-device", "?",
>                                 "-device", "pci-assign,?",
>                                 "-device", "virtio-blk-pci,?",
> +                               "-device", "virtio-net-pci,?",
>                                 NULL);
>      virCommandAddEnvPassCommon(cmd);
>      /* qemu -help goes to stdout, but qemu -device ? goes to stderr.  */
> @@ -1104,6 +1105,8 @@ qemuCapsParseDeviceStr(const char *str, unsigned long long *flags)
>          if (strstr(str, "pci-assign.bootindex"))
>              *flags |= QEMUD_CMD_FLAG_PCI_BOOTINDEX;
>      }
> +    if (strstr(str, "virtio-net-pci.tx="))
> +        *flags |= QEMUD_CMD_FLAG_VIRTIO_TX_ALG;

That is just so slick for detecting the new bit!  I'm glad I added that
improvement in -device string parsing.

> +++ b/src/qemu/qemu_capabilities.h
> @@ -92,6 +92,7 @@ enum qemuCapsFlags {
>      QEMUD_CMD_FLAG_CCID_PASSTHRU = (1LL << 55), /* -device ccid-card-passthru */
>      QEMUD_CMD_FLAG_CHARDEV_SPICEVMC = (1LL << 56), /* newer -chardev spicevmc */
>      QEMUD_CMD_FLAG_DEVICE_SPICEVMC = (1LL << 57), /* older -device spicevmc*/
> +    QEMUD_CMD_FLAG_VIRTIO_TX_ALG = (1LL << 58), /* -device virtio-net-pci,tx=string */

5 more bits left.  Get your patches in now, before we run out of space.
 Last one in has to rewrite this to be a bitset :)

>      virBufferAdd(&buf, nic, strlen(nic));
> +    if (usingVirtio && net->driver.virtio.tx_alg) {
> +        if (qemuCmdFlags & QEMUD_CMD_FLAG_VIRTIO_TX_ALG) {
> +            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
VIR_ERR_CONFIG_UNSUPPORTED.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110207/d502c999/attachment-0001.sig>