Repository navigation
UCP/CORE: Ignore DPU in topology assignments - #11990
Conversation
|
🤖 Starting review — findings will be posted here when done. |
| return 0; | ||
| } | ||
|
|
||
| static void ucp_gpu_nic_candidates_add(ucp_context_h context, |
There was a problem hiding this comment.
minor: pls update the GPU_NIC_ASSIGNMENT_MODE help text too — it says all NICs of a group are assigned, but DPU NICs and NICs without memory registration (UCT_MD_FLAG_REG) are now skipped.
|
Coverage gap:
|
|
🤖 CI Triage Agent — TL;DR: The ASAN GPU gtest run failed on a single protocol-selection assertion, Full analysisSummary: Root cause: That test (test/gtest/ucp/test_ucp_proto.cc:805-813) runs with Implicated commit: 0311ed7 — "UCP/PROTO/RNDV: Isolate remote system device namespace (#11685)", jeffnvidia (test author). The PR head commit 8f18f05 (Guy Ealey Morag) is not implicated. File: test/gtest/ucp/test_ucp_proto.cc:805 (test body), test/gtest/ucp/test_ucp_proto.cc:438-458 (failing expectations) Suggested fix:
Related: PR #11990 (build under test); PR #11685 / commit 0311ed7 (introduced the test); prior diagnostic PRs for the same test: #11858, #11861, #11864. |
|
🤖 Starting review — findings will be posted here when done. |
84fd7f2 to
0ee51e0
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| /* TODO: Detect DPU VFs, which share the generic mlx5 VF PCI id, by the | ||
| * board PSID (board_id) */ | ||
| if (uct_ib_device_spec(&md->dev)->flags & UCT_IB_DEVICE_FLAG_DPU) { |
There was a problem hiding this comment.
minor: why not set this once in md->cap_flags inside uct_ib_md_open_common(), like the other IB MD flags? dev->pci_id and the custom specs are already parsed by then, and md_query would not repeat the spec-table lookup.
|
Residual coverage gaps: the |
|
🤖 CI Triage Agent — TL;DR: The ASan roce shard aborted because UMR mkey registration on Full analysisSummary: Root cause: Two chained issues visible in the log:
Implicated commit: unknown — no commit in the PR's diff range touches the failing path; the UMR sync code is pre-existing (most recent change to File: Suggested fix:
Related: PR #11990 (#11990); no existing issue found for the
|
| md_attr->reg_cost = md->reg_cost; | ||
| md_attr->exported_mkey_packed_size = sizeof(uct_ib_md_packed_mkey_t); | ||
|
|
||
| /* TODO: Detect DPU VFs, which share the generic mlx5 VF PCI id, by the |
There was a problem hiding this comment.
a custom UCX_IB_DEVICE_SPECS entry overrides the builtin one and its flags string has no letter for DPU, so overriding a BlueField id silently disables this filter - can we add a letter for it, next to '4'/'5'/'d'/'D'/'a'?
There was a problem hiding this comment.
It seems that UCX_IB_DEVICE_SPECS is relevant only for ib verbs, and it is not used with mlx5 DV/DEVX MDs.
There was a problem hiding this comment.
Maybe not the common case but easy to address: the case is running without the mlx5 md, e.g. UCX_IB_MLX5_DEVX=n UCX_IB_MLX5_DV=n or a host where mlx5dv_is_supported() is false: there the verbs md parses UCX_IB_DEVICE_SPECS, and UCX_IB_DEVICE_SPECS=0x15b3:0xa2dc:BlueField3:D:61 shadows the builtin BlueField 3 entry, so UCT_IB_DEVICE_FLAG_DPU is lost and the DPU becomes a gpu-nic assignment candidate. Can we add a letter so it stays expressible, as 0x15b3:0xa2dc:BlueField3:5Db:61? Regarding other MD we can leave them out for now.
--- a/src/uct/ib/base/ib_md.c
+++ b/src/uct/ib/base/ib_md.c
@@ -91,6 +91,7 @@ static ucs_config_field_t uct_ib_md_config_table[] = {
" 'd' - DC version 1 (Connect-IB, ConnectX-4)\n"
" 'D' - DC version 2 (ConnectX-5 and above)\n"
" 'a' - Compact address vector support\n"
+ " 'b' - DPU device (e.g. BlueField)\n"
" <priority> - (optional) device priority, integer.\n"
"\n"
@@ -1022,6 +1023,8 @@ uct_ib_md_parse_device_config(uct_ib_md_t *md,
} else if (*p == 'a') {
spec->flags |= UCT_IB_DEVICE_FLAG_AV;
+ } else if (*p == 'b') {
+ spec->flags |= UCT_IB_DEVICE_FLAG_DPU;
} else {
ucs_error("invalid device flag: '%c'", *p);Note a custom spec replaces the builtin flags instead of merging, so the other letters have to be respelled - that is pre-existing, just easy to get wrong here.
Add a network device filter to ucs_topo_build_groups(), so callers can leave NICs out of the groups before they are assigned to GPUs. A NIC with some filtered ports keeps its other ports. NULL keeps all network devices. 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. |
60bfde0 to
9317d4f
Compare
| * @ingroup UCS_RESOURCE | ||
| * Bitmap of system devices. | ||
| */ | ||
| typedef ucs_static_bitmap_s(UCS_SYS_DEVICE_ID_COUNT) ucs_sys_device_bitmap_t; |
There was a problem hiding this comment.
maybe use this type in ucp_gpu_nic_assignment.h as well? ucp_gpu_nic_sys_dev_bitmap_t is now an identical duplicate of it.
|
🤖 Starting review — findings will be posted here when done. |
|
Dropping the CX9 filter widens the NIC set on Vera hardware (e.g. a CX7 used for storage now participates in the flip mapping). Intentional per the updated |
|
🤖 CI Triage Agent — TL;DR: The ASAN GPU gtest run failed on a single protocol‑selection test, Full analysisSummary: Root cause: The failing case is one of three sibling tests that call the identical helper Implicated commit: Not this PR. Tests introduced by 0311ed7 "UCP/PROTO/RNDV: Isolate remote system device namespace (#11685)" (jeffnvidia); selection behaviour also recently changed by fe0ea85 "UCP/RMA: Make put/rndv and get/rndv a fallback of zcopy (#11940)" (Thomas Vegas). Branch HEAD bab4a09 (Guy Ealey Morag) is unrelated to the failing code path. File: test/gtest/ucp/test_ucp_proto.cc:805-813 (test body) and test/gtest/ucp/test_ucp_proto.cc:438-459 ( Suggested fix: Harden the explicit‑scheme tests instead of the topology code:
Related: #11685 (added these tests), #11858 / #11861 / #11864 (temporary PRs explicitly created to diagnose these same Azure protocol-selection test failures), #11902 (UCP/PROTO: Fix buffer copy estimation for CPU-accessible memory), #11940 (rndv-as-fallback change), PR under test: #11990. |
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. |
| /** | ||
| * Bitmap of system devices. | ||
| */ | ||
| typedef ucs_static_bitmap_s(UCS_SYS_DEVICE_ID_COUNT) ucs_sys_device_bitmap_t; |
There was a problem hiding this comment.
minor: ucs_sys_device_bitmap_t is part of the ucs_topo_build_groups() signature and is now used from UCP (ucp_gpu_nic_assignment.h, ucp_context.c), while topo_int.h is UCS-internal. Can we declare it in topo.h next to ucs_sys_device_t, like the new flags API in this PR?
There was a problem hiding this comment.
seems it could be moved to topo.h
| } | ||
|
|
||
| static int | ||
| ucp_groups_is_net_device_allowed(const ucp_tl_md_t *md, |
There was a problem hiding this comment.
minor: the ucp_groups_ prefix does not match any UCP module; maybe ucp_context_is_net_device_allowed() to follow <layer>_<object>_<verb>.
|
Residual gaps: |
|
🤖 CI Triage Agent — TL;DR: The ASAN-GPU gtest run failed on a single assertion in Full analysisSummary: Azure job "AddressSanitizer gpu on worker 0" (build 137512) — gtest ran 10220 tests, 10219 passed, and Root cause: The failing test ( Implicated commit: PR #11990 branch File: Suggested fix:
Related: PR #11990 (this PR); test introduced by #11685; earlier diagnostics of the same test name: #11858, #11861, #11864 |
| /** | ||
| * Bitmap of system devices. | ||
| */ | ||
| typedef ucs_static_bitmap_s(UCS_SYS_DEVICE_ID_COUNT) ucs_sys_device_bitmap_t; |
There was a problem hiding this comment.
seems it could be moved to topo.h
| } | ||
|
|
||
| static int | ||
| ucp_groups_is_net_device_allowed(const ucp_tl_md_t *md, |
| } | ||
|
|
||
| ucs_status_t | ||
| ucs_topo_sys_device_add_flags(ucs_sys_device_t sys_dev, unsigned flags) |
There was a problem hiding this comment.
minor/later PR: seems we have a lot of setter/getter in this file, maybe some macro would help deduplicating eventually
| "All ports of a NIC are assigned together, and all GPUs with the same PCI\n" | ||
| "address are assigned together (e.g. MLOPart partitions of a GPU).\n" | ||
| "NICs without memory registration and DPUs are skipped during assignment.\n" | ||
| "NICs excluded by UCX_NET_DEVICES or UCX_TLS are ignored.\n" |
There was a problem hiding this comment.
AFAIU tcp lanes are keyed by sys_dev like any other net lane, so they are subject to the gpu/nic assignment. With UCX_TLS=tcp the allowed set is empty and every tcp lane is restricted, and in a mixed run a tcp lane over an mlx5 port resolves to the same sys_dev as the ib device (both go through ucs_topo_get_sysfs_dev() on the same PCI function), so it inherits that NIC's assignment.
Since nothing consumes the assignment yet, maybe fix it on the proto selection side when the restriction lands, either:
- allow the lane when the restriction would otherwise leave none, or
- skip the restriction for lanes whose md cannot register the memory, e.g.
context->reg_md_map[mem_type]. TCP ranks far behind mlx5 anyway, the actual GPU/NIC assignment use-case.
The second looks simpler and also covers the empty case, so no fallback would be needed, patch can be added to PR #11985 as it is small
| ucs_string_set_t *avail_tls, uint64_t dev_cfg_masks[], | ||
| uint64_t *tl_cfg_mask, ucp_tl_info_array_t *all_rscs) | ||
| uint64_t *tl_cfg_mask, ucp_tl_info_array_t *all_rscs, | ||
| ucs_sys_device_bitmap_t *allowed_net_devices) |
There was a problem hiding this comment.
minor: maybe something like assignable_net_device or better in UCP. in build_group() we can keep generic unchanged.
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
| ucp_context_gpu_nic_assignment_net_device_filter_init(context, | ||
| &net_device_filter); | ||
| if (UCS_STATIC_BITMAP_IS_ZERO(net_device_filter)) { | ||
| ucs_error("no network devices can be used for gpu-nic assignment"); |
There was a problem hiding this comment.
An empty filter is reachable for valid configs — UCX_TLS=shm or UCX_TLS=tcp (TCP md has no UCT_MD_FLAG_REG) leaves no assignable NIC, and then ucp_init() fails. Can we just keep the assignment disabled instead of failing init?
| ucs_error("no network devices can be used for gpu-nic assignment"); | |
| if (UCS_STATIC_BITMAP_IS_ZERO(net_device_filter)) { | |
| ucs_debug("gpu-nic assignment is disabled: no eligible network devices"); | |
| return UCS_OK; | |
| } |
(context->gpu_nic_assignment stays NULL, same as the off/non-Vera paths, since the context is ucs_calloc'd.)
| typedef ucs_topo_group_array_t ucs_topo_groups_t; | ||
|
|
||
|
|
||
| /** |
There was a problem hiding this comment.
minor: ucs_sys_device_bitmap_t is a general sys_dev bitmap (UCP uses it for its own NIC bitmaps, unrelated to groups) — maybe define it in topo.h next to ucs_sys_device_t/UCS_SYS_DEVICE_ID_COUNT.
|
Residual coverage gaps: |
| "All ports of a NIC are assigned together, and all GPUs with the same PCI\n" | ||
| "address are assigned together (e.g. MLOPart partitions of a GPU).\n" | ||
| "NICs without memory registration and DPUs are skipped during assignment.\n" | ||
| "NICs excluded by UCX_NET_DEVICES or UCX_TLS are ignored.\n" |
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
| ucp_context_gpu_nic_assignment_net_device_filter_init(context, | ||
| &net_device_filter); | ||
| if (UCS_STATIC_BITMAP_IS_ZERO(net_device_filter)) { | ||
| if (context->config.ext.gpu_nic_assignment_mode == |
There was a problem hiding this comment.
minor: mode was already rewritten from auto to flip above, so this has to re-read the config. pls keep an is_auto flag with the other locals and test it here.
There was a problem hiding this comment.
ok as it's reading the ask from user, not its resolution
| * @ingroup UCS_RESOURCE | ||
| * Bitmap of system devices. | ||
| */ | ||
| typedef ucs_static_bitmap_s(UCS_SYS_DEVICE_ID_COUNT) ucs_sys_device_bitmap_t; |
There was a problem hiding this comment.
maybe move ucs_sys_device_bitmap_t to topo.h next to ucs_sys_device_t? it is not groups-specific and ucp uses it for resource filtering.
What?
Exclude DPUs (e.g. BlueField) from the GPU-NIC assignment, moving the filtering from UCS topology to UCT/UCP:
Why?