[libvirt] [PATCH] Add XML config switch to enable/disable vhost-net support

[Eric Blake]

On 01/12/2011 11:45 PM, Laine Stump wrote:
> This patch is in response to
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=643050
> 
> The existing libvirt support for the vhost-net backend to the virtio
> network driver happens automatically - if the vhost-net device is
> available, it is always enabled, otherwise the standard userland
> virtio backend is used.
> 
> This patch makes it possible to force whether or not vhost-net is used
> with a bit of XML. Adding a <driver> element to the interface XML, eg:
> 
>      <interface type="network">
>        <model type="virtio"/>
>        <driver name="vhost"/>
> 
> will force use of vhost-net (if it's not available, the domain will
> fail to start). if driver name="qemu", vhost-net will not be used even
> if it is available.
> 
> If there is no <driver name='xxx'/> in the config, libvirt will revert
> to the pre-existing automatic behavior - use vhost-net if it's
> available, and userland backend if vhost-net isn't available.
> ---
> 
> Note that I don't really like the "name='vhost|qemu'" nomenclature,
...
> (So far the strongest opinion has been for the current
"name='vhost|qemu'")

Also, using name= as the attribute name matches the fact that we already
have <driver name='xyz'> for <disk>.

> 
> Oh, and also - sorry Eric, but I didn't have the brain cells left
> tonight to add this new bit to the documentation, and I really want to
> get the patch up/in now, so that will have to wait for a followup next
> week :-)

I'll have to live with that, but come closer to the 0.8.8 release date,
watch for the reminders if you haven't followed through :)

Meanwhile, https://bugzilla.redhat.com/show_bug.cgi?id=643050#c3 hints
that we may yet need to make an explicit third state (particularly if
virsh dumpxml always lists the chosen driver rather than staying silent
in the default case) - it's easier to add a third state later than it is
to wish we hadn't added it up front, so I'm glad that your initial patch
only proposes two valid strings in the XML.

> diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
> index a524e4b..6d0654d 100644
> --- a/docs/schemas/domain.rng
> +++ b/docs/schemas/domain.rng
> @@ -1005,6 +1005,19 @@
>          </element>
>        </optional>
>        <optional>
> +        <element name="driver">
> +          <optional>
> +            <attribute name="name">

Thinking aloud:  This permits <driver/> with no attributes, which looks
weird.  For comparison with the <disk> element, the rng requires the
driver element has to have at least one attribute from the set (name,
cache), which means name is optional there.  Which means on the one
hand, <disk> will never have <driver/> without attributes, but on the
other hand, why should name be mandatory here if it is optional there.
I guess that means this is fine to keep the <optional> here.

> @@ -2558,6 +2565,18 @@ virDomainNetDefParseXML(virCapsPtr caps,
>          model = NULL;
>      }
>  
> +    if ((backend != NULL) &&
> +        (def->model && STREQ(def->model, "virtio"))) {
> +        int b;
> +        if ((b = virDomainNetBackendTypeFromString(backend)) < 0) {
> +            virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> +                                 _("Unkown interface <driver name='%s'> has been specified"),

s/Unkown/Unknown/

> +                                 backend);
> +            goto error;
> +        }
> +        def->backend = b;
> +        def->backend_specified = 1;

It seems like our debate on IRC has been whether this two-variable
tracking is better than a three-way enum value.  If we go with the
three-way enum (default, qemu, vhost), then you only need one variable,
but this line of code needs one extra check that rejects
STREQ(backend,"default") (or else the XML parsing would silently accept
name='default' but discard it).

[for comparison with your sndbuf patch - there, you have to use
sndbuf_specified, since sndbuf==0 is a valid but non-default
configuration.  But here, using a three-state enum provides a perfect
default without having to track two variables, at the cost of having to
check at parse time that only two of the three states can be explicitly
used, because the default third state happens by the absence of an
explicit request.]

> @@ -6275,9 +6295,14 @@ virDomainNetDefFormat(virBufferPtr buf,
>      if (def->ifname)
>          virBufferEscapeString(buf, "      <target dev='%s'/>\n",
>                                def->ifname);
> -    if (def->model)
> +    if (def->model) {
>          virBufferEscapeString(buf, "      <model type='%s'/>\n",
>                                def->model);
> +        if (STREQ(def->model, "virtio") && def->backend_specified) {

with a tri-state enum, here, the condition would be def->backend !=
VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT

>  
> +/* the backend driver used for virtio interfaces */
> +enum virDomainNetBackendType {

and here, you would have VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT = 0, /* try
kernel, but fall back to userland */

> +    VIR_DOMAIN_NET_BACKEND_TYPE_QEMU,         /* userland */
> +    VIR_DOMAIN_NET_BACKEND_TYPE_VHOST,        /* kernel */
> +
> +    VIR_DOMAIN_NET_BACKEND_TYPE_LAST,
> +};
>  
>  /* the mode type for macvtap devices */
>  enum virDomainNetdevMacvtapType {
> @@ -310,6 +317,8 @@ struct _virDomainNetDef {
>      enum virDomainNetType type;
>      unsigned char mac[VIR_MAC_BUFLEN];
>      char *model;
> +    enum virDomainNetBackendType backend;
> +    int backend_specified : 1;

and here you could lose backend_specified.

I guess another advantage of keeping a tri-state enum rather than two
variables is that if the request to have an explicit name='default' ever
materializes in the future, we would already have the enum set up for that.

> +
> +    *vhostfd = open("/dev/vhost-net", O_RDWR, 0);

The third argument is not necessary, since the second does not include
O_CREAT.

I can live with the patch as-is (once you fix the spelling and open()
nits), but if you have time, I wouldn't mind a v2 with the approach of a
tri-state enum.  So, you have a weak:

ACK.

-- 
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/20110113/6eab2db8/attachment-0001.sig>