Get device/vendor ids of SF from its parent PCI #5964

leibin2014 · 2020-11-28T12:54:07Z

What

Get device/vendor ids of SF from its parent PCI.

Why ?

We can't get device/vendor ids of SF from its sysfs directly, so we get the ids from SF's parent PCI.

How ?

Get the parent PCI of SF, then get the device/vendor ids of parent PCI. The SF has the same device/vendor ids as its parent PCI.

leibin2014 · 2020-12-01T06:50:53Z

/azp run

azure-pipelines · 2020-12-01T06:51:06Z

Azure Pipelines successfully started running 1 pipeline(s).

src/uct/ib/base/ib_device.c

yosefe · 2020-12-04T15:54:47Z

src/uct/ib/base/ib_device.h

@@ -56,6 +56,9 @@
 #define UCT_IB_DEVICE_SYSFS_GID_TYPE_FMT  UCT_IB_DEVICE_SYSFS_GID_ATTR_PFX "/types/%d"
 #define UCT_IB_DEVICE_SYSFS_GID_NDEV_FMT  UCT_IB_DEVICE_SYSFS_GID_ATTR_PFX "/ndevs/%d"

+#define UCT_IB_PCI_BUS_ID_LEN             7   /* PCI BUS ID */


maybe use NAME_MAX constant?

@yosefe We need the real length of 0000:03 which is 7 to memcpy 0000:03 from 0000:03:00.0.

@yosefe Or is it better to use something like strstr(":") to get the length of 0000:03 then we don't need to define monstant here?

better parse it to components

@yosefe Maybe no need to do this because the pci bus id and format is pretty fixed. No need to add more code and logic to parse it.

@yosefe Fixed.

yosefe · 2020-12-04T16:12:41Z

bot:pipe:retest

yosefe · 2020-12-14T16:18:24Z

src/uct/ib/base/ib_device.c

+    /* get 0000:03 from 0000:03:00.0*/
+    memcpy(pci_bus, bus_id, UCT_IB_PCI_BUS_ID_LEN);
+    pci_bus[UCT_IB_PCI_BUS_ID_LEN] = 0;


can we unite common code with uct_ib_device_get_sys_dev ?

@yosefe This is not common code with uct_ib_device_get_sys_dev. uct_ib_device_get_sys_dev doesn't need to use 0000:03. It only needs 0000:03:00.0.

yosefe · 2020-12-14T16:23:14Z

src/uct/ib/base/ib_device.c

+    status = ucs_read_file_number(&vendor_id, 1, UCT_IB_DEVICE_SYSFS_PCI_BUS_FMT,
+                                  pci_bus, bus_id, "vendor");
+    if (status != UCS_OK) {
+        return status;
+    }
+
+    /* /sys/class/pci_bus/0000:03/device/0000:03:00.0/device */
+    status = ucs_read_file_number(&device_id, 1, UCT_IB_DEVICE_SYSFS_PCI_BUS_FMT,
+                                  pci_bus, bus_id, "device");
+    if (status != UCS_OK) {


can we unite common code with 549-552?

@yosefe Tried, but seems not simply to do it. The SYSFS path format is much different for both places, the contents and path layers are different. Need much more code to deal with the diffrence.

@yosefe Fixed.

yosefe · 2020-12-14T16:23:45Z

src/uct/ib/base/ib_device.c

+        return status;
+    }
+
+    bus_id = basename(dirname(dirname(dirname(path_buffer))));


check return values from dirname

@yosefe Fixed.

yosefe · 2020-12-14T16:24:07Z

src/uct/ib/base/ib_device.h

@@ -56,6 +56,9 @@
 #define UCT_IB_DEVICE_SYSFS_GID_TYPE_FMT  UCT_IB_DEVICE_SYSFS_GID_ATTR_PFX "/types/%d"
 #define UCT_IB_DEVICE_SYSFS_GID_NDEV_FMT  UCT_IB_DEVICE_SYSFS_GID_ATTR_PFX "/ndevs/%d"

+#define UCT_IB_PCI_BUS_ID_LEN             7   /* PCI BUS ID */


better parse it to components

leibin2014 · 2020-12-16T02:39:05Z

/azp run

azure-pipelines · 2020-12-16T02:39:18Z

Azure Pipelines successfully started running 1 pipeline(s).

leibin2014 · 2020-12-16T09:12:34Z

bot:pipe:retest

leibin2014 · 2020-12-16T14:11:22Z

bot:pipe:retest

leibin2014 · 2020-12-28T02:12:25Z

/azp run

azure-pipelines · 2020-12-28T02:12:38Z

Azure Pipelines successfully started running 1 pipeline(s).

leibin2014 · 2020-12-29T07:17:52Z

/azp run

azure-pipelines · 2020-12-29T07:18:04Z

Azure Pipelines successfully started running 1 pipeline(s).

