UCT/IB: Add refcounted, TTL-based address handle cache - #11845
Conversation
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().
|
🤖 Starting review — findings will be posted here when done. |
| uct_ud_verbs_ep_release_ah(self); | ||
| } | ||
|
|
||
| static void uct_ud_verbs_ep_destroy(uct_ep_h tl_ep) |
There was a problem hiding this comment.
blocker — uct_ud_ep_disconnect() does not free the endpoint synchronously; it only sets close_time / UCT_UD_EP_FLAG_DISCONNECTED and defers the real ep_free() (which runs the uct_ud_verbs_ep_t cleanup) to the timer via uct_ud_ep_free_by_timeout() after linger_timeout. But the new destroy releases the AH immediately:
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));
}During the linger window the endpoint stays alive and can still retransmit its TX window. uct_ud_verbs_post_send() reads ep->ah_entry->ah, but uct_ud_verbs_ep_release_ah() has already set ep->ah_entry = NULL, so a resend dereferences NULL. It also drops the AH cache reference while the ep may still be sending, so with a short/zero TTL the underlying ibv_ah can be destroyed and reused by the device. The AH release must happen when the ep is actually freed (in the cleanup func), not right after uct_ud_ep_disconnect(). The cleanup func already calls uct_ud_verbs_ep_release_ah(), so this extra release in destroy looks unsafe.
Suggested question to the author: can we drop the AH release from uct_ud_verbs_ep_destroy and rely solely on the cleanup func, since the ep can still resend during the disconnect linger period?
| kh_foreach_value(&dev->ah_hash, ah, ibv_destroy_ah(ah)); | ||
| uct_ib_ah_entry_t *entry; | ||
|
|
||
| kh_foreach_value(&dev->ah_hash, entry, { |
There was a problem hiding this comment.
The new cleanup both asserts refcount == 0 and warns + destroys when it is non-zero:
ucs_assertv(entry->refcount == 0, ...);
if (entry->refcount != 0) {
ucs_warn(...);
}
ibv_destroy_ah(entry->ah);
ucs_free(entry);If any live holder still references the entry at device cleanup (which the blocker above can produce), this frees the entry and destroys the AH out from under it. The assert catches it in debug builds, but release builds warn and then continue into a use-after-free for the holder. Worth confirming the lifetime guarantees so this path is truly unreachable, otherwise the leak-vs-UAF tradeoff needs a decision.
|
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)
|
3 participants
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. It avoids for instance a lingering UD endpoint pinning a stale AH.
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.