Skip to content

UCP/PROTO/RNDV: Isolate remote system device namespace - #11685

Merged
tvegas1 merged 29 commits into
openucx:masterfrom
jeffnvidia:agent/fix-rndv-remote-sysdev
Sep 14, 2026
Merged

tvegas1 merged 29 commits into
openucx:masterfrom
jeffnvidia:agent/fix-rndv-remote-sysdev

Conversation

@jeffnvidia

@jeffnvidia jeffnvidia commented Jul 21, 2026 •

Copy link
Copy Markdown
Contributor

What?

Fix UCP rendezvous protocol selection for Direct NIC when system-device IDs
belong to different topology namespaces. Model GET/PUT mtype network lanes
using the staging fragment device.

Why?

Direct NIC RNDV can fail in find_lanes: while estimating the protocol the
peer will select, UCX interprets a peer-local sys_dev from the rkey in the
local topology and can reject a valid lane. This remote-namespace correction
applies to both mtype and non-mtype protocols. For mtype GET/PUT, network I/O
accesses the staging fragment (for example, a host bounce buffer for CUDA),
but protocol selection still needs the user buffer sys_dev for lane
spreading.

How?

Never interpret the rkey's peer-local sys_dev in the local topology. For
nested remote estimation, use the local select_param.sys_dev as a
namespace-safe topology proxy when the local and remote memory types match;
otherwise use UCS_SYS_DEVICE_ID_UNKNOWN. For mtype probes, keep the
user/final device in select_param.sys_dev and pass the staging fragment
through reg_mem_info. The common minimum-fragment filter validates
transport reachability once against reg_mem_info: for SEND_ZCOPY this
describes the selected user buffer, while RNDV mtype protocols supply their
staging fragment.

Keep the existing GPU peer-failure pipeline test deterministic by using a host
staging fragment; it still exercises CUDA user memory through
rndv/put/mtype and retains the original failure assertions. Regression
tests independently cover remote namespace isolation and the user/fragment
device roles.

@jeffnvidia
jeffnvidia force-pushed the agent/fix-rndv-remote-sysdev branch 4 times, most recently from 2c8f110 to 99f9f4b Compare July 21, 2026 15:30
@jeffnvidia
jeffnvidia marked this pull request as ready for review July 21, 2026 15:53
@jeffnvidia
jeffnvidia force-pushed the agent/fix-rndv-remote-sysdev branch from 99f9f4b to 5fa905a Compare July 22, 2026 07:31
@gleon99

gleon99 commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

@jeffnvidia can add a gtest?

@svc-ucx

svc-ucx commented Aug 23, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR · commit 5fa905a7

Summary: Unable to fetch logs — cannot diagnose without them.
Root cause: Unknown — log access failed.
Implicated commit: unknown
File: unknown
Suggested fix: Provide a direct link to the failed build log in a follow-up comment.
Log fetch attempts:

  • fetch_azure_logs({'build_url': 'https://dev.azure.com/ucfconsort/0b36e3f0-8ab9-4a48-b68b-[REDACTED:Hex High Entropy String]/_build/results?buildId=129077'}) → [fetch_azure_logs ERROR: timeline HTTP 404 for build 129077]
  • fetch_azure_logs({'build_url': 'https://dev.azure.com/ucfconsort/ucx/_build/results?buildId=129077'}) → [fetch_azure_logs ERROR: timeline HTTP 404 for build 129077]
  • fetch_jenkins_stages({'job': 'UCX PR', 'build': 129077}) → [fetch_jenkins_stages ERROR] build URL names Jenkins controller 'https://dev.azure.com/ucfconsort/0b36e3f0-8ab9-4a48-b68b-[REDACTED:Hex High Entropy String]/_build/results?buildId=129077', but this deployment is configured for 'https://nbuprod.blsm.nvidia.com/nb
  • fetch_jenkins_logs({'job': 'UCX PR', 'build': 129077}) → [fetch_jenkins_logs ERROR] build URL names Jenkins controller 'https://dev.azure.com/ucfconsort/0b36e3f0-8ab9-4a48-b68b-[REDACTED:Hex High Entropy String]/_build/results?buildId=129077', but this deployment is configured for 'https://nbuprod.blsm.nvidia.com/nbu-
  • fetch_azure_logs({'build_url': 'https://dev.azure.com/ucfconsort/0b36e3f0-8ab9-4a48-b68b-[REDACTED:Hex High Entropy String]/_build/results?buildId=129077&view=logs'}) → [fetch_azure_logs ERROR: timeline HTTP 404 for build 129077]
  • fetch_azure_logs({'build_url': 'https://dev.azure.com/ucfconsort/_build/results?buildId=129077'}) → [fetch_azure_logs ERROR: not an Azure DevOps URL: 'https://dev.azure.com/ucfconsort/_build/results?buildId=129077']

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 51469824-b7af-4451-ad65-8dab123760e3 in the triage console for the audit trail.