leibin2014 · 2020-12-30T06:49:18Z

/azp run

azure-pipelines · 2020-12-30T06:49:30Z

Azure Pipelines successfully started running 1 pipeline(s).

yosefe · 2020-12-30T12:25:51Z

src/uct/ib/base/ib_device.c

+    return UCS_OK;
+}
+
+char* uct_ib_device_get_pcie_bus(char *path_buffer, uint8_t layers)


add function to UCS:
ucs_dirname(chat *dir_path, int num_layers)
pls add doxygen and small gtest for this function

@yosefe Done.

yosefe · 2020-12-30T12:26:34Z

src/uct/ib/base/ib_device.c

+    return basename(path_buffer);
+}
+
+static ucs_status_t uct_ib_device_get_ids_from_path(char *path,


const char *path

@yosefe Fixed.

yosefe · 2020-12-30T12:46:03Z

src/uct/ib/base/ib_device.c

+        return status;
+    }
+
+    bus_id = uct_ib_device_get_pcie_bus(path_buffer, 3);


seems like 'bus_id' here, after stripping 3 directory components, already points to the SF

$ cat $(realpath /sys/class/infiniband/mlx5_0)/../../../vendor 0x15b3 $ cat $(realpath /sys/class/infiniband/mlx5_0)/../../../device 0xa2d6 $ cat $(realpath /sys/class/infiniband/mlx5_0)/../../../current_link_speed 16 GT/s

@yosefe Fixed.

leibin2014 · 2021-01-05T02:15:46Z

bot:pipe:retest

yosefe · 2021-01-05T10:39:50Z

src/ucs/sys/string.c

@@ -199,6 +200,17 @@ ucs_status_t ucs_str_to_memunits(const char *buf, void *dest)
    return UCS_OK;
 }

+char *ucs_dirname(char *path, int num_layers)
+{
+    while (num_layers--) {


@yosefe Should not have problem when num_layers==0 since "--" is after num_layers. When num_layers==0, won't go into while loop, will only return path.

sorry, github re-formatted my comment
i've meant:
while (num_layers-- > 0)

@yosefe Fixed.

yosefe · 2021-01-05T10:41:47Z

src/uct/ib/base/ib_device.c

+        for (i = 0; i < 2; i++) {
+            ids_path = ucs_dirname(path_buffer, nums_layers[i]);


call ucs_dirname twice, without loop
also, need to reset path_buffer before 2nd call because ucs_dirname may change it

Fixed. You detected a bug which caused by my fixing on PF but didn't retest on SF. Thanks!

yosefe · 2021-01-05T10:42:36Z

src/uct/ib/base/ib_device.c

+        for (i = 0; i < 2; i++) {
+            ids_path = ucs_dirname(path_buffer, nums_layers[i]);
+            if (ids_path != NULL) {
+                if (uct_ib_device_get_ids_from_path(ids_path, &vendor_id, &device_id) == UCS_OK) {


maybe pass &dev->pci_id.vendor and &dev->pci_id.device?

@yosefe Can't pass them directly since the variable types are different with the function parameters. Have changed to another way.

yosefe · 2021-01-05T10:43:00Z

src/uct/ib/base/ib_device.c

+    pcie_bus = ucs_dirname(path_buffer, 2);
+    if (pcie_bus == NULL) {
+        return UCS_SYS_DEVICE_ID_UNKNOWN;
+    }


space line after

@yosefe Fixed.

leibin2014 · 2021-01-06T04:40:33Z

/azp run

azure-pipelines · 2021-01-06T04:40:45Z

Azure Pipelines successfully started running 1 pipeline(s).

leibin2014 · 2021-01-06T06:33:58Z

/azp run

azure-pipelines · 2021-01-06T06:34:11Z

Azure Pipelines successfully started running 1 pipeline(s).

yosefe · 2021-01-06T09:10:22Z

src/uct/ib/base/ib_device.c

+    /* SF: realpath name is of form /sys/devices/.../0000:03:00.0/<UUID>/infiniband/mlx5_0 */
+
+    status = uct_ib_device_get_path_buffer(dev, path_buffer);
+    if (status == UCS_OK) {


if (status != UCS_OK) { goto not_found; } ... not_found: dev->pci_id.vendor = 0; ...

@yosefe Done.

yosefe · 2021-01-06T09:13:10Z

src/uct/ib/base/ib_device.c

+    if (status == UCS_OK) {
+        /* PF: strip 2 layers. */
+        ids_path = ucs_dirname(path_buffer, 2);
+        if (ids_path != NULL) {


if (ids_path == NULL) { goto not_found; }

@yosefe Fixed.

yosefe · 2021-01-06T09:13:56Z

src/uct/ib/base/ib_device.c

+        /* PF: strip 2 layers. */
+        ids_path = ucs_dirname(path_buffer, 2);
+        if (ids_path != NULL) {
+            if (uct_ib_device_get_ids_from_path(ids_path,


status = uct_ib_device_get_ids_from_path(...) if (status == UCS_OK) { .... return; } /* SF: strip 3 layers (1 more layer than PF). */ ids_path = ucs_dirname(path_buffer, 1); ...

@yosefe Fixed.

yosefe · 2021-01-06T09:49:47Z

src/uct/ib/base/ib_device.c

+    if (status == UCS_OK) {
+        ucs_debug("SF: %s vendor_id: 0x%x device_id: %d", uct_ib_device_name(dev),
+                  dev->pci_id.vendor, dev->pci_id.device);
+            return;


pls fix indent

@yosefe Fixed.

yosefe · 2021-01-06T10:16:08Z

src/uct/ib/base/ib_device.h

+#define UCT_IB_DEVICE_SYSFS_PCI_PFX       "/sys/class/pci_bus/%s"
+#define UCT_IB_DEVICE_SYSFS_PCI_PATH_FMT  UCT_IB_DEVICE_SYSFS_PCI_PFX "/device/%s"


seems unused now?

Yes, according latest implementation they are unused. Removed. Thanks!

yosefe · 2021-01-06T10:16:17Z

src/uct/ib/base/ib_device.h

@@ -51,10 +51,13 @@
 #define UCT_IB_DEFAULT_ROCEV2_DSCP        106  /* Default DSCP for RoCE v2 */
 #define UCT_IB_ROCE_UDP_SRC_PORT_BASE     0xC000
 #define UCT_IB_DEVICE_SYSFS_PFX           "/sys/class/infiniband/%s"
+#define UCT_IB_DEVICE_SYSFS_PATH_FMT      UCT_IB_DEVICE_SYSFS_PFX "/device"


@yosefe Removed. Thanks!

yosefe

pls squash

leibin2014 · 2021-01-06T11:32:40Z

pls squash

Done.

leibin2014 · 2021-01-06T15:32:40Z

/azp run

azure-pipelines · 2021-01-06T15:32:54Z

Azure Pipelines successfully started running 1 pipeline(s).

yosefe reviewed Dec 4, 2020

View reviewed changes

yosefe reviewed Dec 14, 2020

View reviewed changes

leibin2014 force-pushed the bf2 branch 2 times, most recently from bb4245e to abcf45e Compare December 26, 2020 14:43

yosefe reviewed Dec 30, 2020

View reviewed changes

yosefe reviewed Jan 5, 2021

View reviewed changes

yosefe reviewed Jan 6, 2021

View reviewed changes

yosefe approved these changes Jan 6, 2021

View reviewed changes

yosefe requested changes Jan 6, 2021

View reviewed changes

yosefe approved these changes Jan 6, 2021

View reviewed changes

UCS/UCT/IB: Get device/vendor id of SF from its parent PCI

677607a

leibin2014 force-pushed the bf2 branch from 9b403cb to 677607a Compare January 6, 2021 11:31

yosefe merged commit e843d3e into openucx:master Jan 7, 2021

		for (i = 0; i < 2; i++) {
		ids_path = ucs_dirname(path_buffer, nums_layers[i]);

		#define UCT_IB_DEVICE_SYSFS_PCI_PFX "/sys/class/pci_bus/%s"
		#define UCT_IB_DEVICE_SYSFS_PCI_PATH_FMT UCT_IB_DEVICE_SYSFS_PCI_PFX "/device/%s"

Get device/vendor ids of SF from its parent PCI #5964

Get device/vendor ids of SF from its parent PCI #5964

Conversation

leibin2014 commented Nov 28, 2020

What

Why ?

How ?

leibin2014 commented Dec 1, 2020

azure-pipelines bot commented Dec 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosefe commented Dec 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leibin2014 commented Dec 16, 2020

azure-pipelines bot commented Dec 16, 2020

leibin2014 commented Dec 16, 2020

leibin2014 commented Dec 16, 2020

leibin2014 commented Dec 28, 2020

azure-pipelines bot commented Dec 28, 2020

leibin2014 commented Dec 29, 2020

azure-pipelines bot commented Dec 29, 2020

leibin2014 commented Dec 30, 2020

azure-pipelines bot commented Dec 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leibin2014 commented Jan 5, 2021

yosefe Jan 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosefe Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leibin2014 commented Jan 6, 2021

azure-pipelines bot commented Jan 6, 2021

leibin2014 commented Jan 6, 2021

azure-pipelines bot commented Jan 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosefe left a comment

Choose a reason for hiding this comment

leibin2014 commented Jan 6, 2021

leibin2014 commented Jan 6, 2021

azure-pipelines bot commented Jan 6, 2021

yosefe Jan 5, 2021 •

edited

Loading

yosefe Jan 6, 2021 •

edited

Loading