UCT/IB: Add refcounted, TTL-based address handle cache - #11845
Conversation
|
🤖 Starting review — findings will be posted here when done. |
|
Coverage note: The added gtests ( |
|
🤖 CI Triage Agent — TL;DR: The UD-verbs async progress thread segfaults dereferencing Full analysisSummary: Root cause: Both backtraces are identical and deterministic: Line 119 is
static void uct_ud_verbs_ep_destroy(uct_ep_h tl_ep)
{
uct_ud_ep_disconnect(tl_ep);
uct_ud_verbs_ep_release_ah(ucs_derived_of(tl_ep, uct_ud_verbs_ep_t)); /* sets ah_entry = NULL */
}( This is unsafe for UD: Secondary defect: Implicated commit: File: Suggested fix:
Related: none found (no existing issue/PR matches this signature)
|
Replace the AH cache's simple create-or-lookup semantics with refcounted entries that carry an idle timestamp. uct_ib_device_ah_get() shares an entry while it's referenced and re-queries it once idle past the new UCX_IB_AH_CACHE_TTL (default inf; 0 disables the cache and hands out private, unshared AHs). Callers release with ah_put(), or duplicate a reference with ah_hold() for independently-released holders. Port PR openucx#11825's identity-based is_connected()/conn_match for UD verbs (compare dlid/gid instead of AH pointers) onto the refcounted cache, and give EFA/SRD the same treatment since it also holds an AH for the life of the endpoint. Other AH consumers (UD mlx5, RC mlx5 DEVX, DC pure-grant, mlx5 compact-AV probe) switch to a transient get+put around uct_ib_mlx5_get_av().
8247b97 to
0df03aa
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Compilation of Root cause: The new refcounted/TTL AH cache added Implicated commit: File: Suggested fix: Initialize the pointer at declaration so the compiler can never see an unset path: struct ibv_ah *ah = NULL;Optionally also tidy the duplicated cleanup in that block (lines 1528–1542) so the AH/entry teardown happens in one place, e.g. build the entry with Related:
|
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The new gtest Full analysisSummary: "Run gtests" step of EFA on rhel90_ib failed with 2 failures out of 8330: Root cause: In
On the stale path in Implicated commit: [REDACTED:Hex High Entropy String] — "UCT/IB: Add refcounted, TTL-based address handle cache", Thomas Vegas (also 0df03aa, same series) File: test/gtest/uct/ib/test_ib.cc:405 ( Suggested fix:
Related: PR #11845 (this PR); prior related PR #11847 "UCT/IB: Add configurable TTL knob for the AH cache" (c849510)
|
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The RoCE gtest shard hung — the very first test's connection handshake ( Full analysisSummary: Root cause: This is a hang, not a slow test. Timeline from the log: test starts 14:41:23 → 3 min of total silence → 14:44:24 Implicated commit: c4b52d9 (with its predecessor 0df03aa), Thomas Vegas — “UCT/IB: Add refcounted, TTL-based address handle cache” (plus the knob added in c849510 / #11847) File: src/uct/ib/base/ib_device.c:1477-1552 ( Suggested fix:
|
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The Coverity stage failed with 2 new Full analysisSummary: Root cause: In Implicated commit: Defect site is pre-existing ( File: Suggested fix: Check the status at both call sites, matching the style used a few lines below: ASSERT_UCS_OK(uct_iface_get_address(m_e1->iface(),
(uct_iface_addr_t*)(void*)&if_adr1));
ASSERT_UCS_OK(uct_iface_get_address(m_e2->iface(),
(uct_iface_addr_t*)(void*)&if_adr2));Add this as a small commit on the Related: PR #11845 (UCT/IB: Add refcounted, TTL-based address handle cache); prior related PR #11847 (AH cache TTL knob).
|
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The "roce on worker 2" job did not fail on any test or compile error — the Azure DevOps agent on Full analysisSummary: Root cause: Infrastructure/agent-level cancellation, not a product or test failure. The log shows a clean, continuously progressing build (
There is no compiler error, no Implicated commit: none — commit [REDACTED:Hex High Entropy String] is not implicated; the failure precedes any code execution. File: N/A (no source file implicated; cancellation reported in the Suggested fix: Re-trigger the "UCX PR" pipeline for PR #11845 (or just the "roce on worker 2" job). If it recurs on the same host, ask CI ops to check the health/uptime of the Related: PR #11845 (UCT/IB: Add refcounted, TTL-based address handle cache) — the PR under test; no related issue found for this cancellation signature.
|
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: No UCX test actually failed — the self-hosted Azure agent running the Full analysisSummary: The "Run gtests" step of the EFA_Tests job was canceled at 21:02:21 by an agent shutdown signal, not by a failing assertion or crash. Root cause: Infrastructure/agent abort. The log shows continuous, healthy test progress with no stalls — the last ~4 minutes contain back-to-back Implicated commit: unknown — not a code regression; File: unknown (no test failure or source line reported) Suggested fix: Re-run the Related: PR #11845 (UCT/IB: Add refcounted, TTL-based address handle cache) — the PR under test; no related failure issue found.
|
- Reuse the existing entry variable in the stale eviction path instead of repeating the kh_value() lookup. - Override create_connected_entities() in test_uct_ib_ah_cache, matching test_uct_ib_perf, so init() is not duplicated. - Drive the AH refcount wait with short_progress_loop(), like wait_for_ep_destroyed(), so linger timers are swept by progress.
|
🤖 Starting review — findings will be posted here when done. |
Destroying the AH and freeing the entry unconditionally left any remaining holder with a dangling pointer, and a double free on its ah_put(). Drop only the cache's own reference, so a still-referenced entry stays valid.
Now that cached AHs can be destroyed, releasing an AH while the device may still consume a posted WQE would let the HW read a freed address handle. Give a send operation ownership of one AH reference whenever it outlives its sender endpoint: - ep send op purge takes a reference for each orphaned operation, released when its send completion is polled. - A posted control operation hands its reference to the send op, instead of releasing it right after the post. - iface cleanup releases those references only after the QP is destroyed.
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: From the evidence available, this is an isolated failure in the multi-fragment eager AM path with I could not attribute it to the PR: the changes on Caveat, stated explicitly: the gtest failure message/assertion for the failing case is not in the retrievable log — the Azure task log is truncated to the last ~500 lines (last ~70 seconds of a 57-minute shuffled run), so I cannot show whether it was a data-pattern mismatch, a counter mismatch, or a Implicated commit: unknown — no log evidence links the failure to File: test/gtest/ucp/test_ucp_am.cc:1710 (failing case; randomized seg size at test/gtest/ucp/test_ucp_am.cc:1674) Suggested fix: 1) Re-run the Related: PR #11845 (this PR, "UCT/IB: Add refcounted, TTL-based address handle cache"); no existing issue found for |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The RoCE ASan gtest job hung for 11+ minutes inside Full analysisSummary: Root cause: Log evidence: last activity at That is exactly the machinery this branch rewrites:
The commit Implicated commit: File: Suggested fix:
Related: PR #11845 (this PR), PR #11847 "UCT/IB: Add configurable TTL knob for the AH cache"
|
|
🤖 Starting review — findings will be posted here when done. |
Route every AH destruction through a helper that warns on failure, as destructors cannot propagate a status to the caller.
|
🤖 Starting review — findings will be posted here when done. |
|
Tested gtest on EFA UD/SRD with: |
What?
ud_vandsrdhold a long-term AH reference. The transports (rc_x,ud_x,dc) release any reference right away as they mainly need an AV for connect.ud_vandsrdfunctionsis_connected()/ conn_match: compare peer identity (dlid/gid), not AH pointer value.Important: TTL=0 means full cache bypass, no refcnt.
Why?
The AH cache had no lifecycle, entries were never re-resolved or reclaimed, and
is_connected()relied on that by comparing AH pointers directly. Adding refcnt/TTL-based eviction makes pointer comparison unsafe for TTL=0, so identity-based comparison is needed too.Cache use-case: quickly reconnecting endpoints speedup.