Repository navigation
UCS/TOPO: Build device inventory - #11869
Conversation
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
| @@ -65,10 +44,7 @@ UCS_ARRAY_DECLARE_TYPE(ucs_topo_group_array_t, size_t, ucs_topo_group_t); | |||
| * @ingroup UCS_RESOURCE | |||
There was a problem hiding this comment.
pls document what sys_devs holds — the grouping rule differs per class (ucs_topo_groups_bus_id_match() uses full BDF for gpus, bdf-excluding-function for nics), so for a gpu element these are the sys_dev aliases of one BDF and for a nic element the functions/ports of one slot. Today that semantic is only visible in the .c file.
| * @param [in] sys_dev2 Second system device. | ||
| * | ||
| * @return A negative value if @a sys_dev1 precedes @a sys_dev2, a positive | ||
| * value if @a sys_dev2 precedes @a sys_dev1, or zero if equal. |
There was a problem hiding this comment.
minor: with this PR there are three identical ucs_qsort_r adapters around the comparator (ucp_proto_multi_sys_dev_cmp, topo_test_sys_device_cmp, and ucs_topo_groups_sys_dev_cmp). Can we also export a ucs_qsort_r-shaped ucs_topo_sys_device_qsort_cmp(const void*, const void*, void*) here, so the ucp and gtest wrappers can be removed?
|
minor: the function builds group elements out of the collected devices, so
minor: this static wrapper is a single Residual coverage: |
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
minor: this line is 81 columns and the continuation lines are indented one column short of the open paren (docs/CodeStyle.md: up to 80 columns, indent function arguments on column). The two call sites in
minor: nothing in the new test uses Not re-raised as a comment (already covered in earlier threads): |
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
| return ucs_topo_get_bus_id_bit_repr(&bus_id); | ||
| } | ||
|
|
||
| static int ucp_proto_multi_sys_dev_cmp(const void *pa, const void *pb, |
There was a problem hiding this comment.
ucp_proto_multi_sys_dev_cmp here, ucs_topo_groups_sys_dev_cmp in topo_groups.c, and topo_test_sys_device_cmp in test_topo.cc are the same qsort_r trampoline. Can we export a qsort_r-compatible comparator from ucs/sys/topo/base/topo.h and drop this one and the gtest copy?
| ucs_topo_groups_type_t type; | ||
| ucs_topo_group_array_t groups; | ||
| } ucs_topo_groups_t; | ||
| typedef ucs_topo_group_array_t ucs_topo_groups_t; |
There was a problem hiding this comment.
minor: ucs_topo_group_array_t is now used only by this typedef — can we name the array type ucs_topo_groups_t directly in the UCS_ARRAY_DECLARE_TYPE above and drop this line?
| ucs_array_cleanup_dynamic(&group->gpus); | ||
| } | ||
|
|
||
| static ucs_status_t |
There was a problem hiding this comment.
minor: inventory is a ucs_topo_group_t, but it holds every GPU/NIC in the system, while the type is documented as "Group of GPUs and NICs sharing a topology locality". Can we add a one-line comment that the inventory is the pre-split single-group form?
|
Residual coverage: |
| sys_dev = ucs_array_elem(nics, src); | ||
| device = &devices[sys_dev]; | ||
|
|
||
| ucs_debug("cx9_filter: processing network device " UCS_SYS_BUS_ID_FMT |
There was a problem hiding this comment.
remove log indent's below, move this ucs_debug() at line of ucs_log_indent(-1) and add boolean as a string "added" / "skipped" or similar also add sys_dev and maybe index src
There was a problem hiding this comment.
I don't think we want this, the goal of printing this before the indent is that we can see which device is related to the logs from ucs_topo_groups_is_nic_cx9
There was a problem hiding this comment.
Updated the log message to include sys_dev
There was a problem hiding this comment.
something like below would log only one line always with context/one line, could help when there is another is_cx9 user to avoid print logs without contenxt / make logs more concise.
static int
ucs_topo_groups_is_nic_cx9(const ucs_topo_sys_device_info_t *device,
ucs_sys_device_t sys_dev)
{
const ucs_sys_pci_id_t *pci_id = &device->pci_id;
char fw_ver[UCS_TOPO_GROUPS_FW_VER_MAX];
const char *msg;
ucs_status_t status;
int is_cx9 = 0;
if (pci_id->vendor != UCS_TOPO_GROUPS_MELLANOX_VENDOR_ID) {
msg = "vendor mismatch, skipped";
} else if (pci_id->device == UCS_TOPO_GROUPS_CX9_DEVICE_ID) {
msg = "CX9 device ID, kept";
is_cx9 = 1;
} else if (pci_id->device != UCS_TOPO_GROUPS_MLX5_VF_DEVICE_ID) {
msg = "device ID mismatch, skipped";
} else {
status = ucs_topo_groups_read_ib_fw_ver(
&device->bus_id, fw_ver, sizeof(fw_ver));
if (status != UCS_OK) {
msg = "firmware read failed, skipped";
} else if (strncmp(fw_ver, "82.", 3) != 0) {
msg = "firmware version mismatch, skipped";
} else {
msg = "CX9 firmware version, kept";
is_cx9 = 1;
}
}
ucs_debug("cx9_filter: sys_dev=%u, bus_id=" UCS_SYS_BUS_ID_FMT
", pci_id=" UCS_SYS_PCI_ID_FMT ": %s",
sys_dev, UCS_SYS_BUS_ID_ARG(&device->bus_id),
UCS_SYS_PCI_ID_ARG(pci_id), msg);
return is_cx9;
}| } | ||
|
|
||
| status = ucs_topo_groups_elements_build(devices, &net_devices, | ||
| UCS_TOPO_DEVICE_CLASS_NET, |
There was a problem hiding this comment.
replace this third parameter by function pointer ucs_topo_groups_bus_id_same_slot and remove assert/dispatch function
There was a problem hiding this comment.
I don't think it's needed, is there a clear advantage?
There was a problem hiding this comment.
it will remove the dispatch function using it
| } | ||
|
|
||
| status = ucs_topo_groups_elements_build(devices, &acc_devices, | ||
| UCS_TOPO_DEVICE_CLASS_ACC, |
There was a problem hiding this comment.
replace this third parameter by function pointer ucs_topo_groups_bus_id_equal and remove assert/dispatch function
There was a problem hiding this comment.
it will remove the dispatch function using it
There was a problem hiding this comment.
src/ucs/sys/topo/base/topo_groups.c | 25 +++++--------------------
1 file changed, 5 insertions(+), 20 deletions(-)
diff --git a/src/ucs/sys/topo/base/topo_groups.c b/src/ucs/sys/topo/base/topo_groups.c
--- a/src/ucs/sys/topo/base/topo_groups.c
+++ b/src/ucs/sys/topo/base/topo_groups.c
@@
-static int ucs_topo_groups_bus_id_match(ucs_topo_device_class_t device_class,
- const ucs_sys_bus_id_t *bus_id1,
- const ucs_sys_bus_id_t *bus_id2)
-{
- if (device_class == UCS_TOPO_DEVICE_CLASS_ACC) {
- /* Accelerator devices (GPUs) are grouped by full bus id equality. */
- return ucs_topo_groups_bus_id_equal(bus_id1, bus_id2);
- }
-
- /* Network devices (NICs) are grouped by bdf equality excl. the function. */
- ucs_assert(device_class == UCS_TOPO_DEVICE_CLASS_NET);
- return ucs_topo_groups_bus_id_same_slot(bus_id1, bus_id2);
-}
-
static ucs_status_t ucs_topo_groups_elements_build(
const ucs_topo_sys_device_info_t *devices,
const ucs_topo_groups_sys_dev_array_t *sys_devices,
- const ucs_topo_device_class_t device_class,
+ int (*bus_id_match)(const ucs_sys_bus_id_t *bus_id1,
+ const ucs_sys_bus_id_t *bus_id2),
ucs_topo_group_element_array_t *elements)
{
@@
- ucs_assert((device_class == UCS_TOPO_DEVICE_CLASS_ACC) ||
- (device_class == UCS_TOPO_DEVICE_CLASS_NET));
ucs_assert(ucs_array_begin(sys_devices) != NULL);
@@
- if ((prev_bus_id == NULL) ||
- !ucs_topo_groups_bus_id_match(device_class, prev_bus_id, bus_id)) {
+ if ((prev_bus_id == NULL) || !bus_id_match(prev_bus_id, bus_id)) {
/* No match, add a new element. */
element = ucs_array_append(elements, return UCS_ERR_NO_MEMORY);
@@
status = ucs_topo_groups_elements_build(devices, &acc_devices,
- UCS_TOPO_DEVICE_CLASS_ACC,
+ ucs_topo_groups_bus_id_equal,
&inventory.gpus);
@@
status = ucs_topo_groups_elements_build(devices, &net_devices,
- UCS_TOPO_DEVICE_CLASS_NET,
+ ucs_topo_groups_bus_id_same_slot,
&inventory.nics);| if (ucs_topo_groups_is_nic_cx9(device)) { | ||
| ucs_array_elem(nics, dst++) = sys_dev; | ||
| } else { | ||
| ucs_debug("Network device is not CX9, skipping"); |
| UCS_TOPO_GROUPS_TYPE_UNKNOWN; | ||
| } | ||
|
|
||
| ucs_status_t ucs_topo_build_groups(ucs_topo_groups_t *groups_p) |
There was a problem hiding this comment.
Residual coverage: ucs_topo_build_groups() still has no production caller and no gtest, so the collect/sort/alias-filter/CX9-fw_ver/element-grouping code added here is not exercised by CI, and ucs_topo_groups_build_groups() is still a stub that discards the inventory.
bot commented like above, could we add test to this pr addressing it and/or remove untested code? if it is from another pr could we maybe add some of them here?
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
| ucs_topo_groups_gpu_aliases_filter(devices, &acc_devices); | ||
|
|
||
| if (is_vera_rubin) { | ||
| ucs_topo_groups_nics_cx9_filter(devices, &net_devices); |
There was a problem hiding this comment.
Previously reported, still present: fw_ver sysfs I/O (opendir/readdir/ucs_read_file_str) plus ucs_malloc and ucs_debug run under ucs_topo_global_ctx.lock, held by ucs_topo_build_groups() → ..._inner → inventory_build → nics_cx9_filter → read_ib_fw_ver (see also topo.c L1417–1421 and topo_groups.c L201).
|
|
||
| groups_p->type = groups_type; | ||
| ucs_array_init_dynamic(&groups_p->groups); | ||
| status = ucs_topo_groups_inventory_build(devices, num_devices, 1, |
There was a problem hiding this comment.
Previously reported, still present: is_vera_rubin is hardcoded to 1 at its single call site.
| { | ||
| const ucs_sys_bus_id_t *bus_id, *prev_bus_id; | ||
| const ucs_sys_device_t *sys_dev; | ||
| ucs_topo_group_element_t *element; |
There was a problem hiding this comment.
Previously reported, still present: element is declared uninitialized in ucs_topo_groups_elements_build and dereferenced at L325 outside the assigning branch (-Wmaybe-uninitialized risk with -Werror).
|
minor:
Previously reported, still present: out-of-bounds read in
Previously reported, still present: CX9 vendor/device-id and mlx5-VF
Previously reported, still present:
Previously reported, still present: returning
Previously reported, still present:
Previously reported, still present: Residual coverage: |
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The "Basic compile" job failed with Full analysisSummary: Azure Pipelines "Basic compile" stage of PR #11869 failed compiling Root cause: This is a real logic bug, not just a warning — the debug message would print a garbage/stale pointer. Implicated commit: File: Suggested fix: Make static int ucs_topo_groups_is_nic_cx9(const ucs_topo_sys_device_info_t *device,
const char **reason_p)
{
...
if (pci_id->vendor != UCS_TOPO_GROUPS_MELLANOX_VENDOR_ID) {
*reason_p = "not a mellanox device";
return 0;
}
/* ... same for every other branch ... */
*reason_p = "cx9 device by firmware version";
return 1;
}and at the call site (line 241): is_cx9 = ucs_topo_groups_is_nic_cx9(device, &reason);Also initialize Related: PR #11869 (openucx/ucx) — this is a self-contained defect in the PR itself, no pre-existing issue found.
|
|
blocker: in Residual coverage: |
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
minor: pls drop the
minor: after this move, both
Not re-raised (already in open threads, still present in this revision):
Not re-raised (already in open threads, still present): fw_ver sysfs I/O (
Not re-raised (already in open threads, still present): CX9 vendor/device-id + mlx5-VF
Not re-raised (already in open threads, still present):
Not re-raised (already in open threads, still present): out-of-bounds read in
Not re-raised (already in open threads, still present):
Not re-raised (already in open threads, still present):
Not re-raised (already in open threads, still present):
Not re-raised (already in open threads, still present): Residual coverage: |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Azure job Root cause: Infrastructure abort, not a code or build failure. Evidence from the log:
Since the pipeline is PR-triggered ( Implicated commit: unknown — no build error attributable to [REDACTED:Hex High Entropy String] appears in the log File: n/a (no source file implicated; failing step is "Build on x86_64" driven by Suggested fix: Re-queue build 135410 (or push/re-trigger CI on PR #11869) and check whether the run was superseded by a later commit on Related: PR #11869 (this build). No existing issue found matching the agent-shutdown signature; the issue search returned only unrelated PRs (#11921, #11895, #11845, #11842, #11857).
|
| size_t src; | ||
| int is_cx9; | ||
|
|
||
| if (ucs_array_is_empty(nics)) { |
There was a problem hiding this comment.
It's for coverty, otherwise it fails because it's thinks the array pointer may be null
What?
Build a device inventory from all of the discovered devices.
The inventory devices are grouped by type (net/acc device), sorted by bdf.
Devices under the same physical device (e.g. NIC ports, GPU sub-devices like with MPS MLOPart) are kept together.
Some filters may be also applied, such as keeping only CX9 NICs when the setup is detected as Vera Rubin.
This inventory will be used to group the devices into GPU-NIC groups, to later be used for GPU-NIC assignments.
How?