Repository navigation
Conversation
|
🤖 CI Triage Agent — TL;DR: The BlueField gtest aborted on a new debug assertion Full analysisSummary: Root cause: The PR commit adds, in Implicated commit: b81d5f1 "UCP/PROTO: Initialize short thresholds from protocol selection" — Yossi Itigin (HEAD of PR #11817) File: src/ucp/proto/proto_select.c:486 (and the guarded init at src/ucp/proto/proto_select.c:514-518) Suggested fix: Do not assume non-async context in
Option 1 is preferable since it also removes the Related: PR #11817 (this PR); protocol selection re-entrancy note added in the same file at proto_select.c:575-581 |
b81d5f1 to
bafc4ec
Compare
|
🤖 Starting review — findings will be posted here when done. |
bafc4ec to
554bec5
Compare
|
|
||
| /* If we are still in the wireup phase, add the request to the pending queue | ||
| * of the AM lane. This avoids full protocol lookup from async thread. */ | ||
| if (ucp_wireup_ep_test(ucp_ep_get_lane(ucp_ep, am_lane))) { |
There was a problem hiding this comment.
AM lane redirect assumes the AM lane exists and is a wireup EP; verify the non-proto case.
The new block runs for all replayed requests, including non-proto (protov1) requests:
if (ucp_wireup_ep_test(ucp_ep_get_lane(ucp_ep, am_lane))) {
req->send.lane = am_lane;
ucp_request_pending_add(req);
return;
}A request originally pending on a non-AM lane (e.g. an RMA/AMO lane wireup EP) gets its send.lane rewritten to am_lane and re-queued there. For protov1 requests whose send.uct.func targets a specific lane's resource, moving them to the AM lane can send on the wrong lane once replayed. Can we confirm this path is only reachable for requests that are lane-agnostic at this point, or restrict the redirect to protov2 (proto_request_reset/PROTO_SEND) requests? A one-line justification in the comment would help.
| @@ -4793,7 +4799,6 @@ void ucp_ep_set_cfg_index(ucp_ep_h ep, ucp_worker_cfg_index_t cfg_index, | |||
| ucs_trace("ep %p: set cfg_index %u -> %u", ep, ep->cfg_index, cfg_index); | |||
| ep->cfg_index = cfg_index; | |||
| ep->am_lane = ucp_ep_config(ep)->key.am_lane; | |||
There was a problem hiding this comment.
Removing the proto-init call from ucp_ep_set_cfg_index changes when short thresholds are ready.
Previously short thresholds were initialized synchronously when the cfg index was set. Now they are only initialized on the first ucp_proto_select_lookup_slow. Any protov2 code path that reads config->tag.max_eager_short / am_u.max_eager_short before a proto-select lookup has occurred will observe the uninitialized/default value (set by ucp_ep_config_init_short_thresh). The new test only covers the tag-send-then-check ordering. Is there a path (e.g. ucp_ep_print_info, or a first send that hits the short fast-path before any slow lookup) that reads these thresholds without first triggering ucp_proto_select_lookup_slow? Worth confirming the short fast-path in tag_send.c/ucp_am.c cannot fire before the lazy init runs.
| tag, ¶m); | ||
|
|
||
| /* Expect short-circuit path initialization to be done after first send */ | ||
| ep_config = &ucs_array_elem(&worker()->ep_config, sender().ep()->cfg_index); |
There was a problem hiding this comment.
Test reads cfg_index right after send without guarding against immediate completion.
ep_config is fetched via sender().ep()->cfg_index after ucp_tag_send_nbx. This is fine, but the test asserts SHORT_INITIALIZED is set purely as a side effect of the send. If the send is satisfied by a path that does not go through ucp_proto_select_lookup_slow (e.g. served from the proto-select cache populated elsewhere), the assertion could become flaky. Minor: consider asserting via an explicit lookup path or documenting why the first send is guaranteed to hit the slow lookup.
|
🤖 CI Triage Agent — TL;DR: Full analysisSummary: Azure job "althca on worker 0" — gtest Root cause: In proto-v2, UCP worker ifaces are activated lazily from protocol selection: This PR replaced that with a lazy call: Implicated commit: File: src/ucp/proto/proto_select.c:555-556 (lazy init call site) and src/ucp/core/ucp_worker.c:2225-2241 (removed eager init); failing assertion at test/gtest/ucp/test_ucp_sockaddr.cc:3333 Suggested fix: Don't rely on protocol selection for iface activation. Either:
Also update/remove the now-misleading comment in Related: PR #11817 (this change); prior art on the activation counter: PR #9650, PR #7819, issue #7964.
|
|
🤖 Starting review — findings will be posted here when done. |
| ucp_proto_select_key_t key; | ||
| khiter_t khiter; | ||
|
|
||
| ucs_assert(!ucs_async_is_from_async(&worker->async)); |
There was a problem hiding this comment.
blocker: The assertion ucs_assert(!ucs_async_is_from_async(&worker->async)) is placed at the top of the inline ucp_proto_select_lookup, so it fires on every lookup, including the common cache/hash-hit fast path that only reads an already-initialized entry (which is safe from any thread). The intent is to guarantee protocol initialization (ucp_proto_select_lookup_slow → ucp_proto_select_elem_init, which recursively modifies the hash) never runs from the async thread — and that is correctly guarded by the separate assert in proto_select.c:486. Sends can legitimately reach this inline lookup from an async-thread context in signal mode with the protocol already selected, e.g. ucp_proto_request_restart() → ucp_proto_request_init() → ucp_proto_request_lookup_proto() on a failover/completion callback (ucp_proto_request_zcopy_complete_cb). Those will hit the cache/hash and trip this assertion in assert-enabled builds. The inline assert should only cover the case where the slow path is actually taken (i.e. move it into the else branch just before calling ucp_proto_select_lookup_slow), matching the .c guard.
| ucs_assert(!ucs_async_is_from_async(&worker->async)); | |
| UCS_STATIC_ASSERT(sizeof(key.param) == sizeof(key.u64)); | |
| key.param = *select_param; |
(remove the top-level assert; the slow-path assert in ucp_proto_select_lookup_slow/ucp_proto_select_elem_init already guards actual initialization)
| ucs_debug("ep %p: adding request %p to pending of lane %d", ucp_ep, req, | ||
| am_lane); | ||
| req->send.lane = am_lane; | ||
| ucp_request_pending_add(req); |
There was a problem hiding this comment.
In the new protov2 replay branch, the return value of ucp_request_pending_add(req) is ignored. For a wireup-EP lane ucp_wireup_ep_pending_add() always queues and returns UCS_OK for non-ucp_wireup_msg_progress requests, so today this is safe, but the pattern is fragile: if pending_add ever returns 0 (BUSY) here, the extracted request is silently dropped (it was already removed from the queue by ucs_queue_for_each_extract). Consider asserting the success (e.g. ucs_assert(... == 1)) to document and enforce this invariant.
|
|
||
| - For any file you touch, read the nearest `AGENTS.md` and each parent guide up | ||
| to this root guide before editing. | ||
| - For tasks that inspect, diagnose, review, or modify UCX source code, use |
There was a problem hiding this comment.
The added guidance mandates using a specific external tool (codegraph_explore / codegraph sync) for all source inspection/review/edit tasks. This is a tool-specific workflow that, per this repo's own rule ("add tool-specific adapters only when a tool requires them" and keep the root guide tool-agnostic), reads as an ownership/tool-coupling concern in the canonical guide. Consider whether this belongs in a tool adapter/skill rather than the root AGENTS.md.
b012851 to
766fe79
Compare
|
🤖 Starting review — findings will be posted here when done. |
| ucp_proto_select_init_protocols_t proto_init; | ||
| ucs_status_t status; | ||
|
|
||
| ucs_assert(!ucs_async_is_from_async(&worker->async)); |
There was a problem hiding this comment.
The same ucs_assert(!ucs_async_is_from_async(&worker->async)) guard is added in both the inline ucp_proto_select_lookup() and inside ucp_proto_select_elem_init() (reached via ucp_proto_select_lookup_slow). The inline one already covers the common send path before it reaches the slow path. The elem_init copy is only non-redundant for direct ucp_proto_select_lookup_slow callers (tests, short-init). Not wrong, but worth a one-line comment on why the guard exists in two places, or dropping the redundant one. Non-blocking.
|
|
||
| /* With protov2, requests are lane-agnostic before protocol lookup, so keep | ||
| * them pending on the AM lane to avoid lookup from async thread. */ | ||
| if (ucp_ep->worker->context->config.ext.proto_enable && |
There was a problem hiding this comment.
The new replay guard ucp_ep->worker->context->config.ext.proto_enable && ucp_wireup_ep_test(ucp_ep_get_lane(ucp_ep, am_lane)) also re-queues non-proto requests (e.g. wireup messages) onto the AM lane's wireup_ep pending queue and rewrites req->send.lane = am_lane. For wireup-msg requests the lane is re-selected in ucp_wireup_msg_progress, so this is harmless, but the comment only mentions "requests are lane-agnostic before protocol lookup". Consider narrowing the condition to proto requests or clarifying the comment that non-proto requests are also intentionally handled here.
766fe79 to
94f6b7e
Compare
|
🤖 Starting review — findings will be posted here when done. |
| } | ||
|
|
||
| /* Replay pending requests */ | ||
| ucs_trace("ep %p: replaying pending requests", ucp_ep); |
There was a problem hiding this comment.
This added ucs_trace("ep %p: replaying pending requests", ...) duplicates information already logged per-request inside ucp_wireup_replay_pending_request ("replay proto ..."). It's harmless but adds little; can drop it or keep as-is.
| ucp_proto_select_key_t key; | ||
| khiter_t khiter; | ||
|
|
||
| /* Protocol selection should not be triggered from async context, since |
There was a problem hiding this comment.
The assertion ucs_assert(!ucs_async_is_from_async(&worker->async)) is only on the inlined fast-path ucp_proto_select_lookup. Direct callers of ucp_proto_select_lookup_slow (rndv_ppln.c, proto_rndv.c, rma_rndv.c) bypass it, but those are internal recursive selections on the main thread. Was it intentional to leave the guard off ucp_proto_select_lookup_slow (which now also runs lazy_init)? It seems acceptable since the top-level entry is guarded, just confirming.
94f6b7e to
ef0ed42
Compare
|
🤖 Starting review — findings will be posted here when done. |
| khiter_t khiter; | ||
| int khret; | ||
|
|
||
| ucs_assert(!ucs_async_is_from_async(&worker->async)); |
There was a problem hiding this comment.
The ucs_assert(!ucs_async_is_from_async(...)) here duplicates the identical assert added in ucp_proto_select_lookup (proto_select.inl:87). The inline path always calls into this slow path, so for those callers the check runs twice. It's still reachable directly (e.g. ucp_proto_rndv_ctrl_select_remote_proto), so not fully redundant, but consider adding the same short justification comment that the inline version has, for consistency.
| if (ucp_wireup_ep_test(ucp_ep_get_lane(ucp_ep, am_lane))) { | ||
| ucs_debug("ep %p: adding request %p to pending of lane %d", ucp_ep, | ||
| req, am_lane); | ||
| req->send.lane = am_lane; |
There was a problem hiding this comment.
req->send.lane = am_lane overwrites the lane of a PROTO_SEND request before adding it to the wireup_ep pending queue. When this request is later replayed from that queue, ucp_wireup_replay_pending_request runs again; if by then cfg_index == proto_config->ep_cfg_index and proto_request_reset is false, it falls to the else branch and calls ucp_request_send(req) — is send.lane guaranteed to be re-derived from the (possibly changed) proto config at that point, or could the stale am_lane be used? Worth a comment clarifying why the overwrite is safe.
There was a problem hiding this comment.
cfg_index == proto_config->ep_cfg_index will not become true by then since proto_config is not updated with the new config
|
🤖 CI Triage Agent — TL;DR: Full analysisSummary: Azure job "roce on worker 1" failed: gtest watchdog aborted the run ( Root cause: A hang, not a timeout. Log timestamps show continuous activity (every test 0.1–1.7 s) up to The commit under test changes exactly the code path that runs on the first send of that test.
Note: the hung thread's backtrace is not in the log (only the aborting main thread), so the exact mechanism between (1) and (2) cannot be confirmed from the log alone. Implicated commit: [REDACTED:Hex High Entropy String] — "UCP/PROTO: Initialize short thresholds from protocol selection", Yossi Itigin (the same commit touches File: Suggested fix:
Related: PR #11817 (the change under test). No pre‑existing issue found for
|
| khiter_t khiter; | ||
|
|
||
| /* Protocol selection should not be triggered from async context, since | ||
| it may race with protocol table updates from main thread */ |
There was a problem hiding this comment.
I understand that the fix makes sure we don't call select_lookup_slow in async, to avoid thread race.
But there is also possibility for rkey_config pointer invalidation due to recursive select_lookup_slow calls, like described in #11802? Or you think we can assume rkey_config array is already fully populated and not subject to such realloc?
was similar: #11676 (async covered by lock and rkey_config with one level of indirection to make it stable)
There was a problem hiding this comment.
rkey configuration can happen only from protocol initialization or rkey_unpack (API or protocol flow)
the recursive rkey is a related but different problem: IMO the callers in protocol init flow should not hold a pointer to rkey config but always lookup using an index.
There was a problem hiding this comment.
do we want to keep rkey_config in one contiguous array? there is proposal to not store array of rkey_config, but instead array of rkey_config pointers (#11676).
There was a problem hiding this comment.
@tvegas1 @yosefe Yes, this is the remaining design choice. #11676 makes stable rkey-config addresses an invariant, while #11802 keeps contiguous storage and re-fetches configurations by index after potentially recursive operations.
The forced 32→33 relocation confirms that #11817 alone does not solve the synchronous invalidation: it failed 5/5, while #11817 combined with #11676 passed 5/5.
Do you prefer the stable-storage approach in #11676 or the index/re-lookup approach in #11802? I can adapt based on that decision.
There was a problem hiding this comment.
Fable5 analysis:
The lazy init makes this worse: when proto_select points into worker->rkey_config(RMA send, or put_short init in ucp_worker_add_rkey_config), the tag/am selection it triggers can add rkey configs via the RTS probe and grow the array, so the caller keeps writing to the old copy (e.g. put_short is lost and short put stays disabled).
Can we run the lazy init only when rkey_cfg_index == UCP_WORKER_CFG_INDEX_NULL?
Short thresholds are only used by tag/am sends, and &ep_config->proto_select is stable.
|
🤖 Starting review — findings will be posted here when done. |
| ucp_trace_req(req, "replay proto %s", | ||
| req->send.proto_config->proto->name); | ||
| ucp_proto_request_restart(req); | ||
| if (ucp_wireup_ep_test(ucp_ep_get_lane(ucp_ep, am_lane))) { |
There was a problem hiding this comment.
ucp_wireup_ep_test(ucp_ep_get_lane(ucp_ep, am_lane)) — if ucp_ep_get_lane returns NULL (lane not yet created), ucp_wireup_ep_test will dereference NULL and crash. In the replay context (ucp_wireup_eps_progress), the ep has just gone through wireup and lanes should be set, but the surrounding code uses NULL-safe helpers (ucp_wireup_ep, ucp_ep_is_lane_failed_stub). If a NULL lane is possible, use ucp_wireup_ep(...) != NULL (NULL-safe) instead. Soft question about reachability.
| ucp_proto_select_key_t key; | ||
| khiter_t khiter; | ||
|
|
||
| /* Protocol selection should not be triggered from async context, since |
There was a problem hiding this comment.
ucs_assert(!ucs_async_is_from_async(&worker->async)) — potential false positive on RNDV receive path. RNDV receive-side handlers (ucp_rndv_rts_handler → ucp_proto_rndv_receive_start → ucp_proto_rndv_send_reply → ucp_proto_request_lookup_proto → ucp_proto_select_lookup, and ucp_proto_rndv_handle_rtr → ucp_proto_rndv_send_start) run from the AM callback, which can be dispatched from the async progress thread (UCT_CB_FLAG_ASYNC). On the first RNDV op for a given select-param this reaches lookup_slow. Confirm that all send-side protocol selection (including RNDV reply/RTR handling) is guaranteed to run only on the main thread; otherwise this assert would be a regression in debug builds.
There was a problem hiding this comment.
rndv callback cannot be dispatch from async thread
| khiter_t khiter; | ||
| int khret; | ||
|
|
||
| ucs_assert(!ucs_async_is_from_async(&worker->async)); |
There was a problem hiding this comment.
ucs_assert(!ucs_async_is_from_async(&worker->async)) — potential false positive on RNDV receive path. RNDV receive-side handlers can be dispatched from the async progress thread (UCT_CB_FLAG_ASYNC), reaching lookup_slow on the first RNDV op for a given select-param. This selection path already existed before this PR, so the new assert may fire on a path that previously worked. Confirm all send-side protocol selection runs only on the main thread.
There was a problem hiding this comment.
rndv callback cannot be dispatch from async thread
|
🤖 Starting review — findings will be posted here when done. |
| @@ -575,13 +581,44 @@ ucp_worker_iface_error_handler(void *arg, uct_ep_h uct_ep, ucs_status_t status) | |||
| return status; | |||
There was a problem hiding this comment.
arm_block_count can leak if a busy-marked iface is deactivated/closed. UCP_WORKER_IFACE_FLAG_ACTIVATE_ARM_BUSY is set in check_events_do when arm returns BUSY, and only cleared in ucp_worker_iface_activate or when arm later succeeds. ucp_worker_iface_deactivate() / ucp_worker_iface_disarm() / iface cleanup do not call ucp_worker_iface_activate_arm_unmark_busy(). If an iface is deactivated or torn down while marked busy, worker->arm_block_count stays > 0 and ucp_worker_arm() returns UCS_ERR_BUSY forever, which can hang a ucp_worker_wait() loop. Confirm the busy flag is always cleared on the deactivate/cleanup paths — can we unmark in ucp_worker_iface_deactivate()/cleanup?
There was a problem hiding this comment.
- deactivate is called only after activate , which already clears BUSY state
- ifaces are destroyed only when worker is destroyed so it does not matter
| @@ -558,6 +558,33 @@ UCS_TEST_P(test_ucp_proto, dump_protocols) { | |||
| ucp_ep_print_info(sender().ep(), stdout); | |||
There was a problem hiding this comment.
test_ucp_proto.ep_config_proto_init_on_send: requests_wait({send_req, recv_req}) is passed pointers that may be UCS_OK (immediate completion, not a valid request pointer) rather than in-progress request handles. Confirm requests_wait tolerates non-pointer status values here; otherwise the test can deref a status code.
| @@ -3767,9 +3767,12 @@ ucs_status_t ucp_ep_config_init(ucp_worker_h worker, ucp_ep_config_t *config, | |||
| &config->tag.rndv.am_thresh); | |||
There was a problem hiding this comment.
Behavior change under protov2: the four ucp_ep_config_set_memtype_thresh() calls are now skipped when proto_enable. This is consistent with protov2 computing short thresholds lazily via ucp_ep_config_proto_short_lazy_init, and the fields were being overwritten anyway, so this is a reasonable cleanup. Noting for scope awareness.
|
Scope note: The PR mixes several logically distinct changes — the proto-short lazy-init move, the wireup pending-replay fix, and the worker arm-busy tracking, plus a large number of trace→debug logging changes. These are related (all touch async-safety of protocol selection / iface activation) but the logging churn makes the functional diff harder to review. Not a blocker. |
| khiter_t khiter; | ||
|
|
||
| /* Protocol selection should not be triggered from async context, since | ||
| it may race with protocol table updates from main thread */ |
There was a problem hiding this comment.
Fable5 analysis:
The lazy init makes this worse: when proto_select points into worker->rkey_config(RMA send, or put_short init in ucp_worker_add_rkey_config), the tag/am selection it triggers can add rkey configs via the RTS probe and grow the array, so the caller keeps writing to the old copy (e.g. put_short is lost and short put stays disabled).
Can we run the lazy init only when rkey_cfg_index == UCP_WORKER_CFG_INDEX_NULL?
Short thresholds are only used by tag/am sends, and &ep_config->proto_select is stable.
|
🤖 Starting review — findings will be posted here when done. |
|
PR scope / logging churn: the large volume of |
|
@iyastreb regarding the comment The issue is real, but not introduced or made worse by this PR, because we still call ucp_proto_select_short_init from ucp_worker_add_rkey_config |
Fixed in 45a0983 |
|
🤖 Starting review — findings will be posted here when done. |
|
@iyastreb fixed |
If check_events_do could not arm the iface (returned BUSY), we can hang forever in ucp_worker_wait because one of the inactive ifaces is not armed to be re-activated and trigger wakeup via worker->eventfd channel.
45a0983 to
2feb274
Compare
|
🤖 Starting review — findings will be posted here when done. |
| } | ||
| } | ||
|
|
||
| /* FIXME proto_select could get invalidated by adding rkey config */ |
There was a problem hiding this comment.
New FIXME documents an unfixed latent bug (question): the cached select_elem points into the hash's value storage, which can be reallocated when a later lookup adds an rkey config. Since this PR is about async-safety of protocol selection, please clarify: is caching a raw select_elem pointer here actually unsafe today, or is it covered by ucp_proto_select_cache_reset() on epoch change? If it's a real hazard, a FIXME alone in an async-hardening PR leaves a known crash/UAF path open — please either scope it out explicitly in the description or file/reference a tracking issue.
|
ucp_ep_config_proto_short_lazy_init no longer honors the protov1 early-out (question): the old ucp_ep_config_proto_init returned early for !proto_enable. The new lazy variant drops that guard and relies solely on being called from ucp_proto_select_lookup_slow (proto_enable-only). This load-bearing invariant isn't asserted. Adding ucs_assert(worker->context->config.ext.proto_enable) at the top of ucp_ep_config_proto_short_lazy_init would make the coupling explicit and catch any future protov1 caller. |
…nitialize-short-thresholds-from-protocol
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The ASAN/roce gtest job failed on a single unrelated UCT test — Full analysisSummary: Root cause: Evidence in the log: the run completed normally ( Implicated commit: unknown — not attributable to 9a7838c (PR #11817); the flaky test area was last touched by 69718f6 (Michal Shalev, "TEST/GTEST/UCT: Add retries and rand sleep when it cannot allocate MEMIC memory") File: test/gtest/uct/test_atomic_key_reg_rdma_mem_type.cc:33 (device-memory buffer alloc) and test/gtest/uct/uct_test.cc:968-998 (retry loop + Suggested fix: Re-trigger the "AddressSanitizer roce on worker 2" job — this failure is unrelated to the PR. If it reproduces, capture the full gtest output for Related: PR #10475 ("TEST/GTEST/UCT: Increase retries and add random sleep when MEMIC allocation fails"), PR #11817 (this PR)
|
What
Avoid protocols initialization from async thread; delay initialization to communication operations slow-path one-shot lazy init on main thread
Why
Fix race condition and segfault - Internal issue 5216089