@jeffnvidia

Copy link
Copy Markdown
Contributor Author

@gleon99 Added test_ucp_proto_mock_rcx.rndv_remote_sys_dev_namespace in 8e90a98. It runs the real rndv/rtr selection with a known peer-local sys_dev, verifies that the rkey configuration preserves it while the synthetic remote selection uses UNKNOWN, and fails when only the original assignment is restored.

@svc-ucx

svc-ucx commented Aug 25, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests gpu on worker 2) · commit 8e90a98b

TL;DR: The GPU gtest job failed with 2 real test failures — shm_ib/test_ucp_peer_failure_rndv_put_ppln_abort.pipeline/1 and shm_ib_ipc/…pipeline/1 (CUDA, 3‑stage rndv/put/mtype pipeline with RNDV_FRAG_SIZE=cuda:8K) — and the PR's only change (ucp_proto_rndv_remote_select_param_init() now passes UCS_SYS_DEVICE_ID_UNKNOWN as the remote memory's system device) alters/breaks remote protocol estimation for GPU mtype/pipeline protocols, so the expected rndv/put/mtype pipeline is no longer exercised and the abort assertions fail.

Full analysis

Summary: make test in the gtest stage exited non‑zero: 10194 tests ran, 10172 passed, 2 failed — both are the GPU‑aware test_ucp_peer_failure_rndv_put_ppln_abort.pipeline/1 (tag) variants on shm,ib,gdr_copy,cuda_copy,rocm_copy and shm,ib,cuda_ipc,rocm_ipc,cuda_copy,rocm_copy.

Root cause: The failure is not a timeout or infra issue — the log shows continuous test activity up to the summary, make: *** [Makefile:4714: test] Error 1, and exactly two [ FAILED ] entries. Both failures are the CUDA 3‑stage rendezvous pipeline abort test, which is the narrow path affected by this PR. The PR touches only src/ucp/rndv/proto_rndv.c (commit 5fa905a7), where the remote select param for a known remote buffer now discards the rkey's system device:

/* The rkey system device belongs to the remote topology namespace ... */
mem_info.sys_dev = UCS_SYS_DEVICE_ID_UNKNOWN;
mem_info.type    = init_params->rkey_config_key->mem_type;

(src/ucp/rndv/proto_rndv.c:340)

That select param drives probing of the peer's protocols, including ucp_proto_rndv_put_mtype_probe(), which is sys_dev‑sensitive: it calls ucp_mm_get_alloc_md_index(context, frag_mem_type, init_params->select_param->sys_dev, …) and bails out (return) on failure, and its perf/frag segment feeds rndv/send/ppln (rndv_ppln.c:98‑139). With an UNKNOWN device the mtype/ppln remote variant is either not probed or aggregates to empty perf ("Empty perf means that this ctrl message cannot be used with that remote proto (e.g. rndv/rtr/mtype + rndv/send/ppln)", proto_rndv.c:449‑454), so the 3‑stage rndv/rtr/mtype + rndv/send/ppln + rndv/put/mtype combination is not selected. The test's mocked progress_wrapper then never sees proto->name == "rndv/put/mtype", close_peer() is never called, the send/recv succeed, and ASSERT_TRUE(UCS_STATUS_IS_ERR(result.first/second)) / ASSERT_TRUE(m_is_peer_closed) fail (test/gtest/ucp/test_ucp_peer_failure.cc:910‑918). Consistently, the sibling rtr_mtype test (default frag size, single fragment, no ppln) still passes — only the fragmented pipeline variant regressed.

