[evgeny-leksikov]

What?

UCP: Exclude device atomics from failover endpoints.

Why?

NIC hardware atomics have no replay path, so failover lane selection leaves those transports out. Token failover is rejected when IB indirect atomics are enabled; auto selects software PSN recovery.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Build build_icc_pgi centos7_x86_64) · commit 60acc905

TL;DR: The icx (build_icc_pgi) build fails with -Werror on src/ucp/wireup/select.c:1342: the new ucp_wireup_exclude_device_amo_tls() iterates a TL bitmap with the generic UCS_STATIC_BITMAP_FOR_EACH_BIT macro using a ucp_rsc_index_t (uint8_t) index, so the macro's _bit_index < num_words*64 (=256) check is tautologically true. Switch to UCP_TL_BITMAP_FOR_EACH_BIT, which is the TL-bitmap-specific macro designed for ucp_rsc_index_t.

Full analysis

Summary: Compilation of src/ucp/ucp_la-select.lo aborts under Intel icx with -Wtautological-constant-out-of-range-compare promoted to error, failing the build_icc_pgi centos7_x86_64 job.

Root cause: Commit 60acc90 ("UCP: Exclude device atomics from failover endpoints") added:

ucp_rsc_index_t rsc_index;
UCS_STATIC_BITMAP_FOR_EACH_BIT(rsc_index, tl_bitmap) { ... }

ucp_tl_bitmap_t is ucs_static_bitmap_s(UCP_MAX_RESOURCES=255), which rounds up to 4 × 64-bit words. UCS_BITMAP_BITS_FOR_EACH_BIT (src/ucs/datastruct/bitmap.h:76) therefore compares the loop variable against 4*64 == 256, but ucp_rsc_index_t is uint8_t (max 255), so clang/icx flags the comparison as always-true. This is exactly the situation that the immediately preceding commit e57ea1c ("UCP/CORE: Support up to 255 TL resources") handled by introducing UCP_TL_BITMAP_FOR_EACH_BIT/ucp_tl_bitmap_ffs() (src/ucp/core/ucp_types.h:134-156), which clamp to UCP_MAX_RESOURCES and keep the comparison in range. The new code bypassed that helper — note the adjacent loop at select.c:1389 in the same function correctly uses UCP_TL_BITMAP_FOR_EACH_BIT. Only the stricter icx/clang-based build enables this warning as an error, so GCC jobs pass.

Implicated commit: [REDACTED:Hex High Entropy String] — Evgeny Leksikov, "UCP: Exclude device atomics from failover endpoints." (interacts with e57ea1c by Guy Ealey Morag)

File: src/ucp/wireup/select.c:1342 (macro definitions: src/ucs/datastruct/bitmap.h:76, src/ucp/core/ucp_types.h:153)

Suggested fix: In ucp_wireup_exclude_device_amo_tls() replace the generic macro with the TL-bitmap one:

-    UCS_STATIC_BITMAP_FOR_EACH_BIT(rsc_index, tl_bitmap) {
+    UCP_TL_BITMAP_FOR_EACH_BIT(rsc_index, tl_bitmap) {

(Alternatively declare rsc_index as size_t/unsigned, but UCP_TL_BITMAP_FOR_EACH_BIT is the established convention for ucp_tl_bitmap_t in this file.) Worth grepping for any other UCS_STATIC_BITMAP_FOR_EACH_BIT used with a ucp_rsc_index_t variable to avoid the same error elsewhere.

Related: PR #12085 (this change); PR #12054 "UCP/CORE: Support up to 255 TL resources" (introduced UCP_TL_BITMAP_FOR_EACH_BIT)

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id e3fbfe09-9d5b-45c1-aece-f9a7d949f8ee in the triage console for the audit trail.

[svc-nvidia-pr-review]

This only checks that the ib component is registered, not that any IB device/transport is actually used — uct_query_components() returns ib whenever the IB module is loaded, even with no IB devices or with UCX_TLS=tcp. So UCX_FAILOVER_MODE=token now fails ucp_init() on such hosts (that is also why the token gtest needs IB_INDIRECT_ATOMIC=n). Can we base the decision on the MDs/resources actually selected into context->tl_mds/tl_rscs instead?

[svc-nvidia-pr-review]

If device AMO lanes are excluded for every failover endpoint anyway (select.c), why does token mode still have to reject UCX_IB_INDIRECT_ATOMIC=y? Please explain what else indirect atomics affect on the token path.

[svc-nvidia-pr-review]

With device AMO lanes removed, a failover endpoint sends SW atomics, but the target worker still has device atomics selected in worker->atomic_tls, so ucp_atomic_req_handler() prints "got software atomic request while device atomics are selected" and the CPU atomic is not coherent with device atomics used by other endpoints of that worker. How is this handled?

[svc-nvidia-pr-review]

The test only checks lane selection; please also run an actual AMO on the failover endpoint, so the SW atomic path this change forces is covered.

[gleon99]

+1

[evgeny-leksikov]

done

[svc-nvidia-pr-review]

Submit as REQUEST_CHANGES (first finding changes ucp_init() behavior for existing configurations), empty review body.

Residual coverage gap: no CI job appears to exercise UCX_FAILOVER_MODE=token with IB hardware (the token fixture is DISABLED_), so the new reject path and the auto→sw selection are only validated by inspection.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Build build_icc_pgi centos7_x86_64) · commit de87c843

TL;DR: The build_icc_pgi (icx) build failed with -Werror,-Wtautological-constant-out-of-range-compare in src/ucp/wireup/select.c:1342, because the new ucp_wireup_exclude_device_amo_tls() iterates a 255-bit TL bitmap with the generic UCS_STATIC_BITMAP_FOR_EACH_BIT macro using a uint8_t index; switch it to the TL-specific UCP_TL_BITMAP_FOR_EACH_BIT macro.

Full analysis

Summary: Compilation of src/ucp/ucp/wireup/select.c aborted under icx (-O3 -Wall -Werror), failing make[2]: *** [wireup/libucp_la-select.lo] Error 1 and the whole build.

Root cause: The new helper added by this PR:

ucp_rsc_index_t rsc_index;                 /* uint8_t */
UCS_STATIC_BITMAP_FOR_EACH_BIT(rsc_index, tl_bitmap) { ... }

expands (via UCS_BITMAP_BITS_FOR_EACH_BIT in src/ucs/datastruct/bitmap.h:76) to the loop condition _bit_index < (_num_words) * UCS_BITMAP_BITS_IN_WORD. ucp_tl_bitmap_t is ucs_static_bitmap_s(UCP_MAX_RESOURCES=255), i.e. 4 words → the bound is the constant 256. Comparing a ucp_rsc_index_t (uint8_t, max 255) against 256 is always true, so clang/icx emits -Wtautological-constant-out-of-range-compare, which is an error under -Werror. GCC does not emit this warning, which is why only the icc/icx job failed.

This is exactly what commit e57ea1c3 ("UCP/CORE: Support up to 255 TL resources", Guy Ealey Morag) introduced UCP_TL_BITMAP_FOR_EACH_BIT / ucp_tl_bitmap_ffs() for — it clamps to UCP_MAX_RESOURCES (255) and keeps the comparison in range. Note the very next function in the same file (ucp_wireup_add_amo_lanes, line 1389) already uses the correct macro.

Implicated commit: 60acc905 — "UCP: Exclude device atomics from failover endpoints." by Evgeny Leksikov (PR #12085, branch ucp_tf_disables_hw_amo); interacts with e57ea1c3 by Guy Ealey Morag.

File: src/ucp/wireup/select.c:1342 (macro definitions: src/ucs/datastruct/bitmap.h:76, src/ucp/core/ucp_types.h:153)

Suggested fix: Use the TL-bitmap iterator in ucp_wireup_exclude_device_amo_tls():

-    UCS_STATIC_BITMAP_FOR_EACH_BIT(rsc_index, tl_bitmap) {
+    UCP_TL_BITMAP_FOR_EACH_BIT(rsc_index, tl_bitmap) {

(UCP_TL_BITMAP_FOR_EACH_BIT is defined in src/ucp/core/ucp_types.h:153 and is already included via the wireup headers.) Resetting the current bit inside the loop remains safe, since iteration advances via ucp_tl_bitmap_ffs(..., rsc_index + 1). Alternatively, declare the loop variable as size_t/unsigned — but the UCP_TL_BITMAP_FOR_EACH_BIT macro is the idiomatic choice in this codebase.

Related: PR #12085 (#12085); prerequisite PR #12054 ("UCP/CORE: Support up to 255 TL resources") which added the clamped iterator.

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Tests roce on worker 0) · commit b6788941

TL;DR: The gtest process didn't fail a check — it hung for exactly 900 s (the gtest watchdog timeout) inside ucp_perf_test_flush_workers() while progressing a UD/verbs iface during endpoint wireup for self/test_ucp_loopback.envelope/14 <self/tag_mr_sync>, and the watchdog aborted it (SIGABRT/core). Root cause is an unbounded ucp_worker_flush_nbx() spin in src/tools/perf/lib/libperf.c:1550 where wireup/flush never completes on the UD lane; raising the job time limit will not help.

Full analysis

Summary: Azure job "Tests roce on worker 0" failed with make: *** [Makefile:4925: test] Aborted (core dumped) after self/test_ucp_loopback.envelope/14 <self/tag_mr_sync> hung and the gtest watchdog killed it.

Root cause: Timestamp analysis shows continuous per-test output every ~0.2 s until 15:47:48.4305 ([ RUN ] self/test_ucp_loopback.envelope/14 <self/tag_mr_sync>), then a single 900.001-second gap with zero output until 16:02:48.4315, which is test/gtest/common/test_helpers.cc:31 watchdog_timeout = 900. firing (ADD_FAILURE() << strerror(ETIMEDOUT) << " - abort testing" → "Connection timed out - abort testing", then pthread_kill → signal 6). That is a hang, not slowness. The captured backtrace pins the hung stack precisely:

ucx_perf_run -> ucp_perf_setup -> ucp_perf_test_setup_endpoints (libperf.c:1677)
             -> ucp_perf_test_flush_workers (libperf.c:1552)
             -> ucp_worker_progress -> uct_ud_verbs_iface_progress (ud_verbs.c:528)
             -> uct_ud_verbs_iface_poll_tx (ud_verbs.c:402) -> ibv_poll_cq

So the failure happened in endpoint setup, before any iteration: ucp_worker_flush_nbx() was posted to force wireup completion and its completion never arrived, so the loop

/* Progress all workers in parallel to avoid deadlocks */
while (ctx.num_outstanding > 0) { ucp_worker_progress(...); }

spins forever with no timeout and no diagnostics. The in-flight lane being polled is ud_verbs (the RoCE UD transport used for wireup on this worker), i.e. wireup/flush on the UD lane never completed.

Implicated commit: unknown with certainty. The branch's own commits are 60acc905 "UCP: Exclude device atomics from failover endpoints." and 7a7a32a0 "UCP: Check indirect atomics only for a selected IB domain." (both Evgeny Leksikov), which touch src/ucp/wireup/select.c and src/ucp/core/ucp_context.c. Note that the new ucp_wireup_exclude_device_amo_tls() path (select.c:1395) is gated on UCP_EP_INIT_ERR_MODE_FAILOVER, which this loopback test (err mode NONE) does not set — so the hang is not explained by that gate. However ucp_context_adjust_failover_mode() / ucp_context_query_ib_indirect_atomic() (ucp_context.c:2221-2277) added by 7a7a32a0 runs unconditionally for every context during ucp_fill_resources() and can flip failover_mode AUTO→SW globally, so it is the one new code path that affects this test and must be ruled out.

File: src/tools/perf/lib/libperf.c:1550-1554 (unbounded flush spin, the place that hung) — reached from src/tools/perf/lib/libperf.c:1677; hung lane progress at src/uct/ib/ud/verbs/ud_verbs.c:402; watchdog at test/gtest/common/test_helpers.cc:31,57.

Suggested fix:

1. Do not bump the 200 m job timeout or the 900 s watchdog — the process made zero progress for 15 minutes.
2. Reproduce locally with UCX_TLS unset (the loopback test deliberately does not pin TLS, unlike test_ucp_perf_cuda) and only gtest_filter=self/test_ucp_loopback.envelope/14, with UCX_LOG_LEVEL=debug to capture which lane's flush stays outstanding.
3. Rule the PR in or out quickly by running the same filter on master and on the branch with UCX_FAILOVER_MODE forced to the pre-patch value: if the hang disappears, the regression is 7a7a32a0's unconditional ucp_context_adjust_failover_mode() silently switching AUTO→SW for non-failover contexts; restrict that adjustment to contexts that actually request UCP_ERR_HANDLING_MODE_FAILOVER instead of applying it at context fill time.
4. Independently, harden the test harness so this class of failure is diagnosable: give ucp_perf_test_flush_workers() a deadline (e.g. ucs_get_time()-based bound) and return UCS_ERR_TIMED_OUT with the per-worker outstanding count and lane info, instead of spinning until the watchdog aborts the whole gtest binary and loses all remaining test results.
5. Also fix the resource leak spotted next door: ucp_context_query_ib_indirect_atomic() returns 1 at src/ucp/core/ucp_context.c:2239 without calling uct_config_release(md_config).

Related: PR #12085 (this PR), #11994 (UCX_FAILOVER_MODE parameter, introduces ucp_failover_mode_t), #11988 "UCP/WIREUP: fix worker flush hang from recovery wireup ep" — a prior, closely-related worker-flush hang; #12048 "UCP/FT: Purge an unaware lane before rebuilding its address".

[svc-nvidia-pr-review]

🤖 Starting review — findings will be posted here when done.

[gleon99]

👍🏼