Re: [libvirt] [PATCH 6/7] Forward Mode Hostdev network driver Implementation

Friday, 17 August 2012

On 08/16/2012 11:42 AM, Shradha Shah wrote:
...
 This patch updates the network driver to properly utilize the new
 attributes/elements that are now in virNetworkDef

 Signed-off-by: Shradha Shah <sshah(a)solarflare.com&gt;
 ---
  docs/formatnetwork.html.in  |   62 ++++++++++
  src/network/bridge_driver.c |  282 ++++++++++++++++++++++++++++++++++---------
  2 files changed, 284 insertions(+), 60 deletions(-)

 diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
 index 38f6d12..065af3e 100644
 --- a/src/network/bridge_driver.c
 +++ b/src/network/bridge_driver.c
 @@ -47,6 +47,7 @@
  #include "datatypes.h"
  #include "bridge_driver.h"
  #include "network_conf.h"
 +#include "device_conf.h"
  #include "driver.h"
  #include "buf.h"
  #include "virpidfile.h"
 @@ -1935,7 +1936,7 @@ networkStartNetworkExternal(struct network_driver *driver
ATTRIBUTE_UNUSED,
                              virNetworkObjPtr network ATTRIBUTE_UNUSED)
  {
      /* put anything here that needs to be done each time a network of
 -     * type BRIDGE, PRIVATE, VEPA, or PASSTHROUGH is started. On
 +     * type BRIDGE, PRIVATE, VEPA, HOSTDEV or PASSTHROUGH is started. On
       * failure, undo anything you've done, and return -1. On success
       * return 0.
       */
 @@ -1946,7 +1947,7 @@ static int networkShutdownNetworkExternal(struct network_driver
*driver ATTRIBUT
                                          virNetworkObjPtr network ATTRIBUTE_UNUSED)
  {
      /* put anything here that needs to be done each time a network of
 -     * type BRIDGE, PRIVATE, VEPA, or PASSTHROUGH is shutdown. On
 +     * type BRIDGE, PRIVATE, VEPA, HOSTDEV or PASSTHROUGH is shutdown. On
       * failure, undo anything you've done, and return -1. On success
       * return 0.
       */
 @@ -1977,6 +1978,7 @@ networkStartNetwork(struct network_driver *driver,
      case VIR_NETWORK_FORWARD_PRIVATE:
      case VIR_NETWORK_FORWARD_VEPA:
      case VIR_NETWORK_FORWARD_PASSTHROUGH:
 +    case VIR_NETWORK_FORWARD_HOSTDEV:
          ret = networkStartNetworkExternal(driver, network);
          break;
      }
 @@ -2036,6 +2038,7 @@ static int networkShutdownNetwork(struct network_driver *driver,
      case VIR_NETWORK_FORWARD_PRIVATE:
      case VIR_NETWORK_FORWARD_VEPA:
      case VIR_NETWORK_FORWARD_PASSTHROUGH:
 +    case VIR_NETWORK_FORWARD_HOSTDEV:
          ret = networkShutdownNetworkExternal(driver, network);
          break;
      }
 @@ -2825,6 +2828,13 @@ networkCreateInterfacePool(virNetworkDefPtr netdef) {
                  goto finish;
              }
          }
 +        else if (netdef->forwardType == VIR_NETWORK_FORWARD_HOSTDEV) {
 +            netdef->forwardIfs[ii].type = VIR_NETWORK_FORWARD_HOSTDEV_DEVICE_PCI;
/*Assuming PCI as VF's are PCI devices */ 
Long lines are generally discouraged.

...
 +            netdef->forwardIfs[ii].device.pci.domain =
virt_fns[ii]->domain;
 +            netdef->forwardIfs[ii].device.pci.bus = virt_fns[ii]->bus;
 +            netdef->forwardIfs[ii].device.pci.slot = virt_fns[ii]->slot;
 +            netdef->forwardIfs[ii].device.pci.function = virt_fns[ii]->function;
 +        }
      }

      ret = 0;
 @@ -2958,6 +2968,65 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
              }
          }

 +    } else if (netdef->forwardType == VIR_NETWORK_FORWARD_HOSTDEV) {
 +        if (!iface->data.network.actual
 +            && (VIR_ALLOC(iface->data.network.actual) < 0)) {
 +            virReportOOMError();
 +            goto cleanup; 
My patches that created the merge conflicts for you changed the meaning
of cleanup, and added an "error" goto that should now be used for error
exits, so this (and the others below) should be "goto error;"

...
 +        }
 +        
 +        iface->data.network.actual->type = actualType =
VIR_DOMAIN_NET_TYPE_HOSTDEV;
 +        if ((netdef->nForwardPfs > 0) && (netdef->nForwardIfs <= 0))
{
 +            if(networkCreateInterfacePool(netdef) < 0) {
 +                goto cleanup;
 +            }
 +        } 
Unnecessary double nesting - you could put it all in one expression.

...
 +        /* pick first dev with 0 connections */
 +        
 +        for (ii = 0; ii < netdef->nForwardIfs; ii++) {
 +            if (netdef->forwardIfs[ii].connections == 0) {
 +                dev = &netdef->forwardIfs[ii];
 +                break;
 +            }
 +        }
 +        if (!dev) {
 +            virReportError(VIR_ERR_INTERNAL_ERROR,
 +                           _("network '%s' requires exclusive access to
interfaces, but none are available"),
 +                           netdef->name);
 +            goto cleanup;
 +        }
 +        iface->data.network.actual->data.hostdev.def.parent.type =
VIR_DOMAIN_DEVICE_NET;
 +        iface->data.network.actual->data.hostdev.def.parent.data.net = iface;
 +        iface->data.network.actual->data.hostdev.def.info = &iface->info;
 +        iface->data.network.actual->data.hostdev.def.mode =
VIR_DOMAIN_HOSTDEV_MODE_SUBSYS;
 +        iface->data.network.actual->data.hostdev.def.managed =
netdef->managed;
 +        iface->data.network.actual->data.hostdev.def.source.subsys.type =
dev->type;
 +        iface->data.network.actual->data.hostdev.def.source.subsys.u.pci =
dev->device.pci;
 +
 +        /* merge virtualports from interface, network, and portgroup to
 +         * arrive at actual virtualport to use
 +         */
 +        if
(virNetDevVPortProfileMerge3(&iface->data.network.actual->virtPortProfile,
 +                                        iface->virtPortProfile,
 +                                        netdef->virtPortProfile,
 +                                        portgroup
 +                                        ? portgroup->virtPortProfile : NULL) < 0)
{
 +            goto error;
 +        }
 +        virtport = iface->data.network.actual->virtPortProfile;
 +        if (virtport) {
 +            /* make sure type is supported for macvtap connections */
 +            if (virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_8021QBG
&&
 +                virtport->virtPortType != VIR_NETDEV_VPORT_PROFILE_8021QBH) {
 +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 +                               _("<virtualport type='%s'> not
supported for network "
 +                                 "'%s' which uses a macvtap device"),

You didn't change the string when you did the cut-paste (same for the
comment a few lines up)

...
 +                              
virNetDevVPortTypeToString(virtport->virtPortType),
 +                               netdef->name);
 +                goto error;
 +            }
 +        }
 +        
      } else if ((netdef->forwardType == VIR_NETWORK_FORWARD_BRIDGE) ||
                 (netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) ||
                 (netdef->forwardType == VIR_NETWORK_FORWARD_VEPA) ||
 @@ -3124,8 +3193,14 @@ validate:
      if (dev) {
          /* we are now assured of success, so mark the allocation */
          dev->connections++;
 -        VIR_DEBUG("Using physical device %s, %d connections",
 -                  dev->device.dev, dev->connections);
 +        if (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV)
 +            VIR_DEBUG("Using physical device %s, %d connections",
 +                      dev->device.dev, dev->connections);
 +        else
 +            VIR_DEBUG("Using physical device with domain=%d bus=%d slot=%d
function=%d, connections %d",
 +                      dev->device.pci.domain, dev->device.pci.bus,
 +                      dev->device.pci.slot, dev->device.pci.function,
 +                      dev->connections);
      }

      if (netdef) {
 @@ -3164,9 +3239,12 @@ networkNotifyActualDevice(virDomainNetDefPtr iface) 
This function (and also networkReleaseActualDevice) were very badly
affected by all the churn and the merge conflict resolution. It ended up
to be something like this:

    if (typeA)
       Astuff
    else if (typeB)
       Bstuff
    if (typeA)
       Astuff
    else if (typeB)
       Bstuff
    dev->connections++;
    if (typeA)
       Astuff
    else if (typeB)
       Bstuff

This is what made me decide to refactor the those two functions based on
your latest code. I turned it into:

    if (typeA)
      Astuff
      Astuff
      dev->connections++;
      Astuff
    else /* if (typeB) */
      Bstuff
      Bstuff
      dev->connections++;
      Bstuff

(the 2nd if can go into comments because we already determined at the
top of the function that it is either typeA or typeB (i.e. HOSTDEV or
DIRECT)

I'll be posting a proposed replacement patch for this one in a few
minutes. Please try it out as soon as possible. I haven't gone through
7/7 yet, but with the small changes I've squashed in elsewhere, plus the
3.5/7 I posted and the 6/7 refactor I'm about to post, definitely the
first 6 are ready to push.

(By the way, a general note - I've been running "make check" and "make
syntax-check" after applying each of your patches in succession, and
came up with a *lot* of errors (see my add-on patch that I want to
squash into 3/7). In particular, you seem to end up with a lot of lines
that have trailing blanks - if you run "make syntax-check" on each
patch, it will catch those. There are occasional slipups, but in general
every patch is supposed to pass both make check and make syntax-check
(and for a patch series, both of those should complete after applying
each patch, not just after the end of the series).

===================
...
      struct network_driver *driver = driverState;
      virNetworkObjPtr network;
      virNetworkDefPtr netdef;
 -    const char *actualDev;
 +    const char *actualDev = NULL;
 +    virDomainHostdevDefPtr def = NULL;
      int ret = -1;

 +    enum virDomainNetType actualType = virDomainNetGetActualType(iface);
 +
      if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
         return 0;

 @@ -3181,52 +3259,86 @@ networkNotifyActualDevice(virDomainNetDefPtr iface)
      }
      netdef = network->def;

 -    if (!iface->data.network.actual ||
 -        (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_DIRECT)) {
 +    if ((!iface->data.network.actual) ||
 +        ((actualType != VIR_DOMAIN_NET_TYPE_DIRECT) &&
 +         (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV))) {
          VIR_DEBUG("Nothing to claim from network %s",
iface->data.network.name);
          goto success;
      }

 -    actualDev = virDomainNetGetActualDirectDev(iface);
 -    if (!actualDev) {
 -        virReportError(VIR_ERR_INTERNAL_ERROR,
 -                       "%s", _("the interface uses a direct "
 -                               "mode, but has no source dev"));
 -        goto error;
 +    if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
 +        actualDev = virDomainNetGetActualDirectDev(iface);
 +        if (!actualDev) {
 +            virReportError(VIR_ERR_INTERNAL_ERROR,
 +                           "%s", _("the interface uses a direct mode, but
has no source dev"));
 +            goto error;
 +        }
 +    }
 +    else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
 +        def = virDomainNetGetActualHostdev(iface);
 +        if (!def) {
 +            virReportError(VIR_ERR_INTERNAL_ERROR,
 +                           "%s", _("the interface uses a hostdev mode,
but has no hostdev"));
 +            goto error;
 +        }
      }

 -    if (netdef->nForwardIfs == 0) {
 +    if ((netdef->nForwardIfs == 0) && (netdef->nForwardPfs == 0)) {
          virReportError(VIR_ERR_INTERNAL_ERROR,
 -                       _("network '%s' uses a direct mode, but "
 -                         "has no forward dev and no interface pool"),
 +                       _("network '%s' uses a direct/hostdev mode, but has
no forward dev and no interface pool"),
                         netdef->name);
          goto error;
      } else {
          int ii;
          virNetworkForwardIfDefPtr dev = NULL;

 -        /* find the matching interface and increment its connections */
 -
 -        for (ii = 0; ii < netdef->nForwardIfs; ii++) {
 -            if (STREQ(actualDev, netdef->forwardIfs[ii].device.dev)) {
 -                dev = &netdef->forwardIfs[ii];
 -                break;
 +        /* find the matching interface in the pool and increment its connections */
 +        if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
 +            for (ii = 0; ii < netdef->nForwardIfs; ii++) {
 +                if (STREQ(actualDev, netdef->forwardIfs[ii].device.dev)) {
 +                    dev = &netdef->forwardIfs[ii];
 +                    break;
 +                }
              }
 +            if (!dev) {
 +                virReportError(VIR_ERR_INTERNAL_ERROR,
 +                               _("network '%s' doesn't have
dev='%s' in use by domain"),
 +                               netdef->name, actualDev);
 +                goto error;
 +            } 
          }
 -        /* dev points at the physical device we want to use */
 -        if (!dev) {
 -            virReportError(VIR_ERR_INTERNAL_ERROR,
 -                           _("network '%s' doesn't have
dev='%s' in use by domain"),
 -                           netdef->name, actualDev);
 -            goto error;
 +        else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
 +            if ((netdef->nForwardPfs > 0) && (netdef->nForwardIfs <=
0)) {
 +                if(networkCreateInterfacePool(netdef) < 0) {
 +                    goto error;
 +                }
 +            }
 +            for (ii = 0; ii < netdef->nForwardIfs; ii++) {
 +                if (virDevicePCIAddressEqual(def->source.subsys.u.pci,
 +                                             netdef->forwardIfs[ii].device.pci) == 0)
{
 +                    dev = &netdef->forwardIfs[ii];
 +                    break;
 +                }
 +            }
 +            if (!dev) {
 +                virReportError(VIR_ERR_INTERNAL_ERROR,
 +                               _("network '%s' doesn't have
dev='%04x:%02x:%02x.%x' in use by domain"),
 +                               netdef->name, 
 +                               def->source.subsys.u.pci.domain,
 +                               def->source.subsys.u.pci.bus,
 +                               def->source.subsys.u.pci.slot,
 +                               def->source.subsys.u.pci.function);
 +                goto error;
 +            }
          }

 -        /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require
 -         * exclusive access to a device, so current connections count
 -         * must be 0 in those cases.
 -         */
 +       /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require
 +        * exclusive access to a device, so current connections count
 +        * must be 0 in those cases.
 +        */
          if ((dev->connections > 0) &&
              ((netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) ||
 +             (netdef->forwardType == VIR_NETWORK_FORWARD_HOSTDEV) || 
               ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) &&
                iface->data.network.actual->virtPortProfile &&
                (iface->data.network.actual->virtPortProfile->virtPortType
 @@ -3238,8 +3350,16 @@ networkNotifyActualDevice(virDomainNetDefPtr iface)
          }
          /* we are now assured of success, so mark the allocation */
          dev->connections++;
 -        VIR_DEBUG("Using physical device %s, %d connections",
 -                  dev->device.dev, dev->connections);
 +        if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
 +            VIR_DEBUG("Using physical device %s, connections %d",
 +                      dev->device.dev, dev->connections);
 +        }
 +        else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
 +            VIR_DEBUG("Using physical device with domain=%d bus=%d slot=%d
function=%d, connections %d",
 +                      dev->device.pci.domain, dev->device.pci.bus,
 +                      dev->device.pci.slot, dev->device.pci.function,
 +                      dev->connections);
 +        }
      }

  success:
 @@ -3273,9 +3393,12 @@ networkReleaseActualDevice(virDomainNetDefPtr iface)
      struct network_driver *driver = driverState;
      virNetworkObjPtr network;
      virNetworkDefPtr netdef;
 -    const char *actualDev;
 +    const char *actualDev = NULL;
 +    virDomainHostdevDefPtr def = NULL;
      int ret = -1;

 +    enum virDomainNetType actualType = virDomainNetGetActualType(iface);
 +
      if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
         return 0;

 @@ -3290,47 +3413,86 @@ networkReleaseActualDevice(virDomainNetDefPtr iface)
      }
      netdef = network->def;

 -    if (!iface->data.network.actual ||
 -        (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_DIRECT)) {
 +    if ((!iface->data.network.actual) ||
 +        ((actualType != VIR_DOMAIN_NET_TYPE_DIRECT) &&
 +         (actualType != VIR_DOMAIN_NET_TYPE_HOSTDEV))) {
          VIR_DEBUG("Nothing to release to network %s",
iface->data.network.name);
          goto success;
      }

 -    actualDev = virDomainNetGetActualDirectDev(iface);
 -    if (!actualDev) {
 -        virReportError(VIR_ERR_INTERNAL_ERROR,
 -                       "%s", _("the interface uses a direct "
 -                               "mode, but has no source dev"));
 -        goto error;
 +    if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
 +        actualDev = virDomainNetGetActualDirectDev(iface);
 +        if (!actualDev) {
 +            virReportError(VIR_ERR_INTERNAL_ERROR,
 +                           "%s", _("the interface uses a direct mode, but
has no source dev"));
 +            goto error;
 +        }
 +    }
 +    else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
 +        def = virDomainNetGetActualHostdev(iface);
 +        if (!def) {
 +            virReportError(VIR_ERR_INTERNAL_ERROR,
 +                           "%s", _("the interface uses a hostdev mode,
but has no hostdev"));
 +            goto error;
 +        }
      }

 -    if (netdef->nForwardIfs == 0) {
 +    if ((netdef->nForwardIfs == 0) && (netdef->nForwardPfs == 0)) {
          virReportError(VIR_ERR_INTERNAL_ERROR,
 -                       _("network '%s' uses a direct mode, but "
 +                       _("network '%s' uses a direct/hostdev mode, but
"
                           "has no forward dev and no interface pool"),
                         netdef->name);
          goto error;
      } else {
          int ii;
          virNetworkForwardIfDefPtr dev = NULL;
 -
 -        for (ii = 0; ii < netdef->nForwardIfs; ii++) {
 -            if (STREQ(actualDev, netdef->forwardIfs[ii].device.dev)) {
 -                dev = &netdef->forwardIfs[ii];
 -                break;
 +        
 +        if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
 +            for (ii = 0; ii < netdef->nForwardIfs; ii++) {
 +                if (STREQ(actualDev, netdef->forwardIfs[ii].device.dev)) {
 +                    dev = &netdef->forwardIfs[ii];
 +                    break;
 +                }
 +            }
 +            if (!dev) {
 +                virReportError(VIR_ERR_INTERNAL_ERROR,
 +                               _("network '%s' doesn't have
dev='%s' in use by domain"),
 +                               netdef->name, actualDev);
 +                goto error;
 +            } 
 +        }        
 +        else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
 +            for (ii = 0; ii < netdef->nForwardIfs; ii++) {
 +                if (virDevicePCIAddressEqual(def->source.subsys.u.pci,
 +                                             netdef->forwardIfs[ii].device.pci) == 0)
{
 +                    dev = &netdef->forwardIfs[ii];
 +                    break;
 +                }
 +            }
 +            if (!dev) {
 +                virReportError(VIR_ERR_INTERNAL_ERROR,
 +                               _("network '%s' doesn't have
dev='%04x:%02x:%02x.%x' in use by domain"),
 +                               netdef->name, 
 +                               def->source.subsys.u.pci.domain,
 +                               def->source.subsys.u.pci.bus,
 +                               def->source.subsys.u.pci.slot,
 +                               def->source.subsys.u.pci.function);
 +                goto error;
              }
          }
 -        /* dev points at the physical device we've been using */
 -        if (!dev) {
 -            virReportError(VIR_ERR_INTERNAL_ERROR,
 -                           _("network '%s' doesn't have
dev='%s' in use by domain"),
 -                           netdef->name, actualDev);
 -            goto error;
 -        }
 -
          dev->connections--;
 -        VIR_DEBUG("Releasing physical device %s, %d connections",
 -                  dev->device.dev, dev->connections);
 +        if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
 +            VIR_DEBUG("Releasing physical device %s, connections %d",
 +                      dev->device.dev, dev->connections);
 +        }
 +        else if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
 +            VIR_DEBUG("Releasing physical device with domain=%d bus=%d slot=%d
function=%d, connections %d",
 +                      dev->device.pci.domain,
 +                      dev->device.pci.bus,
 +                      dev->device.pci.slot,
 +                      dev->device.pci.function,
 +                      dev->connections);
 +        } 
      }

  success: 

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

Re: [libvirt] [PATCH 6/7] Forward Mode Hostdev network driver Implementation