Repository navigation
fix(rocjitsu): DBT host selection on multi-GPU systems - #9213
Conversation
Resolve automatic DBT host selection against the effective ROCm-visible topology and publish the selected KFD GPU ID through the atomic runtime config handoff.\n\nAdd selector, handoff, and selection edge-case coverage.\n\nRefs #9212
github-actions
Bot
added
documentation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
amd-arosa
changed the title
|
🎉 All checks passed! This PR is ready for review. |
|
fixing merge conflicts |
6a5bdb9 to
9378a5e
Compare
…-dbt-tests-multigpu # Conflicts: # emulation/rocjitsu/tests/launch_preload_test.cpp
atgutier
left a comment
atgutier
left a comment
There was a problem hiding this comment.
A couple agent items:
MAJOR
-
tools/rocjitsu/rocm_visibility.cpp:61-69,111-119 — malformed selectors are converted into valid device selections.
The parser is more permissive than the ROCr and CLR implementations it models:
- ROCR_VISIBLE_DEVICES=GPU- is invalid in ROCr. On a one-GPU topology, rocjitsu accepts it as a unique UUID prefix and rewrites it to 0,1, exposing both host and guest.
- CLR requires numeric HIP/CUDA tokens to use canonical decimal spelling. HIP_VISIBLE_DEVICES=01 should terminate selection, but rocjitsu accepts ordinal 1 and normalizes it to 1.
This is not merely diagnostic drift: invalid isolation settings that should expose no device can instead select and execute on a GPU.
Fix: enforce ROCr’s UUID-token length requirements and, after parsing a client ordinal, require token == std::to_string(index). Add coverage for GPU-, 01, 00, and valid-prefix-then-malformed sequences.
Confidence: high.
-
lib/rocjitsu/src/rocjitsu/kmd/linux/interposer.cpp:168-181 — the interposer can consume a different GPU ID than the HSA hook.
The launcher writes:
The HSA-hook loader reads the complete file, but the interposer performs one read() capped at 4095 bytes and treats any positive result as complete. A valid near-PATH_MAX config path can place only part
of the GPU ID inside that buffer. For example, the interposer can parse 2 while the HSA hook parses 28851; if the newline falls outside the buffer, the interposer instead performs its own automatic
selection.That directly violates the PR’s central invariant that GuestKfd and the HSA hook share one resolved host. KFD operations may then be rewritten to one GPU while queues and translated code objects use
another.Fix: read to EOF into a dynamic buffer, retry EINTR, fail on other read errors, and apply a deliberate maximum handoff size. Add a handoff exceeding 4095 bytes and verify both consumers obtain the
identical ID.Confidence: high.
MINOR
-
tests/config_test.cpp:637-660 — the new runtime-handoff test uses the fixed shared directory /tmp/rocjitsu_runtime_config_handoff_test.
Concurrent test processes can overwrite each other’s config_path or remove the directory while another process is reading it, producing flaky failures or cross-test configuration reads.
Fix: use the existing test::ScopedTempDirectory helper and RAII cleanup.
Confidence: high.
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
We should mirror the malformed-UUID validation on the client path. In emulation/rocjitsu/tools/rocjitsu/rocm_visibility.cpp:114-117, filter_client_visible_gpus still treats any token containing GPU- as a substring match; against the current source, bare GPU- selects gpu_id 100 and 1,GPU-,2 exposes all three GPUs, while the ROCR path rejects it. Please require a valid GPU-... prefix and length, use prefix matching, and cover both inputs in MalformedSelectorsTerminateSelection.
are you sure?
and the test [TEST(RocmVisibilityTest, MalformedSelectorsTerminateSelection) { const auto rocr_prefix = rocjitsu::cli::filter_rocr_visible_gpus(test_gpus(), "0,GPU-,1"); const auto client_prefix = rocjitsu::cli::filter_client_visible_gpus(test_gpus(), "1,00,2"); rocm-systems/emulation/rocjitsu/tests/launch_preload_test.cpp Lines 163 to 175 in 436245e |
Compile the hook sources once into rocjitsu_hooks_objects and link them into two shared libraries: the installed rocjitsu_hooks, which exports only OnLoad and OnUnload, and the test-only rocjitsu_hooks_testing, which adds rj_hsa_dbt_set_topology_nodes_root_for_test. Both shared libraries must name RJ_HOOKS_LINK_LIBS themselves: CMake propagates an object library's usage requirements transitively but not its object files, so attaching the ISA and VM dependencies only to rocjitsu_hooks_objects leaves them out of the linked libraries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kuhar
left a comment
kuhar
left a comment
There was a problem hiding this comment.
We should apply the UUID spelling checks to filter_client_visible_gpus as well. The size and starts_with guard quoted in the existing discussion is only in the ROCR branch; emulation/rocjitsu/tools/rocjitsu/rocm_visibility.cpp:114-117 still uses substring matching, so bare GPU- selects gpu_id 100 and 1,GPU-,2 produces 101,100,102 at 7010b4e. Please require a valid GPU- prefix and length there and add both cases to MalformedSelectorsTerminateSelection.
relocate_active_config_to_long_path() grew the parent directory greedily in 201-byte steps, then subtracted a separator from whatever remained. When the handoff directory's length was congruent to the 4081-byte target modulo 201 (61 bytes, for one), the loop landed exactly on the target and the remainder underflowed to SIZE_MAX. std::string then threw std::length_error, which the filesystem_error handler does not catch, so the helper terminated the test process instead of failing it. One byte short of the target left no room for a separator plus a component and fell through to a bare false. Plan the padding from the byte budget instead: split it across the fewest components that stay within the per-component limit and spread the characters evenly, so every intermediate length stays in range. Return an AssertionResult naming the size invariant that failed for the two budgets no component can spend, and for the I/O and filesystem failures that previously also returned a bare false. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GuestKfd rejects a host gpu_id it cannot match with ENODEV, but the HSA hook did not. When node_id_for_kfd_gpu_id() found no topology node for the configured host, AgentMapper recorded no mapping and rj_iterate_agents() fell through to the original iterator, publishing both physical agents. An application using the default device then ran untranslated on the real host, bypassing the guest entirely. Public enumeration is the replacement boundary, so fail closed there. Track whether the config demanded a mapping so the passthrough stays available when there is nothing to map, and return HSA_STATUS_ERROR when a required mapping was never resolved. MissingResolvedHostKeepsPhysicalAgentOrder pinned the old passthrough; it now expects the failure and is renamed accordingly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rocm_visibility.{h,cpp} arrived with all 24 of its tests wedged into
launch_preload_test.cpp, between that file's own cases. The file was
advertising eight preload tests and carrying thirty-two.
tests/ mirrors the source structure and this directory already follows
one-file-per-unit, so give the visibility helpers their own test file.
The only coupling was that rocm_visibility.cpp links into the existing
rocjitsu_cli_launch_preload target, which is a build detail rather than
a reason to share a test file.
Pure move: no test was added, removed, or changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
filter_client_visible_gpus() gated its UUID branch on a bare substring search, so any token merely containing "GPU-" was resolved against the first agent whose UUID also contained it. A bare "GPU-" selected gpu_id 100, and "1,GPU-,2" produced 101,100,102 -- an invalid isolation setting silently selecting devices instead of terminating selection. Apply the same spelling guard the ROCR path already used, factored into is_gpu_uuid_selector(): "GPU-" plus 1..16 hex digits of the 64-bit id, rejecting the "GPU-XX" sentinel ROCR reports for agents without UUID support. The comparison becomes a prefix match rather than a substring search. This makes rocjitsu deliberately stricter than the runtime it models -- CLR gates on the same bare find() with no length or prefix check (projects/clr/rocclr/device/rocm/rocdevice.cpp:431-444), so CLR resolves "GPU-" to agent 0 too. The divergence is safe only because CLR never sees a token rocjitsu rejected: this filter runs in DBT guest mode, where main.cpp unconditionally rewrites the client selector to normalized_client_visible_devices()'s canonical numeric list before execvp. Both the guard and the rewrite carry comments saying so. Everything below the guard stays CLR-shaped, preserving the intentional ROCR-vs-CLR split that DuplicateAndReorderedSelectorsMatchRuntimeBehavior documents: first match wins rather than terminating on ambiguity, and the comparison stays case-sensitive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…coped Refusing to enumerate agents when the DBT host never resolved was right in direction but wrong in three ways. It latched. ensure_discovered() published discovered_ = true even when discovery failed, and clear() only runs at OnUnload, so a single transient miss -- sysfs EMFILE/EACCES, a topology tree not yet populated, ROCR not yet enumerating -- poisoned agent enumeration for the life of the process. Classify the failure instead: TopologyLookup separates "read the whole tree, gpu_id is absent" from "could not read the tree", and DiscoveryOutcome turns that into a retryable or final verdict, retried a bounded number of times. Only snapshot() opts in; map(), is_guest(), guest_agent() and host_for_guest() stay cached-only and O(1), because they run once per copy and once per dispatch. The settled-state check now precedes the condition-variable wait so those callers cannot park behind another thread's retry. It was too broad. Returning HSA_STATUS_ERROR suppressed CPU agents too, breaking host fine-grained pools, hsa_amd_memory_lock and host-side copies that were never a translation hazard, and handing HIP an opaque init failure rather than an empty device list. Filter GPU agents on the refusal path and let CPU agents through; anything that cannot be positively classified counts as a GPU and is suppressed. It was too narrow. The fail-closed property rested entirely on no agent handle escaping enumeration. With no mapping, is_guest() returns false, so rj_executable_load_agent_code_object took its pass-through branch and would load untranslated guest code on the raw host. Guard the five GPU-only execution entry points with mapping_unusable(); deliberately leave the query and copy hooks unguarded, since they legitimately take CPU agents. Also refuse rather than guess when a config reaches the hook with no resolved host gpu_id. The host predicate was only constrained by driver node id when host_gpu_id != 0; at 0 it degraded to "any agent with the host ISA" -- the first-ISA-match selection this series exists to remove. That was unreachable only because load_dbt_guest_config_from_handoff() throws two files away, which is coincidental correctness in the fail-open direction. Resolve the host before the guest so a same-ISA config cannot let the guest predicate claim the physical host, and report that collision distinctly from "no agents found". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GuestKfd::ensure_ready_locked() still derived its own host GPU by scanning the topology for the first ISA match when host_gpu_id_ was 0. This series added the arm that verifies a supplied gpu_id but left the arm that invents one, so a working implementation of the per-layer divergence bug remained -- the KFD interposer and the HSA hook could each pick a different physical GPU on a box with identical devices. It was dead in practice, but only because load_dbt_guest_config_from_ handoff() rejects an enabled DBT config carrying gpu_id 0, an invariant enforced in a different file with nothing here stating it. Refuse instead, matching the hook-side refusal, and delete the two helpers that had no other caller. The interposer reported the same invariant through Logger::debug_print, which is `if constexpr` compiled out unless debug logging is configured on, so the KFD half of the fail-closed contract was silent in every shipped build while the HSA half printed. Use Logger::warn per the logging rule this series documents. Drop apply_resolved_dbt_host_gpu_id()'s `source` parameter, which had a single production caller and one fixed test string. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
relocate_active_config_to_long_path() returns testing::AssertionResult, so every precondition failure was a red test -- including ones that say nothing about the code under test. A CI runner whose $ROCJITSU_INVOCATION_DIR is already deeper than the >4095-byte handoff being constructed would fail ReadsRuntimeHandoffLargerThan4095Bytes while the reader it exercises is fine. GTEST_SKIP expands to a bare return and cannot be issued from a function returning AssertionResult, which is why this stayed as it was. Return a tri-state instead and let the test body issue the verdict: environmental preconditions skip with a reason, genuine defects -- write failures, a built path that misses its target length -- stay red. Nothing is silently swallowed. Register the new case with SKIP_REGULAR_EXPRESSION so a skip cannot be reported as a pass. Also hoist config_test.cpp's local ScopedEnvironmentVariable into scoped_temp.h beside ScopedTempDirectory, and use it from guest_kfd_test rather than hand-rolling save/restore a third time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
any_agent_could_be_a0() folded "an A0 part was found" and "agent enumeration failed" into one bool. Both correctly refuse the load, so the behaviour was already fail-safe -- but when a co-loaded HSA tool fails enumeration, the refusal is reported as an A0 incompatibility and the real cause is invisible. Split the verdict into kNone / kPossible / kUnenumerable so the last can log what actually happened. Both refusal branches still return HSA_STATUS_ERROR_INCOMPATIBLE_ARGUMENTS, so no caller observes a change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CONTRIBUTING.md and docs/style.md both stated a blanket ban on fprintf, printf and std::cerr that the tree has never followed: the hook layer has ~30 tagged stderr diagnostics on hard-error paths, tools/ writes user-facing errors to std::cerr, and the signal backtrace handler uses raw ::write because it must stay async-signal-safe. The ban was also unactionable as written. Logger::print<>() and the group helpers compile to nothing unless RJ_LOG_GROUPS names the group at configure time, which defaults to OFF, so routing a fatal diagnostic through them silences it in every shipped build. Logger::warn() is the always-on channel and the docs never said so. State the actual rule -- group helpers for tracing, Logger::warn() for anything audible by default -- and name the two carve-outs with their reasons rather than pretending they do not exist. Also record that the DBT hook and the gfx1250 hotswap hook are mutually exclusive in HSA_TOOLS_LIB: both patch the same four code-object entries, nothing enforces load order, and neither exports HSA_AMD_TOOL_PRIORITY. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The runtime config handoff has two consumers with two independently written readers: the KFD interposer's raw read loop, and the HSA hook's load_dbt_guest_config_from_runtime_config(). Only the interposer ever had the fixed 4096-byte read that truncated a handoff whose config path approaches PATH_MAX, and only the interposer got a regression test for it -- GuestKfdConfigTest.ReadsRuntimeHandoffLargerThan4095Bytes. That leaves the actual invariant untested. What matters is not that one reader survives a long handoff but that both resolve the same host GPU from it: the two layers picking different physical GPUs on a multi-GPU host is the failure this series has been closing off, and nothing compared the two readers' answers. The hook side reads through an unbounded std::ifstream, so this is coverage rather than a fix -- but coverage that fails loudly if either reader is ever rewritten. Add ConfigLoaderTest.ReadsRuntimeHandoffLargerThan4095Bytes, named to match its KFD counterpart, driving the same >4095-byte handoff through the hook consumer end to end and asserting the resolved gpu_id. Hoist the long-path construction into tests/long_path_handoff.h rather than writing it a second time. The padding arithmetic is delicate -- each component costs a separator plus a character, so a greedy loop can strand a remainder it cannot spend -- and a divergent copy could quietly stop being oversized on one side while both tests still passed. Sharing it also carries the tri-state verdict across, so the new test skips with a reason on a workspace already too deep for the path instead of going red for a reason that says nothing about the reader. Both halves now assert test::kOversizedHandoffHostGpuId, and the helper self-checks that the payload it built really does exceed 4095 bytes. On the KFD side the id is proven by bring-up: ensure_ready_locked() refuses a gpu_id that matches no host-ISA GPU in the topology, so a visible overlay means the interposer read that exact value. A static assert pins the constant to the launcher config supplying that topology, so changing one without the other fails at compile time rather than turning the overlay check into a tautology. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2be8f4c to
150db09
Compare
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
PR #9213 added physical topology validation while GuestKfd::mutex_ is held. Enumerating the real topology through std::filesystem re-enters the sysfs interposer, which attempts to acquire the same mutex and blocks subsequent /dev/kfd opens. Enumerate and inspect topology nodes through libc_passthrough so host validation reaches the real sysfs directly. Add a focused lifecycle test covering close followed by reopen. Tests: - GuestKfd timeout cluster: 9/9 passed - GuestKfd tests: 12/12 passed - Full CTest: 2720 passed, 3 skipped, 2 unrelated failures
PR #9213 added physical topology validation while GuestKfd::mutex_ is held. Enumerating the real topology through std::filesystem re-enters the sysfs interposer, which attempts to acquire the same mutex and blocks subsequent /dev/kfd opens. Enumerate and inspect topology nodes through libc_passthrough so host validation reaches the real sysfs directly. Add a focused lifecycle test covering close followed by reopen. Tests: - GuestKfd timeout cluster: 9/9 passed - GuestKfd tests: 12/12 passed - Full CTest: 2720 passed, 3 skipped, 2 unrelated failures
Motivation
Rocjitsu DBT tests fail before entering their test bodies when multiple GPUs share the configured host ISA. Automatic
host_gpu_id: 0selection was rejected as ambiguous, and simulator-backed DBT was incorrectly checked against physical KFD topology.This change makes one synthetic DBT guest select one host consistently while respecting standard ROCm device visibility.
Technical Details
ROCR_VISIBLE_DEVICES, thenHIP_VISIBLE_DEVICESor itsCUDA_VISIBLE_DEVICESfallback.Issue Tracking
Fixes #9212
Test Plan
rocjitsu_bin,rocjitsu_shared,rocjitsu_hooks,rocjitsu_tests, andlaunch_preload_test.ConfigLoaderTest,LaunchPreloadTest, andRocmVisibilityTesttests.ROCR_VISIBLE_DEVICESandHIP_VISIBLE_DEVICESordering on an eight-GPU gfx942 system.Test Result
Submission Checklist