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@xxxxxxxxxx +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list