UCP/PROTO: Initialize short thresholds from protocol selection - #11817
UCP/PROTO: Initialize short thresholds from protocol selection#11817yosefe wants to merge 1 commit into
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)
4 participants
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