Implicated commit: 5fa905a7 — "UCP/PROTO/RNDV: Isolate remote system device namespace" (Jeffrey Mahou), the sole change in PR #11685 (head 8e90a98b).

File: src/ucp/rndv/proto_rndv.c:332-346 (specifically line 340); affected consumers: src/ucp/rndv/rndv_put.c:596-648 (ucp_proto_rndv_put_mtype_probe), src/ucp/rndv/rndv_ppln.c:98-139; failing assertions at test/gtest/ucp/test_ucp_peer_failure.cc:910-918.

Suggested fix: Don't drop the device information entirely when isolating the namespace. Either (a) translate the rkey's remote sys_dev into a valid local-namespace device for estimation (keeping the memory type and a concrete device), or (b) make the mtype/pipeline probes tolerate UCS_SYS_DEVICE_ID_UNKNOWN — in ucp_proto_rndv_put_mtype_probe()/ucp_proto_rndv_mtype_init() fall back to the default allocation MD/device for frag_mem_type instead of returning early — so the rndv/put/mtype + rndv/send/ppln variant is still probed for CUDA buffers. Validate by running the two failing tests locally with UCX_PROTO_INFO=y and UCX_RNDV_SCHEME=put_ppln UCX_RNDV_FRAG_SIZE=host:8K,cuda:8K, diffing the selected protocols before/after the commit (they must still show pipeline … rndv/put/mtype). Do not simply re-run CI: the same test failed in both GPU configurations, which argues against flakiness.

Related: PR #11685 (this change); PR #10075 "UCP/RNDV/PPLN: Advertise error handling capability for put_mtype/ppln protocols" (origin of RNDV_PIPELINE_ERROR_HANDLING / is_rndv_put_ppln_supported used by this test).

@jeffnvidia

Copy link
Copy Markdown
Contributor Author

Follow-up on the GPU CI failures: pushed d093cd3d8.

The regression came from mtype lane modeling using the remote lookup’s UNKNOWN system device. The fix keeps the remote key UNKNOWN for namespace safety, while using the local staging fragment’s device for GET/PUT mtype lane probing.

On the exact failed CI merge, both affected pipeline-abort tests and the namespace test passed (9/9). The trace confirms the staging GPU ordinal is now used. Fresh Azure CI is pending.

@svc-ucx

svc-ucx commented Aug 27, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (go gpu) · commit d093cd3d

