UCP/DEVICE: check connect only if there are p2p lanes by Artemy-Mellanox · Pull Request #11966 · openucx/ucx

Artemy-Mellanox · 2026-09-17T13:52:47Z

No description provided.

svc-nvidia-pr-review · 2026-09-17T13:57:22Z

🤖 Starting review — findings will be posted here when done.

svc-nvidia-pr-review · 2026-09-17T14:04:25Z


-        if (!(ep->flags & UCP_EP_FLAG_REMOTE_CONNECTED)) {
+        if (!(ep->flags & UCP_EP_FLAG_REMOTE_CONNECTED) &&
+            (ucp_ep_config(ep)->p2p_lanes != 0)) {


p2p_lanes == 0 does not imply the EP is wired. For a client-server (sockaddr/CM) EP the initial config installed by ucp_ep_init_create_wireup() has only a CM lane (rsc_index == UCP_NULL_RESOURCE), and ucp_ep_config_init() skips NULL-resource lanes when computing p2p_lanes, so p2p_lanes == 0 for the whole CM handshake window. In that window this condition skips the early return and ucp_device_remote_mem_list_create_handle() finds no matching lane, returning UCS_ERR_INVALID_PARAM with an ucs_error("lane not found for element ...") log instead of the retryable UCS_ERR_NOT_CONNECTED; later windows where lanes are still ucp_wireup_ep proxies return UCS_ERR_NO_RESOURCE. This breaks the documented retry-on-UCS_ERR_NOT_CONNECTED contract used by the gtest and ucp_cuda_kernel.cu, causing hard failures plus error-log spam. Suggestion:

Suggested change

(ucp_ep_config(ep)->p2p_lanes != 0)) {

if (!(ep->flags & UCP_EP_FLAG_REMOTE_CONNECTED) &&

((ucp_ep_config(ep)->p2p_lanes != 0) || ucp_ep_has_cm_lane(ep))) {

svc-nvidia-pr-review · 2026-09-17T14:04:25Z

        }

-        if (!(ep->flags & UCP_EP_FLAG_REMOTE_CONNECTED)) {
+        if (!(ep->flags & UCP_EP_FLAG_REMOTE_CONNECTED) &&


minor: pls add a short comment why p2p_lanes == 0 skips the wait - such EPs are marked LOCAL_CONNECTED in ucp_wireup_init_lanes() and never do the WIREUP handshake, so REMOTE_CONNECTED is never set.

svc-nvidia-pr-review · 2026-09-17T14:04:26Z

Test coverage: test/gtest/ucp/test_ucp_device.cc is instantiated only with "rc,rc_gda" (p2p lanes), so this newly enabled branch is not exercised by CI. Can we add a connectionless device-lane variant (cuda_ipc/rocm_ipc), or explain why it is not practical?

svc-nvidia-pr-review · 2026-09-17T14:28:54Z

🤖 Starting review — findings will be posted here when done.

svc-nvidia-pr-review · 2026-09-17T14:40:49Z


-        if (!(ep->flags & UCP_EP_FLAG_REMOTE_CONNECTED)) {
+        if (!(ep->flags & UCP_EP_FLAG_REMOTE_CONNECTED) &&
+            (ucp_ep_config(ep)->p2p_lanes != 0)) {


The relaxed check also covers the CM flow, where lanes are not ready yet. p2p_lanes == 0 alone does not mean "endpoint is in its final state". With a CM lane (client-server), ucp_wireup_connect_p2p() returns ucp_worker_is_tl_p2p(), so connectionless device TLs (cuda_ipc/rocm_ipc) are never in p2p_lanes, and the intermediate config created by ucp_ep_init_create_wireup() has only the CM lane. In that window ucp_device_remote_mem_list_create() now proceeds past the connectivity check and fails later in ucp_device_remote_mem_list_create_handle() with ucs_error("lane not found for element ...") / UCS_ERR_INVALID_PARAM, instead of the retryable UCS_ERR_NOT_CONNECTED that the comment right below promises. Please also exclude the CM case here, using the same condition as ucp_proto_reconfig_progress():

Suggested change

(ucp_ep_config(ep)->p2p_lanes != 0)) {

if (!(ep->flags & UCP_EP_FLAG_REMOTE_CONNECTED) &&

((ucp_ep_config(ep)->p2p_lanes != 0) || ucp_ep_has_cm_lane(ep))) {

svc-nvidia-pr-review · 2026-09-17T14:40:49Z

    add_variant_memtypes(variants, get_base_variants,
-                         UCS_BIT(UCS_MEMORY_TYPE_CUDA) |
-                         UCS_BIT(UCS_MEMORY_TYPE_HOST));
+                         UCS_BIT(UCS_MEMORY_TYPE_CUDA));


Host memtype coverage removed from the existing rc_gda instantiation: rx_mem_type() drives the destination buffer type, so rc,rc_gda loses its host-destination variants. Only the new sm,cuda_ipc instantiation needs cuda-only (cuda_ipc reg_mem_types is CUDA only), so this is a coverage loss on the pre-existing suite. Can we keep host and skip it only for the cuda_ipc variant?

svc-nvidia-pr-review · 2026-09-17T14:40:49Z

    ipc_addr       = uct_cuda_ipc_iface_address_unpack(params->iface_addr,
                                                       iface_addr_len);
-    if (same_uuid &&
+    if (same_uuid && !iface->config.enable_same_process &&


Same-process reachability is not restricted to the same device. The data-path shortcut in uct_cuda_ipc_map_memhandle() applies only when the local device UUID equals key->uuid. With ENABLE_SAME_PROCESS=y and peers on two GPUs of the same process, the legacy path calls cuIpcOpenMemHandle() on a handle exported by the same process; CUDA_ERROR_ALREADY_MAPPED maps to UCS_ERR_ALREADY_EXISTS and, after invalidate+purge retries, uct_cuda_ipc_cache_put_region() ends in ucs_fatal(). The config text says "same device", but nothing enforces it. Is that case handled, or should the knob be limited to the same device as the description states?

svc-nvidia-pr-review · 2026-09-17T14:40:50Z

Residual coverage gap: the new sm,cuda_ipc instantiation is added only to test_ucp_device_xfer; the base test_ucp_device create/param-check tests and the CM (client-server) path with connectionless device lanes remain uncovered, which is exactly the flow in finding 1.

tvegas1 · 2026-09-18T12:50:47Z

    ipc_addr       = uct_cuda_ipc_iface_address_unpack(params->iface_addr,
                                                       iface_addr_len);
-    if (same_uuid &&
+    if (same_uuid && !iface->config.enable_same_process &&


fixed in #11967

tvegas1

need to add cm lane as bot suggested

…dev_check_connect

svc-nvidia-pr-review · 2026-09-20T06:56:24Z

🤖 Starting review — findings will be posted here when done.

svc-nvidia-pr-review · 2026-09-20T07:05:36Z


-        if (!(ep->flags & UCP_EP_FLAG_REMOTE_CONNECTED)) {
+        if (!(ep->flags & UCP_EP_FLAG_REMOTE_CONNECTED) &&
+            (ucp_ep_config(ep)->p2p_lanes != 0)) {


minor: pls add a short comment explaining when the connected check can be skipped, e.g. which lane state makes it unnecessary - it is not obvious from the device code why p2p_lanes decides this.

svc-nvidia-pr-review · 2026-09-20T07:05:37Z

CM / client-server flow with connectionless device lanes (previously raised): with a CM lane and p2p_lanes == 0, the lanes are still wireup_ep proxies and the intermediate config has no device lane, so the caller now gets UCS_ERR_NO_RESOURCE/UCS_ERR_INVALID_PARAM plus an ucs_error() instead of a retryable UCS_ERR_NOT_CONNECTED. Note ucp_proto_reconfig_progress() (src/ucp/proto/proto_reconfig.c:89-91) encodes the same "EP is in final state" rule as !p2p_lanes && !ucp_ep_has_cm_lane(ep), which supports that point.

Missing coverage of test_ucp_device / test_ucp_device_kernel with a connectionless device-lane config (base class still only has rc,cuda_ipc, which has p2p lanes and therefore does not reach the new branch).

Residual gap: no CI job exercises the new branch unless the runner has CUDA IPC; on non-GPU/non-IPC machines the new instantiation skips via UCS_ERR_NO_DEVICE.

Residual gap: pre-fix behavior of the new test is a hang (the mem_list ctor retries while (status == UCS_ERR_NOT_CONNECTED) unboundedly) rather than a clean failure, so it is not a strict regression test in the test/AGENTS.md sense. This is pre-existing test-helper code.

UCP/DEVICE: check connect only if there are p2p lanes

d6b231f

svc-nvidia-pr-review reviewed Sep 17, 2026

View reviewed changes

UCT/CUDA_IPC: add UCP device tests

843e959

svc-nvidia-pr-review reviewed Sep 17, 2026

View reviewed changes

tvegas1 reviewed Sep 18, 2026

View reviewed changes

ofirfarjun7 requested a review from tvegas1 September 18, 2026 14:10

tvegas1 reviewed Sep 18, 2026

View reviewed changes

Merge remote-tracking branch 'refs/remotes/origin/master' into topic/…

6c177bf

…dev_check_connect

svc-nvidia-pr-review reviewed Sep 20, 2026

View reviewed changes

ofirfarjun7 approved these changes Sep 20, 2026

View reviewed changes

Artemy-Mellanox mentioned this pull request Sep 20, 2026

UCP/DEVICE: check connect only if there are p2p lanes - v1.23.x #11974

Merged

	(ucp_ep_config(ep)->p2p_lanes != 0)) {
	if (!(ep->flags & UCP_EP_FLAG_REMOTE_CONNECTED) &&
	((ucp_ep_config(ep)->p2p_lanes != 0) \|\| ucp_ep_has_cm_lane(ep))) {

Conversation

Artemy-Mellanox commented Sep 17, 2026

Uh oh!

svc-nvidia-pr-review commented Sep 17, 2026

Uh oh!

svc-nvidia-pr-review Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

svc-nvidia-pr-review Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

svc-nvidia-pr-review commented Sep 17, 2026

Uh oh!

svc-nvidia-pr-review commented Sep 17, 2026

Uh oh!

svc-nvidia-pr-review Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

svc-nvidia-pr-review Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

svc-nvidia-pr-review Sep 17, 2026

Choose a reason for hiding this comment

Uh oh!

svc-nvidia-pr-review commented Sep 17, 2026

Uh oh!

tvegas1 Sep 18, 2026

Choose a reason for hiding this comment

Uh oh!

tvegas1 left a comment

Choose a reason for hiding this comment

Uh oh!

svc-nvidia-pr-review commented Sep 20, 2026

Uh oh!

svc-nvidia-pr-review Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

svc-nvidia-pr-review commented Sep 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants