Skip to content

GTEST/UCP/RMA: Add dram/vram direction variants for SGL tests - #11941

Merged
michal-shalev merged 6 commits into
openucx:masterfrom
michal-shalev:sgl-memtype-variants
Nov 3, 2026
Merged

michal-shalev merged 6 commits into
openucx:masterfrom
michal-shalev:sgl-memtype-variants

Conversation

@michal-shalev

Copy link
Copy Markdown
Contributor

What?

Run every UCP RMA SGL test in four memory type variants: dram_to_dram, dram_to_vram, vram_to_dram, vram_to_vram.

Why?

SGL put was only covered with the same memory type on both sides, leaving cross memory type transfers such as VRAM to DRAM untested.

How?

The local/remote memory type pair is encoded in the test variant value, and the existing CUDA skip is left as is, now applying to vram_to_vram.

@michal-shalev michal-shalev self-assigned this Sep 13, 2026
@michal-shalev
michal-shalev marked this pull request as draft September 13, 2026 19:27
@michal-shalev michal-shalev added the WIP-DNM Work in progress / Do not review label Sep 13, 2026
@michal-shalev michal-shalev added Ready for Review and removed WIP-DNM Work in progress / Do not review labels Oct 28, 2026
@michal-shalev
michal-shalev marked this pull request as ready for review October 28, 2026 17:08
@openucx openucx deleted a comment from svc-nvidia-pr-review Oct 28, 2026
@openucx openucx deleted a comment from svc-nvidia-pr-review Oct 28, 2026
@openucx openucx deleted a comment from svc-nvidia-pr-review Oct 28, 2026
@openucx openucx deleted a comment from svc-nvidia-pr-review Oct 28, 2026
@openucx openucx deleted a comment from svc-nvidia-pr-review Oct 28, 2026
@openucx openucx deleted a comment from svc-nvidia-pr-review Oct 28, 2026
@openucx openucx deleted a comment from svc-nvidia-pr-review Oct 28, 2026
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread test/gtest/ucp/test_ucp_rma.cc Resolved
Comment thread test/gtest/ucp/test_ucp_rma.cc Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

Residual coverage note: the new host↔cuda combinations only run where CUDA is present and the instantiation includes a GPU-capable TL (UCP_INSTANTIATE_TEST_CASE_TLS_GPU_AWARE(test_ucp_rma_sgl, tcp, ...)), so on CUDA-less CI nodes the variant set is unchanged (host→host only) and the new coverage is exercised only by GPU jobs. Worth confirming a GPU CI job ran green on this PR before merge.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread test/gtest/ucp/test_ucp_rma.cc Resolved
Comment thread test/gtest/ucp/test_ucp_rma.cc Resolved
@svc-ucx

svc-ucx commented Oct 28, 2026

Copy link
Copy Markdown

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

TL;DR: The "roce on worker 0" job failed because ./test/apps/test_hooks -n 79 segfaulted on iteration 24/30 of the UCM Bistro hook test — a known race in UCM's live-code (bistro) patching of mmap while dozens of threads are executing it, not anything introduced by PR #11941 (which only adds gtest SGL variants). Re-run the job; to fix properly, harden/lengthen the bistro patch grace window and make test_hooks flush its log so the crash point is identifiable.

Full analysis

Summary: contrib/test_jenkins.sh line 1101 (test_ucm_hooks) aborted with Segmentation fault / "timeout: the monitored command dumped core" when running test_hooks -n 79; the gtest release stage before it passed.

Root cause: test_hooks starts N (= random up to 2×NPROC, here 79) threads that call mmap() in a tight loop, then dlopen()s libucm to trigger bistro hook installation. UCM patches the live mmap code in place: ucm_bistro_apply_patch_atomic() sets the page W+X, writes a "lock" instruction, waits ucm_global_opts.bistro_grace_duration, then memcpy()s the rest of the jump payload over the function prologue (src/ucm/bistro/bistro.c:91-97). If a thread is still inside the patched prologue when the payload is copied, it executes a half-written instruction stream → SIGSEGV. Timing in the log confirms this: healthy iterations last ~1.3 s, but the failing one died 0.44 s after start — i.e. right at the dlopen()/patch moment (threads are started, then usleep(200ms), then patching). test_hooks.log contained only timeout: the monitored command dumped core because the app's buffered printf output was lost in the crash, so no phase/stack info was recorded. This is the same race that commit c77bb48 ("UCM/BISTRO: Increase grace time to reduce patch race") only mitigated; it is independent of the PR under test (#11941 touches only gtest RMA/SGL test code).

Implicated commit: Not the PR commit ([REDACTED:Hex High Entropy String]). Pre-existing mechanism from 676b579 "UCM/BISTRO: Add locking step while patching library functions" (Thomas Vegas), partially mitigated by c77bb48 "UCM/BISTRO: Increase grace time to reduce patch race" (shasson5).

File: src/ucm/bistro/bistro.c:70-103 (ucm_bistro_apply_patch_atomic, grace loop at :86-89, payload copy at :91-93); test harness: contrib/test_jenkins.sh:1098-1115; test/apps/test_hooks.c:83-89

Suggested fix:

  1. Immediate: re-run the roce on worker 0 job — this failure is unrelated to PR GTEST/UCP/RMA: Add dram/vram direction variants for SGL tests #11941 and is a known flake.
  2. Make the flake diagnosable: add setvbuf(stdout, NULL, _IOLBF, 0) at the top of main() in test/apps/test_hooks.c (or run it under stdbuf -o0), and in test_ucm_hooks() add ulimit -c unlimited plus a gdb -batch -ex bt on the core so the next occurrence shows which instruction/thread faulted.
  3. Reduce the race: run the loop with a larger grace window (e.g. UCX_BISTRO_GRACE_DURATION=0.1 exported in test_ucm_hooks), and/or scale the grace duration with the number of runnable threads; longer term, patch via a mirrored page (mremap/memfd swap of the whole code page) instead of writing the payload in place, so no thread can ever observe a partially written prologue.
  4. Unrelated but visible in the same log: ./test/apps/test_ucp_config: error: '.../test/apps/.libs/test_ucp_config' does not exist (line ~+ ./test/apps/test_ucp_config -c 'UCX_MODULES=^ib,rdmacm') — the binary isn't built in this configuration, so the module allow-list check silently passes. Worth making that step fail loudly or building the app.

Related: PR #11941 (branch under test, gtest-only: "GTEST/UCP/RMA: Add dram/vram direction variants for SGL tests"); prior mitigation PR #11112 (UCM/BISTRO: Increase grace time to reduce patch race)

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id d401ca98-da1b-4141-94a4-ccc4c95f7b9c in the triage console for the audit trail.

@michal-shalev
michal-shalev requested a review from a team as a code owner November 1, 2026 22:25
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread test/gtest/ucp/test_ucp_rma.cc Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

Residual coverage note: the new mixed memtype combinations only materialize on CUDA-capable nodes, so the host→cuda / cuda→host SGL put/get paths are exercised only by GPU CI jobs.

@svc-ucx

svc-ucx commented Nov 1, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (AddressSanitizer roce on worker 2) · commit d4a1a516

TL;DR: The ASAN/RoCE gtest aborted in ud_mlx5/uct_p2p_am_misc.no_rx_buffs/2 with ucs_fatal from ud_ep.c:293 ("unhandled timeout error") — the test deliberately starves the UD receiver's RX mpool, and under AddressSanitizer the recovery is so slow that the sender's UD peer timeout expires; since the gtest UCT entity installs no EP error handler, the UD code escalates to fatal/abort. This is a pre-existing, ASAN-timing flake unrelated to PR #11941 (a UCP RMA gtest-variant change); fix by skipping/hardening no_rx_buffs for UD under sanitizer builds (or installing an error handler / raising UCX_UD_TIMEOUT for that test).

Full analysis

Summary: gtest crashed (make: *** [Makefile:4971: test] Aborted (core dumped)) during ud_mlx5/uct_p2p_am_misc.no_rx_buffs/2 <ud_mlx5/mlx5_1:1> with [swx-rain03:26954] ud_ep.c:293 Fatal: UD endpoint 0x6140018af240 to :0: unhandled timeout error.

Root cause: no_rx_buffs (test/gtest/uct/test_p2p_am.cc:627) configures a tiny RX pool (IB_RX_MAX_BUFS=1024, IB_RX_QUEUE_LEN=256) and then returns UCS_INPROGRESS from the AM handler (set_keep_data(true)) so the receiver holds every descriptor until its RX mpool is empty. While starved, the receiver cannot ACK, so the sender keeps retransmitting. In this ASAN build the starve/recover phase took the whole 30.3 s window between [ RUN ] ...no_rx_buffs/2 (23:08:14.17) and the fatal (23:08:44.48) — long enough for the UD peer timeout to expire. uct_ud_ep_deferred_timeout_handler() then calls uct_iface_handle_ep_err(), and because the UCT p2p gtest entity opens the iface without an error handler, that call returns non-UCS_OK and the code calls ucs_fatal() (src/uct/ib/ud/base/ud_ep.c:293), aborting the whole gtest binary and failing the job. The test is explicitly guarded against this kind of slowdown only for Valgrind (RUNNING_ON_VALGRIND in the skip condition), not for ASAN. Nothing in PR #11941 ("GTEST/UCP/RMA: Add dram/vram direction variants for SGL tests") touches UD, UCT AM, or this test, and test_p2p_am.cc has not changed since March 2025 — so the failure is environmental/flaky, not caused by the commit under test.

Implicated commit: unknown (not PR-related); most recent related change is d09569bf "TEST/UCT: Increase RX queue length of no-buffers test to 256" (Yossi Itigin), with the fatal path from 183a66db/8a38ae21 era UD timeout handling.

File: test/gtest/uct/test_p2p_am.cc:627-666 (test), src/uct/ib/ud/base/ud_ep.c:289-296 (fatal)

Suggested fix:

  1. Short term: re-run the "roce on worker 2" ASAN job — this is a known-style timing flake in no_rx_buffs and not caused by d4a1a51.
  2. Harden the test: extend the skip condition to sanitizer builds (e.g. add RUNNING_ON_ASAN/ucs::test_time_multiplier() > 1 alongside RUNNING_ON_VALGRIND at test/gtest/uct/test_p2p_am.cc:628), and/or bound the starvation loop so it releases descriptors before the UD peer timeout can expire (scale the 1 s/3 s send_with_timeout budgets by ucs::test_time_multiplier()).
  3. Alternatively, bump UD_TIMEOUT/UD_TIMER_TICK via uct_config_modify() in the uct_p2p_am_misc constructor for UD transports, or install an EP error handler in that fixture so a UD peer timeout surfaces as a test error instead of ucs_fatal() killing the entire gtest run.

Related: PR #11941 (the PR under test, unrelated to the failing test); prior UD-timeout test-hardening work in #11920 and #11948.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread test/gtest/ucp/test_ucp_rma.cc Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread test/gtest/ucp/test_ucp_rma.cc Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

@michal-shalev
michal-shalev enabled auto-merge (squash) November 3, 2026 08:35
@michal-shalev
michal-shalev merged commit 8827b4f into openucx:master Nov 3, 2026
167 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants