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

Re: [libvirt] [PATCH 7/9 v2] nodedev: Fix the improper logic when enumerating SRIOV VF



On 15.01.2013 10:56, Osier Yang wrote:
> pciGetVirtualFunctions returns 0 even if there is no "virtfn"
> entry under the device sysfs path.
> 
> And pciGetVirtualFunctions returns -1 when it fails to get
> the PCI config space of one VF, however, with keeping the
> the VFs already detected.
> 
> That's why udevProcessPCI and gather_pci_cap use logic like:
> 
> if (!pciGetVirtualFunctions(syspath,
>                             &data->pci_dev.virtual_functions,
>                             &data->pci_dev.num_virtual_functions) ||
>     data->pci_dev.num_virtual_functions > 0)
>     data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
> 
> to tag the PCI device with "virtual_function" cap.
> 
> However, this results in a VF will aslo get "virtual_function" cap.
> 
> This patch fixes it by:
>   * Ignoring the VF which has failure of getting PCI config space
>     (given that the successfully detected VFs are kept , it makes
>     sense to not give up on the failure of one VF too) with a warning,
>     so pciGetVirtualFunctions will not return -1 except out of memory.
> 
>   * Free the allocated *virtual_functions when out of memory
> 
> And thus the logic can be changed to:
> 
>     /* Out of memory */
>     int ret = pciGetVirtualFunctions(syspath,
>                                      &data->pci_dev.virtual_functions,
>                                      &data->pci_dev.num_virtual_functions);
> 
>     if (ret < 0 )
>         goto out;
>     if (data->pci_dev.num_virtual_functions > 0)
>         data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
> ---
>  src/node_device/node_device_hal.c  |   12 +++++++--
>  src/node_device/node_device_udev.c |   11 ++++++--
>  src/util/virpci.c                  |   46 +++++++++++++++++++++---------------
>  3 files changed, 44 insertions(+), 25 deletions(-)
> 
> diff --git a/src/node_device/node_device_hal.c b/src/node_device/node_device_hal.c
> index e64d6ac..6da5873 100644
> --- a/src/node_device/node_device_hal.c
> +++ b/src/node_device/node_device_hal.c
> @@ -151,9 +151,15 @@ static int gather_pci_cap(LibHalContext *ctx, const char *udi,
>          if (!pciGetPhysicalFunction(sysfs_path, &d->pci_dev.physical_function))
>              d->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
>  
> -        if (!pciGetVirtualFunctions(sysfs_path, &d->pci_dev.virtual_functions,
> -            &d->pci_dev.num_virtual_functions) ||
> -            d->pci_dev.num_virtual_functions > 0)
> +        int ret = pciGetVirtualFunctions(sysfs_path,
> +                                         &d->pci_dev.virtual_functions,
> +                                         &d->pci_dev.num_virtual_functions);
> +        if (ret < 0) {
> +            VIR_FREE(sysfs_path);
> +            return -1;
> +        }
> +
> +        if (d->pci_dev.num_virtual_functions > 0)
>              d->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
>  
>          VIR_FREE(sysfs_path);
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 62f6320..a90217d 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -416,6 +416,7 @@ static int udevProcessPCI(struct udev_device *device,
>      union _virNodeDevCapData *data = &def->caps->data;
>      int ret = -1;
>      char *p;
> +    int rc;
>  
>      syspath = udev_device_get_syspath(device);
>  
> @@ -484,9 +485,13 @@ static int udevProcessPCI(struct udev_device *device,
>      if (!pciGetPhysicalFunction(syspath, &data->pci_dev.physical_function))
>          data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_PHYSICAL_FUNCTION;
>  
> -    if (!pciGetVirtualFunctions(syspath, &data->pci_dev.virtual_functions,
> -        &data->pci_dev.num_virtual_functions) ||
> -        data->pci_dev.num_virtual_functions > 0)
> +    rc = pciGetVirtualFunctions(syspath,
> +                                &data->pci_dev.virtual_functions,
> +                                &data->pci_dev.num_virtual_functions);
> +    /* Out of memory */
> +    if (rc < 0)
> +        goto out;
> +    else if (!rc && (data->pci_dev.num_virtual_functions > 0))
>          data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
>  
>      ret = 0;
> diff --git a/src/util/virpci.c b/src/util/virpci.c
> index 0fb9923..ee4b3a8 100644
> --- a/src/util/virpci.c
> +++ b/src/util/virpci.c
> @@ -1968,6 +1968,7 @@ pciGetVirtualFunctions(const char *sysfs_path,
>                         unsigned int *num_virtual_functions)
>  {
>      int ret = -1;
> +    int i;
>      DIR *dir = NULL;
>      struct dirent *entry = NULL;
>      char *device_link = NULL;
> @@ -1989,45 +1990,52 @@ pciGetVirtualFunctions(const char *sysfs_path,
>      *num_virtual_functions = 0;
>      while ((entry = readdir(dir))) {
>          if (STRPREFIX(entry->d_name, "virtfn")) {
> +            struct pci_config_address *config_addr = NULL;
>  
>              if (virBuildPath(&device_link, sysfs_path, entry->d_name) == -1) {
>                  virReportOOMError();
> -                goto out;
> +                goto error;
>              }
>  
>              VIR_DEBUG("Number of virtual functions: %d",
>                        *num_virtual_functions);
> -            if (VIR_REALLOC_N(*virtual_functions,
> -                (*num_virtual_functions) + 1) != 0) {
> -                virReportOOMError();
> -                VIR_FREE(device_link);
> -                goto out;
> -            }
>  
>              if (pciGetPciConfigAddressFromSysfsDeviceLink(device_link,
> -                &((*virtual_functions)[*num_virtual_functions])) !=
> +                                                          &config_addr) !=
>                  SRIOV_FOUND) {
> -                /* We should not get back SRIOV_NOT_FOUND in this
> -                 * case, so if we do, it's an error. */
> -                virReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("Failed to get SR IOV function from device "
> -                               "link '%s'"), device_link);
> +                VIR_WARN("Failed to get SRIOV function from device "
> +                         "link '%s'", device_link);
>                  VIR_FREE(device_link);
> -                goto out;
> -            } else {
> -                (*num_virtual_functions)++;
> +                continue;
>              }
> +
> +            if (VIR_ALLOC_N(*virtual_functions,
> +                            *num_virtual_functions + 1) < 0) {
> +                virReportOOMError();
> +                VIR_FREE(config_addr);
> +                goto error;
> +            }
> +
> +            (*virtual_functions)[*num_virtual_functions] = config_addr;
> +            (*num_virtual_functions)++;
>              VIR_FREE(device_link);
>          }
>      }
>  
>      ret = 0;
> -
> -out:
> +cleanup:
> +    VIR_FREE(device_link);
>      if (dir)
>          closedir(dir);
> -
>      return ret;
> +
> +error:
> +    if (*virtual_functions) {
> +        for (i = 0; i < *num_virtual_functions; i++)
> +            VIR_FREE((*virtual_functions)[i]);
> +        VIR_FREE(*virtual_functions);
> +    }
> +    goto cleanup;
>  }
>  
>  /*
> 

ACK to this and te follow up *_udev.c patch squashed in.

Michal


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