[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 2013年01月15日 17: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;
  }

  /*

Eh, changes on *_udev.c was lost when committing. With the following
diff squashed in:


diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index a90217d..d8637a2 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -491,7 +491,8 @@ static int udevProcessPCI(struct udev_device *device,
     /* Out of memory */
     if (rc < 0)
         goto out;
-    else if (!rc && (data->pci_dev.num_virtual_functions > 0))
+
+    if (data->pci_dev.num_virtual_functions > 0)
         data->pci_dev.flags |= VIR_NODE_DEV_CAP_FLAG_PCI_VIRTUAL_FUNCTION;
 
     ret = 0;

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