[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 01/14/2011 08:22 AM, Daniel P. Berrange wrote:
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.


Okay, I'm convinced! :-)


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.

Right. What I didn't like was that, using the VIR_ENUM_IMPL() macro, we would end up giving the user the ability to put <driver name='default'/> into the XML, which would then disappear. In his review, Eric pointed out the elegant solution of simply generating an error during parse if they try to enter that value; everything else will already do what we want. So that's what I'm doing in V2.


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