UCT/IB: Add configurable TTL knob for the AH cache - #11847
Conversation
Add UCX_IB_AH_CACHE_TTL (0 or inf) to bypass or keep the address handle cache. All non-SRD AH creation call sites (UD, RC DEVX QP connect, DC pure grant, compact AV probe) honor the setting, SRD always uses the cache directly since it needs a stable AH pointer for is_connected().
|
🤖 Starting review — findings will be posted here when done. |
|
minor: The AH_CACHE_TTL option is typed as TIME_UNITS and named TTL, but only 0/inf are legal and any real duration is rejected at MD open. That is a surprising UX (a "TTL" that accepts no time value). Config naming/UX is never a blocker, so this is just a note — consider clarifying in the doc string that no intermediate values are supported yet (the doc already says "No other values are allowed", which is adequate). Coverage note: No test covers the UCX_IB_AH_CACHE_TTL=0 path through SRD / dc-mlx5 FC-grant / rc-devx connect AH release (i.e., that the uncached AHs are actually destroyed and not leaked). This relies on existing transport tests running under a non-default config; if CI does not run a job with IB_AH_CACHE_TTL=0, leak/lifetime regressions in those release sites would go uncaught. Worth confirming a CI job or valgrind run exercises the disabled-cache path across RC/DC/UD/SRD, or adding one. |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
| typedef struct { | ||
| uct_ud_ep_t super; | ||
| uct_ud_verbs_ep_peer_address_t peer_address; | ||
| struct ibv_ah *ah; |
There was a problem hiding this comment.
it increases ep size, maybe we can fix just mlx5 transports in this Pr?
| int uct_ib_iface_prepare_rx_wrs(uct_ib_iface_t *iface, ucs_mpool_t *mp, | ||
| uct_ib_recv_wr_t *wrs, unsigned n); | ||
|
|
||
| /* Honors UCX_IB_AH_CACHE_TTL=0 to bypass the AH cache */ |
There was a problem hiding this comment.
This comment seems out of place
| ucs_offsetof(uct_ib_md_config_t, ext.direct_nic), UCS_CONFIG_TYPE_BOOL}, | ||
|
|
||
| {"AH_CACHE_TTL", "inf", | ||
| "Address handle (AH) cache: 0 disables it, inf keeps it enabled. No\n" |
There was a problem hiding this comment.
Maybe disables it when possible? SRD still has the cache
| return uct_ib_device_create_ah_cached(uct_ib_iface_device(iface), ah_attr, | ||
| uct_ib_device_t *dev = uct_ib_iface_device(iface); | ||
|
|
||
| /* ah_cache_ttl=0 (UCX_IB_AH_CACHE_TTL) bypasses the cache */ |
There was a problem hiding this comment.
This comment is not useful
| {"DIRECT_NIC", "y", "Use Direct NIC functionality for GPU memory access", | ||
| ucs_offsetof(uct_ib_md_config_t, ext.direct_nic), UCS_CONFIG_TYPE_BOOL}, | ||
|
|
||
| {"AH_CACHE_TTL", "inf", |
There was a problem hiding this comment.
Why TTL? Maybe just AH_CACHE_ENABLED?
| return UCS_OK; | ||
| } | ||
|
|
||
| static void uct_ud_verbs_ep_destroy_ah(uct_ud_verbs_ep_t *ep) |
There was a problem hiding this comment.
Consider naming uct_ud_verbs_ep_release_ah
4 participants
What?
Add
UCX_IB_AH_CACHE_TTL:0creates AHs uncached,infkeeps them cached (no eviction).Why?
The AH cache speeds up connection setup, but its underlying L2 resolution can become invalid (RoCE), leaving a stale cached entry.
How?
Users of the TTL configuration (cached or uncached):
Users of the always-cached path:
Added the configuration knob and tests.