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

Re: [libvirt] [PATCH 4/5 v3] Functionality to implicitly get interface pool from SR-IOV PF.



On Wed, Dec 14, 2011 at 10:50:30AM +0000, Shradha Shah wrote:
> If a system has 64 or more VF's, it is quite tedious to mention each VF
> in the interface pool.
> The following modification will implicitly create an interface pool from
> the SR-IOV PF.
> ---
>  src/network/bridge_driver.c |   93 +++++++++++++++++++++++++++++++++---------
>  1 files changed, 73 insertions(+), 20 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 63338a2..9ae9c50 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -2817,15 +2817,19 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
>          /* If there is only a single device, just return it (caller will detect
>           * any error if exclusive use is required but could not be acquired).
>           */
> -        if (netdef->nForwardIfs == 0) {
> +        if ((netdef->nForwardIfs <= 0) && (netdef->nForwardPfs <= 0)) { 
>              networkReportError(VIR_ERR_INTERNAL_ERROR,
>                                 _("network '%s' uses a direct mode, but has no forward dev and no interface pool"),
>                                 netdef->name);
>              goto cleanup;
> -        } else {
> +        } 
> +        
> +        else {

Minor style issue - can you keep the '} else {' alignment.

>              int ii;
>              virNetworkForwardIfDefPtr dev = NULL;
> -
> +            unsigned int num_virt_fns = 0;
> +            char **vfname = NULL;
> +            
>              /* pick an interface from the pool */
>  
>              /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require
> @@ -2833,20 +2837,67 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
>               * 0.  Other modes can share, so just search for the one with
>               * the lowest usageCount.
>               */
> -            if ((netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) ||
> -                ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) &&
> -                 iface->data.network.actual->data.direct.virtPortProfile &&
> -                 (iface->data.network.actual->data.direct.virtPortProfile->virtPortType
> -                  == VIR_NETDEV_VPORT_PROFILE_8021QBH))) {
> +            if (netdef->forwardType == VIR_NETWORK_FORWARD_PASSTHROUGH) {         
> +                if ((netdef->nForwardPfs > 0) && (netdef->nForwardIfs <= 0)) {
> +                    if ((virNetDevGetVirtualFunctions(netdef->forwardPfs->dev, 
> +                                                      &vfname, &num_virt_fns)) < 0){
> +                        networkReportError(VIR_ERR_INTERNAL_ERROR,
> +                                           _("Could not get Virtual functions on %s"),
> +                                           netdef->forwardPfs->dev);
> +                        goto out;
> +                    }
> +                    
> +                    if (num_virt_fns == 0) {
> +                        networkReportError(VIR_ERR_INTERNAL_ERROR,
> +                                           _("No Vf's present on SRIOV PF %s"),
> +                                           netdef->forwardPfs->dev);
> +                        goto out;
> +                    }
> +                    
> +                    if ((VIR_ALLOC_N(netdef->forwardIfs, num_virt_fns)) < 0) {
> +                        virReportOOMError();
> +                        goto out;
> +                    }
> +                    
> +                    netdef->nForwardIfs = num_virt_fns;
> +                    
> +                    for (ii = 0; ii < netdef->nForwardIfs; ii++) {
> +                        netdef->forwardIfs[ii].dev = strdup(vfname[ii]);

Missing check for OOM here

> +                        netdef->forwardIfs[ii].usageCount = 0;
> +                    }
> +                    
> +                out:
> +                    for (ii = 0; ii < num_virt_fns; ii++) {
> +                        VIR_FREE(vfname[ii]);
> +                    }
> +                    VIR_FREE(vfname);
> +                }
> +                
>                  /* pick first dev with 0 usageCount */
> -
> +                
>                  for (ii = 0; ii < netdef->nForwardIfs; ii++) {
>                      if (netdef->forwardIfs[ii].usageCount == 0) {
>                          dev = &netdef->forwardIfs[ii];
>                          break;
>                      }
>                  }
> -            } else {
> +            }
> +            else if ((netdef->forwardType == VIR_NETWORK_FORWARD_PRIVATE) &&
> +                     iface->data.network.actual->data.direct.virtPortProfile &&
> +                     (iface->data.network.actual->data.direct.virtPortProfile->virtPortType
> +                      == VIR_NETDEV_VPORT_PROFILE_8021QBH)) {

Also  '} else if '

> +                
> +                
> +                /* pick first dev with 0 usageCount */
> +                
> +                for (ii = 0; ii < netdef->nForwardIfs; ii++) {
> +                    if (netdef->forwardIfs[ii].usageCount == 0) {
> +                        dev = &netdef->forwardIfs[ii];
> +                        break;
> +                    }
> +                }
> +            } 
> +            else {

And '} else {'

>                  /* pick least used dev */
>                  dev = &netdef->forwardIfs[0];
>                  for (ii = 1; ii < netdef->nForwardIfs; ii++) {
> @@ -2858,7 +2909,7 @@ networkAllocateActualDevice(virDomainNetDefPtr iface)
>              if (!dev) {
>                  networkReportError(VIR_ERR_INTERNAL_ERROR,
>                                     _("network '%s' requires exclusive access to interfaces, but none are available"),
> -                               netdef->name);
> +                                   netdef->name);
>                  goto cleanup;
>              }
>              iface->data.network.actual->data.direct.linkdev = strdup(dev->dev);


>From this point onwards, AFAICT,  none of the patch hunks have
any functional change. They're all just whitespace changes, or
moving variable declarations. So I think the rest of this patch
can just be dropped.

> @@ -2901,6 +2952,7 @@ networkNotifyActualDevice(virDomainNetDefPtr iface)
>      virNetworkObjPtr network;
>      virNetworkDefPtr netdef;
>      const char *actualDev;
> +    virNetworkForwardIfDefPtr dev = NULL;
>      int ret = -1;
>  
>      if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> @@ -2935,12 +2987,12 @@ networkNotifyActualDevice(virDomainNetDefPtr iface)
>                             _("network '%s' uses a direct mode, but has no forward dev and no interface pool"),
>                             netdef->name);
>          goto cleanup;
> -    } else {
> +    } 
> +    else {

Change back to '} else {'

>          int ii;
> -        virNetworkForwardIfDefPtr dev = NULL;
> -
> +        
>          /* find the matching interface in the pool and increment its usageCount */
> -
> +        
>          for (ii = 0; ii < netdef->nForwardIfs; ii++) {
>              if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) {
>                  dev = &netdef->forwardIfs[ii];
> @@ -2954,7 +3006,7 @@ networkNotifyActualDevice(virDomainNetDefPtr iface)
>                                 netdef->name, actualDev);
>              goto cleanup;
>          }
> -
> +        
>          /* PASSTHROUGH mode, and PRIVATE Mode + 802.1Qbh both require
>           * exclusive access to a device, so current usageCount must be
>           * 0 in those cases.
> @@ -3001,6 +3053,7 @@ networkReleaseActualDevice(virDomainNetDefPtr iface)
>      virNetworkObjPtr network = NULL;
>      virNetworkDefPtr netdef;
>      const char *actualDev;
> +    virNetworkForwardIfDefPtr dev = NULL;
>      int ret = -1;
>  
>      if (iface->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> @@ -3036,10 +3089,10 @@ networkReleaseActualDevice(virDomainNetDefPtr iface)
>                             _("network '%s' uses a direct mode, but has no forward dev and no interface pool"),
>                             netdef->name);
>          goto cleanup;
> -    } else {
> +    } 
> +    else {

And here

>          int ii;
> -        virNetworkForwardIfDefPtr dev = NULL;
> -
> +    
>          for (ii = 0; ii < netdef->nForwardIfs; ii++) {
>              if (STREQ(actualDev, netdef->forwardIfs[ii].dev)) {
>                  dev = &netdef->forwardIfs[ii];
> @@ -3053,7 +3106,7 @@ networkReleaseActualDevice(virDomainNetDefPtr iface)
>                                 netdef->name, actualDev);
>              goto cleanup;
>          }
> -
> +        
>          dev->usageCount--;
>          VIR_DEBUG("Releasing physical device %s, usageCount %d",
>                    dev->dev, dev->usageCount);

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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