Skip to content

UCT/UD: Add generation ID to dest_id to make it unique - #11800

Merged
brminich merged 9 commits into
openucx:masterfrom
iyastreb:iyastreb/unique_dest_id
Aug 25, 2026
Merged

brminich merged 9 commits into
openucx:masterfrom
iyastreb:iyastreb/unique_dest_id

Conversation

@iyastreb

@iyastreb iyastreb commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor

What?

Introduce generation ID to the UD dest_id, in order to avoid reusing stale dest_id of dropped connections

Why?

UD line uses dest_id as a connection identifier, which is equal to ep_id = slot in the EPs array.
When certain UD EP gets destroyed, its ep_id might be reused by a subsequent connection.
This is a problem, because remote side may still not know that connection is destroyed and recreated, and either keeps sending keep-alive packets, or stale ACKs come late due to switch problem.

How?

Steal 2 bits from dest_id for always increment generation id.
This reduces the max possible amount of UD EPs created from 16mln to 4mln
Related fix: #11092

@iyastreb
iyastreb marked this pull request as ready for review August 20, 2026 09:11
@iyastreb iyastreb changed the title UCT/UD: Unique dest_id UCT/UD: Add generation ID to dest_id to make it unique Aug 20, 2026
Comment thread src/uct/ib/ud/base/ud_iface.h Resolved
Comment thread src/uct/ib/ud/base/ud_ep.c Outdated
ucs_trace("iface %p: dropping packet with dest_id %u", iface, dest_id);
goto out;
} else if (ucs_unlikely(ep->ep_id != dest_id)) {
ucs_debug("iface %p: dropping packet of a closed connection, dest_id %u"

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.

maybe also print generations?

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.

We can do that, but then we need to update all the places where we print dest_id I think
So we can have something like:

dest_id 0x40007b(gen_id:1, ep_index:123)

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.

Added several logging macros helpers, here is the log example it produces

ep=0x76464c919810(id=0x71(gen_id=0 ep_index=113), conn_sn=22) -> dest_ep_id=0x293(gen_id=0 ep_index=659)

@iyastreb
iyastreb force-pushed the iyastreb/unique_dest_id branch from d8b5c62 to 8a7f223 Compare August 21, 2026 14:09
Comment thread src/uct/ib/ud/base/ud_ep.h Resolved
Comment thread src/uct/ib/ud/base/ud_ep.h Resolved
Comment thread src/uct/ib/ud/base/ud_ep.h Resolved
Comment thread test/gtest/uct/ib/ud_base.cc Resolved

ep_index = ucs_ptr_array_insert(&iface->eps, ep);
ep_gen = ucs_array_elem(&iface->ep_gen, ep_index);
ep->ep_id = (ep_gen << UCT_UD_EP_INDEX_BITS) | ep_index;

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.

minor: since we have one liner helper uct_ud_ep_id_to_index maybe add the same for ep_id by gen + index here and similar for gen in uct_ud_iface_remove_ep

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.

I had it initially, but then removed. It is used in just one place, so I preferred to have an explicit assignment

@svc-ucx

svc-ucx commented Aug 25, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests gpu on worker 3) · commit 0a7188b2

TL;DR: The GPU job failed on a single gtest case, rc_gda/test_ucp_device_xfer.put/11 (cuda/warp/lazy_without_req), which exercises the CUDA/GDA deferred‑doorbell (non‑NODELAY) put path — an area untouched by this PR (#11800 is a UD‑only dest_id change), so this is an unrelated/flaky GDA failure; re‑run the GPU worker and, if it reproduces, triage the GDA lazy doorbell path rather than the UD change.

Full analysis

Summary: make test in build-test/test/gtest exited with error 1 because exactly 1 of 10195 tests failed: rc_gda/test_ucp_device_xfer.put/11, where GetParam() = rc,rc_gda,cuda_copy,rocm_copy/cuda/warp/lazy_without_req.

Root cause: From the log, the only failure is the lazy_without_req variant of the device put test at warp level. Its siblings on the identical transport/memory configuration (nodelay_with_req, nodelay_without_req, multi_channel, and the thread‑level lazy_without_req) all passed, and the whole rest of the suite (10174 tests) passed — so this is not a broad regression but a failure isolated to the deferred‑doorbell GDA send path. That path is selected in test/gtest/ucp/test_ucp_device.cc:620-623 (with_no_delay = false; with_request = false), and in the kernel (test/gtest/ucp/cuda/test_kernels.cu:136-147) the first num_iters-1 puts are posted with flags == 0 and req_ptr == nullptr, i.e. no completion is tracked and no doorbell is rung; only the final iteration is forced to UCT_DEVICE_FLAG_NODELAY with a request, and each of the 4 warps in the block waits only on its own last operation while all warps share channel_id == 0 (params.num_channels = 1, test_kernels.cu:29-31). Correctness therefore depends entirely on the shared‑channel deferred doorbell / completion logic in the GDA transport, which was reworked recently (1ce08f6e "UCT/D2P: Multi-channel support with per-EP DPA address and RING_DB flag (#11701)").

Importantly, PR #11800 is titled "UCT/UD: Add generation ID to dest_id to make it unique" — a UD‑transport change. This test runs with UCX_TLS=rc,rc_gda and the failure is in the CUDA device data path, not in wireup (the mem‑list creation at test_ucp_device.cc:202-212 succeeded, and every UD/UDX test suite in the log passed). There is no evidence linking the PR to this failure.

Note: the assertion text itself (whether ASSERT_UCS_OK(result.status) at test_ucp_device.cc:538 or a dst_pattern_check mismatch at test_ucp_device.cc:744-746) is earlier than the ~500‑line tail the Azure log API returns, so the exact failing expectation could not be read.

Implicated commit: unknown for the failure itself; the PR under test (0a7188b, iyastreb/unique_dest_id) is not implicated. Most likely area of pre‑existing instability: 1ce08f6e — Artemy Kovalyov, "UCT/D2P: Multi-channel support with per-EP DPA address and RING_DB flag (#11701)".

File: test/gtest/ucp/cuda/test_kernels.cu:136-147 (lazy iterations posted with no completion/doorbell), test definition test/gtest/ucp/test_ucp_device.cc:730-747, variant setup test/gtest/ucp/test_ucp_device.cc:620-623

Suggested fix:

  1. Re‑run the "gpu on worker 3" job; this single‑variant GDA failure is not attributable to the UD dest_id change and should not block PR UCT/UD: Add generation ID to dest_id to make it unique #11800.
  2. To get a diagnosable log next time, re‑run with the full task log (the tail truncates the gtest assertion) or have the GPU stage upload the gtest output as a build artifact.
  3. If it reproduces, open a separate issue against the GDA lazy/deferred‑doorbell path and harden the test: post every iteration with a request (or add an explicit device‑side flush after the loop in ucp_test_kernel_job) instead of relying solely on the final UCT_DEVICE_FLAG_NODELAY op to flush operations queued by other warps sharing channel_id 0, and check the status of each lazily posted operation rather than discarding it.

Related: #11800 (the PR under test — UD dest_id, unrelated to the failure); #11701 (RING_DB / multi‑channel rework touching the lazy doorbell path); #11781 (most recent gdaki change before this build)

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.

4 participants