Skip to content

UCS/SYS/TOPO: Allow multiple sibling devices per memory device #12126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tvegas1 merged 12 commits into from
Oct 30, 2026
Merged

UCS/SYS/TOPO: Allow multiple sibling devices per memory device #12126

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
8e80252
UCS/SYS/TOPO: Allow multiple sibling devices per memory device
tomerg-nvidia Oct 20, 2026
7ad0252
Merge branch 'master' into topo-multi-dev-sibling
tomerg-nvidia Oct 21, 2026
13342ad
UCS/SYS/TOPO: Fix CR comments
tomerg-nvidia Oct 27, 2026
c165850
UCS/SYS/TOPO: Bound sibling index in clear_siblings
tomerg-nvidia Oct 29, 2026
c48b959
UCS/SYS/TOPO: Explain early returns in aux path setup
tomerg-nvidia Oct 29, 2026
49c469b
GTEST/TOPO: Initialize siblings explicitly in topo groups test
tomerg-nvidia Oct 29, 2026
aa7b843
GTEST/TOPO: Factor out PCIe device requirements of sibling tests
tomerg-nvidia Oct 29, 2026
f3bbfd1
GTEST/TOPO: Abort on device registration failure in a helper
tomerg-nvidia Oct 29, 2026
faef0d4
GTEST/TOPO: Factor out memory alias and plane registration in sibling…
tomerg-nvidia Oct 29, 2026
31862a2
UCS/SYS/TOPO: Bound sibling index by the devices table size
tomerg-nvidia Oct 29, 2026
e7a9875
UCS/SYS/TOPO: Check the device role in is_sibling
tomerg-nvidia Oct 29, 2026
548e94f
GTEST/TOPO: Rename register_mem_aliases count to num_mems
tomerg-nvidia Oct 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 73 additions & 39 deletions src/ucs/sys/topo/base/topo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,18 +510,18 @@ ucs_topo_find_device_by_bus_id_value(const ucs_sys_bus_id_t *bus_id,
*sys_dev_p = ucs_topo_global_ctx.num_devices++;
device = &ucs_topo_global_ctx.devices[*sys_dev_p];

device->bus_id = *bus_id;
device->name = name;
device->name_priority = 0;
device->numa_node = numa_node;
device->pci_id = pci_id;
device->user_value = user_value;
device->device_class = UCS_TOPO_DEVICE_CLASS_UNKNOWN;
device->flags = 0;
device->class_ordinal = UCS_SYS_DEVICE_ORDINAL_INVALID;
device->sibling_role = UCS_TOPO_SIBLING_ROLE_NONE;
device->sibling_sys_dev = UCS_SYS_DEVICE_ID_UNKNOWN;
device->sys_dev_aux = UCS_SYS_DEVICE_ID_UNKNOWN;
device->bus_id = *bus_id;
device->name = name;
device->name_priority = 0;
device->numa_node = numa_node;
device->pci_id = pci_id;
device->user_value = user_value;
device->device_class = UCS_TOPO_DEVICE_CLASS_UNKNOWN;
device->flags = 0;
device->class_ordinal = UCS_SYS_DEVICE_ORDINAL_INVALID;
device->sibling_role = UCS_TOPO_SIBLING_ROLE_NONE;
device->sys_dev_aux = UCS_SYS_DEVICE_ID_UNKNOWN;
UCS_STATIC_BITMAP_RESET_ALL(&device->siblings);
Comment thread
tomerg-nvidia marked this conversation as resolved.
Resolved

if (user_value == UCS_SYS_DEVICE_USER_VALUE_EMPTY) {
ucs_debug(
Expand Down Expand Up @@ -803,22 +803,23 @@ ucs_topo_is_reachable(ucs_sys_device_t sys_dev, ucs_sys_device_t sys_dev_mem)
result =
(sys_dev >= ucs_topo_global_ctx.num_devices) ||
(sys_dev_mem >= ucs_topo_global_ctx.num_devices) ||
/*
* Memory device was never matched with a sibling, it does not
* mandate an auxiliary path.
*/
(ucs_topo_global_ctx.devices[sys_dev_mem].sibling_sys_dev ==
UCS_SYS_DEVICE_ID_UNKNOWN) ||
/* The device itself never uses auxiliary path */
(ucs_topo_global_ctx.devices[sys_dev].sibling_role !=
UCS_TOPO_SIBLING_ROLE_DEV) ||
/*
* MPS MLOParts create multiple MEM aliases for one DEV. Each MEM
* alias records its matched DEV; the DEV stores only one
* representative MEM sibling.
* MPS MLOParts create multiple MEM aliases for one DEV, and
* multiplane NICs create multiple DEVs for one MEM. Each MEM
* records all of its matched DEVs.
*/
UCS_STATIC_BITMAP_GET(
ucs_topo_global_ctx.devices[sys_dev_mem].siblings,
sys_dev) ||
/*
* Memory device was never matched with a sibling, it does not
* mandate an auxiliary path.
*/
(ucs_topo_global_ctx.devices[sys_dev_mem].sibling_sys_dev ==
sys_dev);
UCS_STATIC_BITMAP_IS_ZERO(
ucs_topo_global_ctx.devices[sys_dev_mem].siblings);
ucs_spin_unlock(&ucs_topo_global_ctx.lock);

return result;
Expand All @@ -836,11 +837,12 @@ int ucs_topo_is_sibling(ucs_sys_device_t sys_dev, ucs_sys_device_t sys_dev_mem)
(sys_dev_mem != UCS_SYS_DEVICE_ID_UNKNOWN) &&
(sys_dev_mem < ucs_topo_global_ctx.num_devices) &&
/*
* MLOPart allows multiple MEM devices to point at one DEV, so
* the MEM-side sibling is the canonical relationship.
* The MEM side holds every matched DEV, including all planes
* of a multiplane NIC.
*/

@tvegas1 tvegas1 Oct 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bitmap is symmetric, so swapped arguments also return true and trip the assert. Please gate on the role, like ucs_topo_is_reachable() does.

Suggested change
*/
(ucs_topo_global_ctx.devices[sys_dev].sibling_role ==
UCS_TOPO_SIBLING_ROLE_DEV) &&
/*
* The MEM side holds every matched DEV, including all planes
* of a multiplane NIC.
*/
UCS_STATIC_BITMAP_GET(
ucs_topo_global_ctx.devices[sys_dev_mem].siblings,
sys_dev);

@tomerg-nvidia tomerg-nvidia Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

(ucs_topo_global_ctx.devices[sys_dev_mem].sibling_sys_dev ==
sys_dev);
UCS_STATIC_BITMAP_GET(

@svc-nvidia-pr-review svc-nvidia-pr-review Oct 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the siblings bitmap is symmetric now, so is_sibling(mem, dev) also returns true and then trips the assert below (and silently returns 1 in release builds). pls make the role part of the condition instead of only asserting it:
(devices[sys_dev].sibling_role == UCS_TOPO_SIBLING_ROLE_DEV) && UCS_STATIC_BITMAP_GET(...)

Swapped calls do exist in the tree (test_topo.cc sibling test calls ucs_topo_is_sibling(gpu_dev, hca_devs[0])), and the old scalar only exposed this for the one representative MEM, so the bitmap widens the exposure to every alias and plane.

ucs_topo_global_ctx.devices[sys_dev_mem].siblings,
sys_dev);

if (is_sibling) {
role_dev = ucs_topo_global_ctx.devices[sys_dev].sibling_role;
Expand Down Expand Up @@ -1372,28 +1374,44 @@ ucs_topo_is_pci_bridge(ucs_sys_device_t sys_dev1, ucs_sys_device_t sys_dev2)
return result;
}

static int ucs_topo_sys_device_sibling_match(ucs_sys_device_t sys_dev,
ucs_sys_device_t sys_dev_mem)
static void ucs_topo_sys_device_sibling_match(ucs_sys_device_t sys_dev,
ucs_sys_device_t sys_dev_mem)
{
ucs_sys_device_t sys_dev_aux;

if ((ucs_topo_global_ctx.devices[sys_dev_mem].sibling_role !=
UCS_TOPO_SIBLING_ROLE_MEM) ||
(ucs_topo_global_ctx.devices[sys_dev].sibling_role !=
UCS_TOPO_SIBLING_ROLE_DEV)) {
return 0;
return;
}

sys_dev_aux = ucs_topo_global_ctx.devices[sys_dev].sys_dev_aux;
if (ucs_topo_is_pci_bridge(sys_dev_mem, sys_dev_aux)) {
ucs_topo_global_ctx.devices[sys_dev_mem].sibling_sys_dev = sys_dev;
ucs_topo_global_ctx.devices[sys_dev].sibling_sys_dev = sys_dev_mem;
UCS_STATIC_BITMAP_SET(
&ucs_topo_global_ctx.devices[sys_dev_mem].siblings, sys_dev);
UCS_STATIC_BITMAP_SET(&ucs_topo_global_ctx.devices[sys_dev].siblings,
sys_dev_mem);
ucs_trace("sys_dev=%u with sys_dev_aux=%u matched with sys_dev_mem=%u",
sys_dev, sys_dev_aux, sys_dev_mem);
return 1;
}
}

static void ucs_topo_sys_device_clear_siblings(ucs_sys_device_t sys_dev)
{
ucs_topo_sys_device_info_t *device = &ucs_topo_global_ctx.devices[sys_dev];
unsigned peer;

UCS_STATIC_BITMAP_FOR_EACH_BIT(peer, &device->siblings) {
Comment thread
tomerg-nvidia marked this conversation as resolved.
Resolved
if (peer >= ucs_topo_global_ctx.num_devices) {
break;

@svc-nvidia-pr-review svc-nvidia-pr-review Oct 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: bits are only set for indices < num_devices (every registration does RESET_ALL, and num_devices only grows except via extract_state, whose callers bound-check sys_dev), so this branch is unreachable. A silent break would also leave the remaining peers stale while device->siblings is cleared below, i.e. asymmetric state. Suggestion:

    UCS_STATIC_BITMAP_FOR_EACH_BIT(peer, &device->siblings) {
        ucs_assertv(peer < ucs_topo_global_ctx.num_devices,
                    "sys_dev=%u peer=%u num_devices=%u", sys_dev, peer,
                    ucs_topo_global_ctx.num_devices);
        UCS_STATIC_BITMAP_RESET(&ucs_topo_global_ctx.devices[peer].siblings,
                                sys_dev);
    }

}

UCS_STATIC_BITMAP_RESET(&ucs_topo_global_ctx.devices[peer].siblings,
Comment thread
tvegas1 marked this conversation as resolved.
Resolved
sys_dev);
}

return 0;
UCS_STATIC_BITMAP_RESET_ALL(&device->siblings);
}

/* Memory device supports auxiliary path if a sibling is found */
Expand All @@ -1410,13 +1428,20 @@ ucs_status_t ucs_topo_sys_device_enable_aux_path(ucs_sys_device_t sys_dev)
goto out;
}

/* Already a memory device: keep the existing matches and skip the rescan */
if (ucs_topo_global_ctx.devices[sys_dev].sibling_role ==
Comment thread
tvegas1 marked this conversation as resolved.
Resolved
UCS_TOPO_SIBLING_ROLE_MEM) {
status = UCS_OK;
goto out;
}

ucs_topo_sys_device_clear_siblings(sys_dev);
ucs_topo_global_ctx.devices[sys_dev].sibling_role =
UCS_TOPO_SIBLING_ROLE_MEM;

/* All devices sharing the auxiliary device match the memory device */
for (dev = 0; dev < ucs_topo_global_ctx.num_devices; dev++) {

@svc-nvidia-pr-review svc-nvidia-pr-review Oct 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: is_pci_bridge() does 2 realpath() + 3 PATH_MAX allocations per candidate under the global spinlock, so now every memory device pays it for all DEVs. With MPS aliases this becomes num_mems * num_devs path resolutions at init. maybe we can resolve the sysfs path of sys_dev_aux once per device instead of per pair?

if (ucs_topo_sys_device_sibling_match(dev, sys_dev)) {
break;
}
ucs_topo_sys_device_sibling_match(dev, sys_dev);
Comment thread
tomerg-nvidia marked this conversation as resolved.
Resolved
}
Comment thread
tvegas1 marked this conversation as resolved.
Resolved

status = UCS_OK;
Expand All @@ -1440,6 +1465,15 @@ ucs_status_t ucs_topo_sys_device_set_sys_dev_aux(ucs_sys_device_t sys_dev,
goto out;
}

/* Same auxiliary device as before: skip the rescan */
if ((ucs_topo_global_ctx.devices[sys_dev].sibling_role ==
UCS_TOPO_SIBLING_ROLE_DEV) &&
(ucs_topo_global_ctx.devices[sys_dev].sys_dev_aux == sys_dev_aux)) {
status = UCS_OK;
goto out;
}

ucs_topo_sys_device_clear_siblings(sys_dev);
Comment thread
tomerg-nvidia marked this conversation as resolved.
Resolved
ucs_topo_global_ctx.devices[sys_dev].sys_dev_aux = sys_dev_aux;
ucs_topo_global_ctx.devices[sys_dev].sibling_role =
UCS_TOPO_SIBLING_ROLE_DEV;
Expand All @@ -1462,8 +1496,8 @@ int ucs_topo_device_has_sibling(ucs_sys_device_t sys_dev)

ucs_spin_lock(&ucs_topo_global_ctx.lock);
result = (sys_dev < ucs_topo_global_ctx.num_devices) &&
(ucs_topo_global_ctx.devices[sys_dev].sibling_sys_dev !=
UCS_SYS_DEVICE_ID_UNKNOWN);
!UCS_STATIC_BITMAP_IS_ZERO(
ucs_topo_global_ctx.devices[sys_dev].siblings);
ucs_spin_unlock(&ucs_topo_global_ctx.lock);

return result;
Expand Down
7 changes: 0 additions & 7 deletions src/ucs/sys/topo/base/topo_groups.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,6 @@ UCS_ARRAY_DECLARE_TYPE(ucs_topo_group_array_t, size_t, ucs_topo_group_t);
typedef ucs_topo_group_array_t ucs_topo_groups_t;


/**
* @ingroup UCS_RESOURCE
* Bitmap of system devices.
*/
typedef ucs_static_bitmap_s(UCS_SYS_DEVICE_ID_COUNT) ucs_sys_device_bitmap_t;


/**
* Build system topology groups.
*
Expand Down
15 changes: 13 additions & 2 deletions src/ucs/sys/topo/base/topo_int.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

#include "topo.h"

#include <ucs/datastruct/static_bitmap.h>

BEGIN_C_DECLS

/* Possible role of a current device wrt its sibling */
Expand All @@ -24,6 +26,13 @@ typedef enum {
} ucs_topo_sibling_role_t;


/**
* @ingroup UCS_RESOURCE
* Bitmap of system devices.
*/
typedef ucs_static_bitmap_s(UCS_SYS_DEVICE_ID_COUNT) ucs_sys_device_bitmap_t;


typedef struct {
ucs_sys_bus_id_t bus_id;
char *name;
Expand All @@ -43,8 +52,10 @@ typedef struct {
ucs_sys_device_t sys_dev_aux;

ucs_topo_sibling_role_t sibling_role; /* Role of the current device */
/* MEM role: matched DEV. DEV role: one representative matched MEM. */
ucs_sys_device_t sibling_sys_dev;
/* Matched siblings: all DEVs for the MEM role, all MEMs for the DEV role.
* Several DEVs can share one auxiliary device (e.g. planes of a multiplane
* NIC), and several MEMs can sit behind it (e.g. MPS MLOPart aliases). */
ucs_sys_device_bitmap_t siblings;
Comment thread
tvegas1 marked this conversation as resolved.
Resolved
} ucs_topo_sys_device_info_t;


Expand Down
Loading
Loading