TL;DR: The go gpu job failed because TestUcpWorkerEfd hung for the full 10-minute Go test timeout: syscall.EpollWait returned EINTR (Go runtime's async-preemption SIGURG), the goroutine called t.Fatalf and died via runtime.Goexit without ever sending to eventsChan, so the main test goroutine blocked forever on <-eventsChan. Fix by retrying EpollWait on EINTR and never calling t.Fatalf from a non-test goroutine.

Full analysis

Summary: make -C build/bindings/go test failed — panic: test timed out after 10m0s with the sole live test goroutine parked in TestUcpWorkerEfd at worker_test.go:74.

Root cause: Deadlock caused by a test-harness bug, not by the PR's code. Log timeline proves a hang, not slowness:

  • 02:29:46.4413 — worker_test.go:61: Failed to call epoll_wait interrupted system call
  • 02:39:40.7338 — panic: test timed out after 10m0s

That is a ~9m54s gap with zero output — the entire remaining runtime. The stack trace names the culprit precisely:

goroutine 9 [chan receive, 9 minutes]:
github.com/openucx/ucx/bindings/go/tests.TestUcpWorkerEfd(0xc0000f29c0)
	.../bindings/go/tests/worker_test.go:74 +0x2d4
goroutine 1 [chan receive, 9 minutes]:   # blocked in testing.(*T).Run

Mechanism, from bindings/go/tests/worker_test.go:

  1. Line 59: the worker goroutine blocks in syscall.EpollWait(epfdLocal, events[:], -1). Go's syscall.EpollWait is a raw wrapper with no EINTR retry loop, so any signal delivered to that thread — routinely the Go runtime's own async-preemption SIGURG (Go ≥1.14), and this job runs Go 1.18.2 — makes it return EINTR.
  2. Lines 60–62: err != nil, so t.Fatalf("Failed to call epoll_wait %v", err) fires. This is the line 61 message in the log, confirming EINTR was hit.
  3. t.Fatalf is documented as valid only from the goroutine running the test. From a spawned goroutine it calls runtime.Goexit(), killing only that goroutine — it does not fail/unblock the test.
  4. Line 64 eventsChan <- nevents is therefore never reached, and eventsChan is unbuffered and never closed. The test goroutine at line 74 (nevents := <-eventsChan) blocks forever until the 10m alarm panics the process.

This is a latent flake dating to the test's introduction, not a regression from this PR. The earlier passing tests (TestUcpContext, TestUcpEpTag, TestUcpEpAm, TestUcpListener, TestUcpMmap) all passed, and the PR (agent/fix-rndv-remote-sysdev) touches rendezvous/system-device logic with no relation to worker efd/epoll wakeup. Expect this to reproduce intermittently on unrelated PRs. Note also the leftover gtest/ucx_perftest processes still holding GPU memory in the nvidia-smi dump, which is normal noise from prior stages, not a cause here.

Implicated commit: 2bbf7cfe — "BINDINGS/GO: Ucp endpoint functionality." (Peter Rudenko, 2021-08-02) introduced this test pattern; not the PR commit d093cd3d.

File: bindings/go/tests/worker_test.go:59-64 (blocking receive that deadlocks is line 74)

Suggested fix: Retry on EINTR and make the goroutine always unblock the test:

go func() {
	defer close(eventsChan)   // guarantees line 74 never blocks forever
	...
	var nevents int
	for {
		nevents, err = syscall.EpollWait(epfdLocal, events[:], -1)
		if err != syscall.EINTR {   // retry across preemption signals
			break
		}
	}
	if err != nil {
		t.Errorf("Failed to call epoll_wait %v", err)  // Errorf, not Fatalf
		return
	}
	eventsChan <- nevents
}()

Two independent hardening points: (1) syscall.EINTR from EpollWait is expected under the Go runtime and must be looped over, and (2) replace every t.Fatalf inside the spawned goroutine (lines 39, 44, 51, 61) with t.Errorf + return, since Fatalf/Goexit from a non-test goroutine silently strands the parent. Adding defer close(eventsChan) converts any future goroutine death into an immediate zero-value receive and a clean failure instead of a 10-minute timeout. Optionally guard line 74 with a select + time.After so the failure mode is a fast, self-describing error rather than a whole-job wall-clock kill.

Related: none found — no existing issue/PR matches this signature; worth filing one to track the flake.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/ucp/rndv/rndv_mtype.inl Resolved
Comment thread src/ucp/rndv/rndv_put.c Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@svc-nvidia-pr-review

Copy link
Copy Markdown

Coverage note (not a request): the new test_ucp_proto_mock_mtype_sys_dev cases are instantiated only for rcx_gpu ("rc_x,cuda,rocm"), so they are effectively skipped on hosts without GPU support. The fix's core lane-selection logic is host-agnostic, but there is no host-only mock exercising the get/put mtype device override.

@guy-ealey-morag

guy-ealey-morag commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up on the GPU CI failures: pushed d093cd3d8.

The regression came from mtype lane modeling using the remote lookup’s UNKNOWN system device. The fix keeps the remote key UNKNOWN for namespace safety, while using the local staging fragment’s device for GET/PUT mtype lane probing.

On the exact failed CI merge, both affected pipeline-abort tests and the namespace test passed (9/9). The trace confirms the staging GPU ordinal is now used. Fresh Azure CI is pending.

@jeffnvidia Why is the local staging fragment’s device always used for GET/PUT mtype lane probing instead of doing it only when the sys_dev is UNKNOWN?
It seems that this change is of bigger scope than necessary.

@jeffnvidia

Copy link
Copy Markdown
Contributor Author

@guy-ealey-morag The fragment device is used unconditionally because it describes the buffer touched by the network operation, whereas the original select_param->sys_dev describes the user buffer.

For GET mtype, ucp_proto_rndv_mtype_next_iov() builds the GET IOV from req->send.rndv.mdesc->ptr, then the fragment is copied to the user buffer. For PUT mtype, the user buffer is first copied into that same mdesc and the network PUT IOV is built from it. frag_mem_info.sys_dev is also the device stored in the protocol private data and later passed to ucp_rndv_mpool_get(), so it is the authoritative device for lane modeling.

Limiting the override to UNKNOWN would keep modeling the network lane by the user GPU whenever that GPU is known, even when the staging fragment is host memory on a different device. The new GET/PUT tests deliberately use distinct, known user and fragment accelerator ordinals and equal-bandwidth NICs; they assert the fragment-selected lane, and the conditional behavior would select the opposite lane. The wrapper changes only the copied nested-probe parameters, so the original protocol lookup key remains unchanged.

@guy-ealey-morag

Copy link
Copy Markdown
Contributor

@guy-ealey-morag The fragment device is used unconditionally because it describes the buffer touched by the network operation, whereas the original select_param->sys_dev describes the user buffer.

For GET mtype, ucp_proto_rndv_mtype_next_iov() builds the GET IOV from req->send.rndv.mdesc->ptr, then the fragment is copied to the user buffer. For PUT mtype, the user buffer is first copied into that same mdesc and the network PUT IOV is built from it. frag_mem_info.sys_dev is also the device stored in the protocol private data and later passed to ucp_rndv_mpool_get(), so it is the authoritative device for lane modeling.

Limiting the override to UNKNOWN would keep modeling the network lane by the user GPU whenever that GPU is known, even when the staging fragment is host memory on a different device. The new GET/PUT tests deliberately use distinct, known user and fragment accelerator ordinals and equal-bandwidth NICs; they assert the fragment-selected lane, and the conditional behavior would select the opposite lane. The wrapper changes only the copied nested-probe parameters, so the original protocol lookup key remains unchanged.

Okay I understand.
Can you update the PR description with these details?

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/ucp/rndv/proto_rndv.c Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@svc-nvidia-pr-review

Copy link
Copy Markdown

test/gtest/ucp/test_ucp_proto_mock.cc

minor: pls skip instead of asserting here (on the ASSERT_NE(UCS_SYS_DEVICE_ID_UNKNOWN, cuda_mem_info.sys_dev); line) — on hosts where a CUDA allocation has no topology identity this becomes a hard test failure rather than an environment skip, unlike the other prerequisite checks in this class.

        if (cuda_mem_info.sys_dev == UCS_SYS_DEVICE_ID_UNKNOWN) {
            UCS_TEST_SKIP_R("CUDA buffer has no topology device identity");
        }

@tvegas1 tvegas1 left a comment

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.

confirmed tests fail proto without change

@jeffnvidia

Copy link
Copy Markdown
Contributor Author

@gleon99 @rakhmets All review conversations are resolved, Thomas approved the current head, and CI is fully green. If you can, could you please merge the PR or let me know if anything else is required? Thank you!

@tvegas1
tvegas1 merged commit 0311ed7 into openucx:master Sep 14, 2026
162 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants