Skip to content

UCT/CUDA_IPC: Avoid UUID query for non-local rkeys - #12231

Open
michal-shalev wants to merge 6 commits into
openucx:masterfrom
michal-shalev:cuda-ipc-uuid-query-local-only
Open

michal-shalev wants to merge 6 commits into
openucx:masterfrom
michal-shalev:cuda-ipc-uuid-query-local-only

Conversation

@michal-shalev

Copy link
Copy Markdown
Contributor

What?

Call cuDeviceGetUuid() in uct_cuda_ipc_map_memhandle() only for local rkeys. Adds a gtest for the same-process shortcut.

Why?

map_memhandle runs per put/get and per SGL element, and queried the UUID every time, even for remote rkeys where it's unused.

How?

Check uct_cuda_ipc_is_rkey_local() before the UUID query.

@michal-shalev michal-shalev self-assigned this Nov 4, 2026
@michal-shalev
michal-shalev requested a review from a team as a code owner November 4, 2026 04:21
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread test/gtest/uct/cuda/test_cuda_ipc_md.cc Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_cache.c Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

Residual coverage gap worth noting: no gtest exercises the newly-skipped path (non-local pid reaching uct_cuda_ipc_map_memhandle), since gtest cannot produce a real remote PID in-process. That is acceptable here because the change is a no-op for remote keys other than omitting the query, but it means CI only validates the local branch.

tvegas1
tvegas1 previously approved these changes Nov 4, 2026
Comment thread test/gtest/uct/cuda/test_cuda_ipc_md.cc Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_cache.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

🤖 Review complete — no issues found.

Comment thread test/gtest/uct/cuda/test_cuda_ipc_md.cc Resolved
@svc-ucx

svc-ucx commented Nov 5, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests new on worker 2) · commit 0781c2ee

TL;DR: tcp/test_ucp_rma.put_nonblocking/2 <…/user_memh> aborted on the UCS registration-cache assertion !(region->flags & UCS_RCACHE_REGION_FLAG_PGTABLE) during ucp_mem_unmap() — an rcache region's refcount reached 0 while it was still installed in the page table (an unbalanced ucs_rcache_region_put_unsafe() on the memh's parent region). This is a host-memory/TCP rcache refcount bug and is unrelated to PR #12231's cuda_ipc UUID change.

Full analysis

Summary: gtest crash (core dumped, make: *** [Makefile:4992: test] Aborted) in tcp/test_ucp_rma.put_nonblocking/2 <tcp,cuda_copy,rocm_copy/user_memh> at src/ucs/memory/rcache.c:396.

Root cause: From the backtrace:

ucp_mem_unmap()            ucp_mm.c:1291
 ucp_memh_cleanup()        ucp_mm.c:470
  ucp_memh_put_rcache()    ucp_mm.c:441
   ucs_rcache_region_put_unsafe()      rcache.inl:128
    ucs_mem_region_destroy_internal()  rcache.c:396  <-- assert

A region installed in the rcache page table always carries one reference (region->refcount = 2; /* Page-table + user */, rcache.c:1028; the pgtable reference is only dropped by ucs_rcache_region_invalidate_internal(), which clears UCS_RCACHE_REGION_FLAG_PGTABLE first, rcache.c:492-501). So reaching refcount == 0 with the PGTABLE flag still set means the user reference on the memh's parent region was released one time too many — a refcount underflow/double-put, not a legitimate destroy.

The release path used by UCP is the lockless one: ucs_rcache_region_put_unsafe() (rcache.inl:122-132) does a non-atomic --region->refcount and calls ucs_mem_region_destroy_internal() without holding rcache->pgt_lock in write mode, even though that function is documented "Lock must be held in write mode" (rcache.c:383) and it never removes the region from the page table. Meanwhile the asynchronous invalidation/GC path (ucs_rcache_unmapped_callback → pipe → ucs_rcache_invalidate_handler → ucs_rcache_clean, rcache.c:600-601, 1167-1178) drops the page-table reference with ucs_atomic_fsub32() (rcache.c:450). The two paths are therefore not mutually exclusive, which corrupts refcount and trips exactly this assertion. The test in question hammers map/unmap of growing host buffers (test_message_sizes() loop, test_ucp_rma.cc:180-201) with a user memh, which repeatedly creates/evicts/invalidates regions — i.e. precisely the window needed.

Note on relevance to the PR: this job ran with tcp,cuda_copy,rocm_copy on a node where the log shows [ SKIP ] (CUDA memory is not supported), and the transfer is host→host over TCP. PR #12231 ("UCT/CUDA_IPC: Avoid UUID query for non-local rkeys") only touches cuda_ipc rkey unpacking, which is not exercised here. The failure is pre-existing/flaky, not introduced by this PR.

Implicated commit: unknown — not the PR commit 0781c2ee. Most recent changes to this machinery (candidates, not confirmed): 311e3506 "UCS/RCACHE: Change inv_q type to interval tree (#11248)" (nbellalou) and f3d37492 "UCP/MM: Apply MAX_HCA_PER_GPU policy to GPU memory types (#12150)" (Ankith Averineni).

File: src/ucs/memory/rcache.inl:122-129 (ucs_rcache_region_put_unsafe) → asserts at src/ucs/memory/rcache.c:396; caller src/ucp/core/ucp_mm.c:441 (ucp_memh_put_rcache), reached from src/ucp/core/ucp_mm.c:470.

Suggested fix:

  1. For this PR: treat as an unrelated CI flake — re-run "Tests new on worker 2". Do not block UCT/CUDA_IPC: Avoid UUID query for non-local rkeys #12231 on it.
  2. For the real bug, in ucs_rcache_region_put_unsafe():
    • use ucs_atomic_fsub32(&region->refcount, 1) instead of the plain --region->refcount, and
    • when the count reaches 0, go through ucs_rcache_region_put_internal() with UCS_RCACHE_REGION_PUT_FLAG_TAKE_PGLOCK (or ucs_rcache_region_put()) so the write lock is held while ucs_mem_region_destroy_internal() runs, matching that function's documented contract.
  3. Add a diagnostic to make the double-put obvious instead of crashing far away: in ucp_memh_put_rcache() (ucp_mm.c:425-443) assert memh->super.refcount > 1 || !(memh->super.flags & UCS_RCACHE_REGION_FLAG_PGTABLE) before the put, and log region/refcount so the offending get/put pair can be identified.
  4. To reproduce, run the single test in a loop with rcache tracing, e.g. UCX_LOG_LEVEL=trace UCX_TLS=tcp,cuda_copy,rocm_copy ./gtest --gtest_filter='tcp/test_ucp_rma.put_nonblocking/2', and keep the core dump from the failing worker.

Related: PR #12231 (this PR, unrelated to the failure); nearby rcache/mm changes #11248, #12150, #12104.

@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

🤖 Review complete — no issues found.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@michal-shalev
michal-shalev requested a review from tvegas1 November 5, 2026 08:00
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

@tvegas1
tvegas1 enabled auto-merge (squash) November 5, 2026 08:07
@michal-shalev
michal-shalev enabled auto-merge (squash) November 5, 2026 08:07
@svc-ucx

svc-ucx commented Nov 6, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (AddressSanitizer BlueField on worker 0) · commit 4bd76b17

TL;DR: The ASAN BlueField gtest job did not fail an assertion — it hung: rc/test_ucp_rma.put_nonblocking_iov_zcopy/0 issued ucp_put_nbx with an IOV datatype and then blocked in flush_ep() for 11 minutes until the gtest watchdog aborted ("request … did not complete on time" → SIGABRT). The hang is in the UCP endpoint-flush / multi-lane PUT-zcopy path, not in this PR's CUDA-IPC change, so it is a master-side regression/flake that should be reproduced with UCX_LOG_LEVEL=trace_req rather than treated as a timeout to be raised.

Full analysis

Summary: rc/test_ucp_rma.put_nonblocking_iov_zcopy/0 <rc_v,cuda_copy,rocm_copy/flush_worker> hung inside ucp_ep_flush and was killed by the gtest connection-timeout watchdog (Connection timed out - abort testing, signal 6), failing make test with exit code 2.

Root cause: Log timeline proves a hang, not slowness:

  • 12:26:41 last normal line: [ RUN ] rc/test_ucp_rma.put_nonblocking_iov_zcopy/0
  • 12:37:57 (11 min of silence) ucp_test.cc:310 UCX ERROR request 0xffff7b876420 did not complete on time
  • 12:41:41 test_helpers.cc:57: Failure … Connection timed out - abort testing, then Caught signal 6.

The backtrace shows exactly where it stalled: ucp_test::flush_ep() (ucp_test.cc:203) ← test_ucp_rma::do_nbi_iov() (test_ucp_rma.cc:56) ← test_ucp_memheap::test_xfer(). So after ucp_put_nbx() with DATATYPE_IOV (the test iterates iov_count = 0,1,2,4,…,UCP_MAX_IOV, with ZCOPY_THRESH=0 and modify_config("MAX_RMA_LANES","2")), the endpoint-flush request never completed — i.e. req->send.state.uct_comp.count never reached 0 / flush.sw_done never got set / the new memory-flush phase never called flushed_cb.

This is not plausibly caused by the PR under test: #12231 only touches src/uct/cuda/cuda_ipc/ ("avoid UUID query for non-local rkeys"), and the BlueField worker has no CUDA device (cuda_copy,rocm_copy only appear in the variant's TL-name string; all preceding shm tests skip with "cannot connect to server"). The implicated code was instead reworked on master days before this run:

  • d513f98f — "UCP/FLUSH: Add topology-safe selective endpoint flush (UCP/FLUSH: Add topology-safe selective endpoint flush #11923)", nbellalou, 2026-10-25 — introduced flush.lane_mask and a second flush phase (ucp_ep_flush_mem_start/ucp_ep_flush_mem_progress, flush_sys_dev_map). Note ucp_ep_flush_mem_completion() (flush.c:267) only logs — the UCP flush request is completed solely by a re-posted one-shot poll in ucp_ep_flush_mem_progress() (flush.c:1090-1097); if that one-shot is ever dropped or mem_in_progress never drops to 0, the flush request hangs forever with exactly this signature.
  • 4af0b6b5 — "UCP: Split an SGL put between the lanes by bytes (UCP: Split an SGL put between the lanes by bytes #11947)", Michal Shalev, 2026-10-29 — changed multi-lane PUT splitting, the protocol this test exercises (put/offload/zcopy, max_lanes = max_rma_lanes = 2).

Implicated commit: not this PR (4bd76b17). Prime suspects on the merge base: d513f98f (nbellalou, UCP/FLUSH selective endpoint flush, #11923) and 4af0b6b5 (Michal Shalev, SGL put lane split, #11947).

File: test/gtest/ucp/test_ucp_rma.cc:56 (hang point) → src/ucp/rma/flush.c:1041-1097 (ucp_ep_flush_mem_progress, completion only via re-posted one-shot) and src/ucp/rma/put_offload.c:261-302 (ucp_proto_put_offload_zcopy_send_func / update_remote_flush).

Suggested fix:

  1. Re-run this job (and check whether other PRs on the same merge base show the same hang) to confirm it is a master regression rather than a one-off; do not raise the gtest timeout — the log shows zero activity for 11 min, which is a hang.
  2. Reproduce locally on an IB node with the exact variant and tracing:
    UCX_TLS=rc_v UCX_MAX_RMA_LANES=2 UCX_ZCOPY_THRESH=0 UCX_LOG_LEVEL=trace_req ./gtest --gtest_filter='rc/test_ucp_rma.put_nonblocking_iov_zcopy/0' and look for the last flush update ep … comp_count/flush mem … started=x/y trace to see whether a lane's uct_ep_flush stayed INPROGRESS or the mem-flush phase never re-polled.
  3. In the flush code, make completion event-driven instead of poll-driven: have ucp_ep_flush_mem_completion() free mem->entries, decrement flush_state.mem_in_progress and invoke req->send.flushed_cb(req) (guarding against double completion), rather than relying exclusively on the ucp_ep_flush_mem_resume_callback one-shot. Also add a ucs_assert/trace for the iov_count == 0 (zero-length) PUT case, since ucp_proto_put_offload_zcopy_send_func always passes iovcnt = 1 regardless of how many IOV entries ucp_datatype_iter_next_iov() actually filled.

Related: PR under test: #12231. Suspect master commits: #11923 (d513f98f), #11947 (4af0b6b5), #12091 ([REDACTED:Hex High Entropy String]). Possibly related open flush/MR issue: #12265.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 2f60ad55-21ae-446b-b01f-539ab4900ab1 in the triage console for the audit trail.

This branch has not been deployed

No deployments
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.

5 participants