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

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



On Thu, Jan 13, 2011 at 01:45:28AM -0500, 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,
> but am including it here just to get the patches on the list. I could
> live with it this way, or with any of the following (anyone have a
> strong opinion?) (note that in all cases, nothing specified means "try
> to use vhost, but fallback to userland if necessary")
> 
>    vhost='on|off'
>    vhost='required|disabled'
>    mode='vhost|qemu'
>    mode='kernel|user'
>    backend='kernel|user'

I wanted 'name=' becasue that matches <driver> usage in <disk>.
This rules out the first options completely, since we can just
play with attribute values. kernel|user is nice, except when
QEMU invent  vhost2, and now 'kernel' is no longer unique :-(
Also we used 'name=qemu' already in <disk> to refer to the
in-QEMU disk backend, and there's likely to be a 'vhost' backend
for disks too in the future. So in summary I think 'name=vhost|qemu'
is the best optionl.

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b4df38c..04ed502 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -184,6 +184,10 @@ VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST,
>                "internal",
>                "direct")
>  
> +VIR_ENUM_IMPL(virDomainNetBackend, VIR_DOMAIN_NET_BACKEND_TYPE_LAST,
> +              "qemu",
> +              "vhost")
> +
>  VIR_ENUM_IMPL(virDomainChrChannelTarget,
>                VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_LAST,
>                "guestfwd",
> @@ -2289,6 +2293,7 @@ virDomainNetDefParseXML(virCapsPtr caps,
>      char *address = NULL;
>      char *port = NULL;
>      char *model = NULL;
> +    char *backend = NULL;
>      char *filter = NULL;
>      char *internal = NULL;
>      char *devaddr = NULL;
> @@ -2371,6 +2376,8 @@ virDomainNetDefParseXML(virCapsPtr caps,
>                  script = virXMLPropString(cur, "path");
>              } else if (xmlStrEqual (cur->name, BAD_CAST "model")) {
>                  model = virXMLPropString(cur, "type");
> +            } else if (xmlStrEqual (cur->name, BAD_CAST "driver")) {
> +                backend = virXMLPropString(cur, "name");
>              } else if (xmlStrEqual (cur->name, BAD_CAST "filterref")) {
>                  filter = virXMLPropString(cur, "filter");
>                  VIR_FREE(filterparams);
> @@ -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"),
> +                                 backend);
> +            goto error;
> +        }
> +        def->backend = b;
> +        def->backend_specified = 1;
> +    }
>      if (filter != NULL) {
>          switch (def->type) {
>          case VIR_DOMAIN_NET_TYPE_ETHERNET:
> @@ -2584,6 +2603,7 @@ cleanup:
>      VIR_FREE(script);
>      VIR_FREE(bridge);
>      VIR_FREE(model);
> +    VIR_FREE(backend);
>      VIR_FREE(filter);
>      VIR_FREE(type);
>      VIR_FREE(internal);
> @@ -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) {
> +            virBufferVSprintf(buf, "      <driver name='%s'/>\n",
> +                              virDomainNetBackendTypeToString(def->backend));
> +        }
> +    }
>      if (def->filter) {
>          virBufferEscapeString(buf, "      <filterref filter='%s'",
>                                def->filter);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index a459a22..451ccad 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -292,6 +292,13 @@ enum virDomainNetType {
>      VIR_DOMAIN_NET_TYPE_LAST,
>  };
>  
> +/* the backend driver used for virtio interfaces */
> +enum virDomainNetBackendType {
> +    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;

I don't really like this pattern since it is at odds with the way
we handle defaults elsewhere which is to include the default as
the first enum value, eg

  enum virDomainNetBackendType {
    VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT,      /* Try best available */
    VIR_DOMAIN_NET_BACKEND_TYPE_QEMU,         /* userland */
    VIR_DOMAIN_NET_BACKEND_TYPE_VHOST,        /* kernel */

    VIR_DOMAIN_NET_BACKEND_TYPE_LAST,
  };

And then we do

  if (def->backend)
     ...print XML...

when formatting the XML, so that the 'name=default' never
actually appears in the XML output.

Regards,
Daniel


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