Repository navigation
fix(runtimes) Fix several issues based on rigorous review - #14517
Merged
Merged
Conversation
Concurrent kernel submits could publish an older accepted ID after a newer one, and native destroy could then reject live work with BUSY. Both AMDF queue destructors consumed handles on that rejection, allowing a retry to unregister a device child twice. Publish the maximum submission ID and preserve each queue and its device borrow when native preflight reports work-related BUSY without errno. The libamdf unit suite and cargo fmt check pass.
A failed final runtime disable left the departing session counted, so a second frontend could join and make a retry skip the required disable. A queue whose native destroy failed could also drop its driver reference while KFD still held the queue. Record pending disable on the process state and finish it before another session joins. Retain the queue driver until native teardown succeeds. The rocddi unit suite and cargo fmt check pass.
The first signal event fixes the process page's GPU mapping set. AMDF omitted peers, so a later HSA event on another GPU could fail. Requiring every discovered peer also made an unavailable GPU fail the first event. Let rocddi discover peers before the first page offer and include each peer it can activate and prepare. Keep the requested GPU required; later use of an omitted peer reports the mapping mismatch. Verified with the three DDI library suites, strict Clippy, and cargo fmt. Multi-GPU KFD hardware was not available for this check.
AMDF hard-coded wave32 in AQL scratch requests, while qualified GFX9 queues require wave64. Read the activated GPU wavefront size when building the native scratch descriptor. cargo check for libamdf and cargo fmt check pass.
AMDF passed a page cover to rocddi, while KFD added the sub-page offset again. An unaligned registration could pin an extra page or fail with EFAULT. DRM and KFD also interpreted the same size differently. Pass the exact byte length from AMDF and use one rocddi page-cover rule in both backends. The rocddi and libamdf unit suites pass.
A failed fixed file mapping can leave a hole in an owned range, while an early failure can leave the original reservation intact. Recovering with NOREPLACE treated that intact range as EEXIST and poisoned it. Moving a KFD doorbell or MMIO VMA can also lose driver mapping state. Stage render, DMA-BUF, and host mappings before relocating them over an owned range. Keep the reservation live if staging fails and poison it if relocation may have opened a hole. Map fresh exact-size KFD pages in place so their native VMA is created at its final address. Verified with early-failure, child-view, and in-place mapping tests, 293 rocddi library tests, strict Clippy, and cargo fmt. KFD hardware was not available for this check.
A single wait could read a slot after concurrent destruction freed or reused it. Putting every wait under the lifecycle mutex also serialized already-satisfied waits with long host copies. Track live public slots behind a short registry read lock so destruction excludes the fast load. Only an unsatisfied blocking wait takes the lifecycle mutex to acquire a per-slot async reference. Check the slot generation again before retaining it so reuse cannot redirect the wait. Zero-timeout waits return without the lifecycle mutex. Verified with the completed-wait lock regression, the libhsa library suite, strict Clippy, and cargo fmt.
A public queue mapping on a peer GPU did not count against that producer device. The device could be destroyed while the mapping still exposed its address space and doorbell. Register the peer device before mapping, roll the count back on failure, and release it when the public mapping is unmapped. The libamdf unit suite passes.
The standard reader-writer lock can block a recursive read after a writer starts waiting. A traversal callback that iterates again could therefore deadlock with concurrent executable create or destroy. Use a traversal gate that admits reads already held by the current thread while preserving writer exclusion and returning an error for mutation from a traversal callback. The libhsa unit suite and cargo check pass.
github-actions
Bot
added
the
documentation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical issues can deadlock shutdown, lose queue memory-fault notifications, and permanently block instance destruction after native GPU event use.
3 open issues
What changed in this PR
This PR hardens resource ownership, teardown, and event handling across rocddi and its HSA and AMDF frontends, while moving shared GPU and memory policies into rocddi.
Changes:
- Centralizes GPU target profiles, queue transport validation, host registration, and scratch handling.
- Strengthens lifecycle synchronization, signal safety, event failure reporting, and retryable teardown.
- Updates frontend integrations and adds focused regression tests.
| File | Description |
|---|---|
| runtimes/ddi/rocddi/tests/cts.rs | Adopts virtual-memory options in conformance tests. |
| runtimes/ddi/rocddi/src/topology/model.rs | Generalizes driver nodes and host routes. |
| runtimes/ddi/rocddi/src/topology.rs | Defines shared GPU target profiles. |
| runtimes/ddi/rocddi/src/session.rs | Adds teardown preflight and resource tests. |
| runtimes/ddi/rocddi/src/os/linux/process_identity.rs | Adds per-slot fork witnesses. |
| runtimes/ddi/rocddi/src/os/linux/memory/host_reservation.rs | Uses shared mapping-replacement logic. |
| runtimes/ddi/rocddi/src/os/linux/file.rs | Duplicates raw descriptors through fcntl. |
| runtimes/ddi/rocddi/src/os/linux.rs | Maps missing native objects to NotFound. |
| runtimes/ddi/rocddi/src/memory/driver_virtual_tests.rs | Updates virtual-memory test interfaces. |
| runtimes/ddi/rocddi/src/memory/device/virtual_memory.rs | Accepts structured virtual-memory options. |
| runtimes/ddi/rocddi/src/memory/device/types.rs | Adds page-cover calculation and stable placement identities. |
| runtimes/ddi/rocddi/src/memory/device/allocation.rs | Exposes host-range registration and alias metadata. |
| runtimes/ddi/rocddi/src/memory/device.rs | Exports shared memory options and helpers. |
| runtimes/ddi/rocddi/src/memory.rs | Updates internal memory exports. |
| runtimes/ddi/rocddi/src/error.rs | Adds the portable NotFound category. |
| runtimes/ddi/rocddi/src/driver/resources.rs | Routes alias operations and validates queue publication. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/vmem.rs | Checks reservation ownership and translates placement identities. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/tests/vmem.rs | Tests imported placement and updated options. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/tests/sys.rs | Tests native exception waits. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/tests/shutdown.rs | Updates shutdown fixtures for new state. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/tests/queue.rs | Tests transport validation and target profiles. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/tests/memory.rs | Tests host aliases and native event waiting. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/sysfs.rs | Resolves target profiles during discovery. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/sys.rs | Adds exception waits and specialized KFD mapping calls. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/registered_host.rs | Registers and frees complete host page covers. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/queue.rs | Uses shared target and scratch qualification. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/operations.rs | Integrates aliases, registration extents, and profiling reports. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/memory/scratch.rs | Implements scratch-aperture range management. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/kernel_queue.rs | Updates the kernel-queue target fixture. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/interop.rs | Resolves imported backing to endpoint identities. |
| runtimes/ddi/rocddi/src/driver/linux_kfd/allocation.rs | Routes host-alias allocation operations. |
| runtimes/ddi/rocddi/src/driver/instance.rs | Adds event waiting and test GPU routing. |
| runtimes/ddi/rocddi/src/driver.rs | Updates memory and profiling driver contracts. |
| runtimes/ddi/rocddi/src/device/gpu/queue/types.rs | Defines queue transport publication checks. |
| runtimes/ddi/rocddi/src/device/gpu/queue/scratch.rs | Uses target profiles for scratch planning. |
| runtimes/ddi/rocddi/src/device/gpu/queue/exception.rs | Shares exception decoding and scratch-owner replacement. |
| runtimes/ddi/rocddi/src/device/gpu/queue.rs | Validates transports and retains uncertain queue owners. |
| runtimes/ddi/rocddi/src/device/gpu/profiling.rs | Returns profiling progress alongside failures. |
| runtimes/ddi/rocddi/src/device/gpu/image.rs | Uses centralized image support qualification. |
| runtimes/ddi/rocddi/src/device/gpu/event/linux.rs | Discovers signal-page peer mappings automatically. |
| runtimes/ddi/rocddi/src/device/event.rs | Adds timed event subscription waits. |
| runtimes/ddi/rocddi/docs/safety.md | Documents host-alias ownership guarantees. |
| runtimes/ddi/libhsa/src/signal/wait.rs | Retains signal storage across blocking waits. |
| runtimes/ddi/libhsa/src/signal/tests.rs | Tests the lifecycle-lock-free satisfied wait path. |
| runtimes/ddi/libhsa/src/signal/ipc.rs | Registers IPC signal identities and wait lifetimes. |
| runtimes/ddi/libhsa/src/signal.rs | Tracks signal generations and trusted IPC metadata. |
| runtimes/ddi/libhsa/src/runtime/tests.rs | Tests concurrent initialization and lifecycle reentry. |
| runtimes/ddi/libhsa/src/runtime/lifecycle.rs | Coordinates initialization waits and transition notifications. |
| runtimes/ddi/libhsa/src/runtime/events.rs | Bounds retries and records event-observer failure. |
| runtimes/ddi/libhsa/src/runtime/agent.rs | Uses generalized topology nodes for CPU agents. |
| runtimes/ddi/libhsa/src/runtime.rs | Integrates lifecycle, event, and topology changes. |
| runtimes/ddi/libhsa/src/queue/tests.rs | Tests scratch retention and counted-queue rejection. |
| runtimes/ddi/libhsa/src/queue/index.rs | Clarifies counted-queue control access. |
| runtimes/ddi/libhsa/src/queue/event.rs | Uses shared exception handling and observer-failure fallback. |
| runtimes/ddi/libhsa/src/queue/create.rs | Relies on rocddi transport validation. |
| runtimes/ddi/libhsa/src/queue.rs | Updates shared queue helper imports. |
| runtimes/ddi/libhsa/src/platform/linux.rs | Provides native CPU-node ordinal translation. |
| runtimes/ddi/libhsa/src/platform.rs | Exports CPU-node translation. |
| runtimes/ddi/libhsa/src/memory/tests.rs | Removes tests for the relocated page-cover helper. |
| runtimes/ddi/libhsa/src/memory/pool.rs | Uses native endpoint topology attachments. |
| runtimes/ddi/libhsa/src/memory/interop.rs | Delegates host-range alignment to rocddi. |
| runtimes/ddi/libhsa/src/loader/traversal.rs | Adds reentrant executable traversal admission. |
| runtimes/ddi/libhsa/src/loader.rs | Installs the executable traversal gate. |
| runtimes/ddi/libhsa/src/image_abi.rs | Uses target profiles for sampler support. |
| runtimes/ddi/libhsa/src/api/signal/atomic.rs | Prevents IPC metadata from authorizing doorbell stores. |
| runtimes/ddi/libhsa/src/api/queue/core.rs | Rejects counted-queue inactivation before shared mutation. |
| runtimes/ddi/libhsa/src/api/memory/virtual_memory.rs | Adopts options and endpoint-based placement lookup. |
| runtimes/ddi/libhsa/src/api/loader/query.rs | Adapts queries to traversal-gate errors. |
| runtimes/ddi/libhsa/src/api/loader/executable/state.rs | Adapts mutations to traversal-gate errors. |
| runtimes/ddi/libhsa/src/api/lifecycle.rs | Guards lifecycle reentry and waits for concurrent initialization. |
| runtimes/ddi/libhsa/src/api/amd.rs | Preserves profiling outputs on native failure. |
| runtimes/ddi/libamdf/src/tests.rs | Tests target-based SDMA feature reporting. |
| runtimes/ddi/libamdf/src/support.rs | Maps NotFound and clarifies destruction borrowing. |
| runtimes/ddi/libamdf/src/reservation.rs | Separates mutable reservation state from immutable queries. |
| runtimes/ddi/libamdf/src/queue.rs | Retains peer producers and preserves retryable teardown. |
| runtimes/ddi/libamdf/src/queue_tests.rs | Tests retryable destruction and peer borrow release. |
| runtimes/ddi/libamdf/src/kernel_queue.rs | Publishes monotonic submission IDs and preserves busy handles. |
| runtimes/ddi/libamdf/src/instance.rs | Adds teardown preflight and observer-failure tracking. |
| runtimes/ddi/libamdf/src/gpu_event.rs | Reports observer failure separately from device loss. |
Review effort: Balanced
Comment on lines
+743
to
+746
| instance | ||
| .native | ||
| .preflight_destroy() | ||
| .map_err(|error| native(&error))?; |
Comment on lines
+303
to
+305
| failed.store(true, Ordering::Release); | ||
| VM_FAULT_CONDVAR.notify_all(); | ||
| return; |
| while guard.init_transition_in_progress() { | ||
| // A deferred final shutdown must join callbacks and workers before | ||
| // it can leave Preparing. Waiting here from either would deadlock. | ||
| if CallbackScope::active() || guard.as_ref().is_some_and(HsaState::running_on_worker) { |
Concurrent hsa_init calls need to wait for a generation transition and then add a reference. A callback or worker cannot wait during prepared shutdown because the shutdown helper may need to join that caller. A borrowed logging stream callback also holds the mutex shutdown needs. Wait on the lifecycle condition variable for ordinary callers. Reject callback, worker, and log-write reentry with INVALID_RUNTIME_STATE. Have the transition method notify its injected condition variable in the test. Verified with concurrent-init, callback, and log-write reentry tests, 133 libhsa library tests, strict Clippy, and cargo fmt.
AMDF inspected Linux errno 2 to translate a vanished endpoint into its portable NOT_FOUND status. That leaked a native error code across the backend boundary. Add a rocddi NotFound kind for missing native objects and translate it in AMDF. The rocddi and libamdf unit suites pass.
The safe rocddi descriptor helpers constructed BorrowedFd from caller integers, which requires a valid and live descriptor under Rust fd safety. Call F_DUPFD_CLOEXEC directly so the kernel validates the integer and construct File only from a successful duplicate. The rocddi unit suite passes.
Frontend and native target checks duplicated GFX family rules and could drift on scratch, SDMA packet formats, and image support. Resolve a profile when KFD discovers a GPU and use its exhaustive capability methods in queue, copy, image, and format negotiation paths. Verified with cargo test for rocddi, libamdf, and libhsa library suites. Cargo fmt and git diff --check pass.
Counted handles share a hardware ring and doorbell. Marking one handle inactive leaves a producer able to write through its cached ring address and overwrite packets belonging to another live handle. Return INVALID_QUEUE for counted handles before touching shared native state. Put the unintercepted queue decision in one function called by the public API and its unit fixture. Verify rejection leaves the shared write index unchanged and the counted handle usable. Verified with 130 libhsa library tests, strict Clippy, and cargo fmt.
AMDF and HSA polled the same native stream at different intervals. An unclassified driver error could stop their event workers. A queue VM fault deferred to HSA's system event worker could then lose both notification paths when that worker exited. Use rocddi's bounded subscription wait. Retry Busy with bounded backoff and up to 16 consecutive Driver failures. Latch terminal observation failure separately from reset and shutdown. Track each HSA queue fault through deferral and system delivery; after observer failure, its own queue worker reports any still-pending fault once. Preserve completed AMDF GPU event waits before consulting the polling-failure latch. Only queue errors already observed can be marked as covered by a system callback; KFD provides no shared ID for later queue errors. Verified with injected worker failures and fault-delivery state tests, 47 AMDF and 133 HSA library tests, strict Clippy, and cargo fmt.
The traversal and lifecycle fixes introduced private guards without required documentation, and a KFD test used a forbidden panic path. Document the ownership state, match unit errors explicitly, and report an unexpected test ioctl as an I/O error. Verified with cargo clippy for all targets in rocddi, libamdf, and libhsa with warnings denied.
A live session owner could make instance_destroy return Busy after AMDF had set closing and released its signal page. The page itself can retain one driver reference after all public events are gone, causing the ordinary preflight to return Busy forever. Before teardown, require a unique session and reject external driver owners while accounting for the exact allocation held by the signal event service. Release that page only after this scoped preflight, then use the ordinary session destroy check. Busy leaves the instance usable. Verified with exact-owner and foreign-page preflight tests, 294 rocddi and 47 AMDF library tests, strict Clippy, and cargo fmt.
A process-wide slot compared only PIDs. A descendant can eventually reuse an ancestor PID and match inherited KFD state. Give each published slot a separate wipe-on-fork witness and require it to remain set before touching the slot mutex. Verified with rocddi unit tests and strict Clippy across all three DDI crates.
The shared process mutex covered runtime disable and activation, so debugger-held ioctls could block unrelated sessions. Mark a pending disable as in progress, issue it outside the mutex, and let joins wait for its result. Retain context resources through independent shared ownership during activation. Verified with rocddi library tests, a test assertion that the slot lock is free inside the disable ioctl, and strict Clippy for all DDI crates.
Reservation updates created an exclusive Rust reference to the whole handle even though immutable address queries may run concurrently. Keep published facts immutable and put caller-serialized native mapping state in UnsafeCell. Reserve whole-handle exclusive references for terminal destruction. Verified with the libamdf library suite and strict Clippy for all three DDI crates.
HSA owned native CP bit decoding and the candidate scratch lifetime sequence, leaving other frontends without those mechanisms. Expose portable AQL exception facts and stage replacement owners in rocddi; HSA keeps its status mapping and callback policy. Verified with the three DDI library suites and strict Clippy for all targets.
AMDF, HSA, and the SDMA copy service repeated different ring and index geometry checks. Validate each backend transport against a compact request expectation in rocddi, including peer producer mappings. Retire malformed unpublished queues and retain dependencies when cleanup is uncertain. Verified with the three DDI library suites and strict Clippy for all targets.
KFD can reject a second USERPTR registration of an address already mapped from a BO, while the frontends rounded host ranges independently. HSA could fail to lock an AMDF host view, and a native alias needed a retryable source lifetime. Centralize page-cover registration in rocddi and retain mapped BO records per KFD connection. Matching registrations borrow the existing GPU mapping; source release and access changes stay BUSY until aliases end. Preserve AMDF's public source metadata and publish its backing identity for aliases created across instances. Verified with rocddi, libamdf, and libhsa library tests, strict Clippy, rustfmt, and diff whitespace checks. GPU execution was not available.
The core and driver passed pinned and uncached as adjacent booleans, making call sites easy to transpose and hiding the policy at the HSA boundary. Pass a VirtualMemoryOptions value through the device, driver, and KFD owner. The HSA adapter now names both requested properties when it creates detached backing. Verified with rocddi and libhsa library tests, strict Clippy across all three crates, rustfmt, and diff whitespace checks.
The rocddi SPM destination call passed several mutable scalar outputs through each layer, mirroring the ioctl and obscuring its partial-progress behavior on failure. Return a report on success and pair the same report with a native error on failure. The HSA adapter publishes the report in either case, preserving its C output contract while the internal interface carries one coherent result. Verified with rocddi and libhsa library tests, strict Clippy across all three crates, rustfmt, and diff whitespace checks.
IPC signal storage is writable by peers, but host stores used its kind word to select MMIO doorbell and intercept operations. A peer could turn an ordinary IPC store into an unsafe host operation. Track locally created and imported IPC handles through their native mapping lifetime and route their stores through ordinary signal atomics. Queue-owned doorbell signals retain their existing behavior. Verified with 127 HSA library tests (4 GPU tests ignored), strict Clippy, rustfmt, and git diff --check.
The shared device-event wait API slept between zero-time polls, adding latency and repeated ioctls for both frontends. KFD already exposes the persistent exception events through WAIT_EVENTS. Block on both native exception events for bounded intervals, then inspect the retained subscription records. A full retained buffer still waits for another subscriber to drain it, and native errors remain visible. Verified with focused wait tests, 288 rocddi tests (2 ignored), 45 AMDF tests, 127 HSA tests (4 ignored), strict Clippy, and rustfmt.
Detached VM placement exposed a raw KFD GPU ID, forcing HSA to compare backend-specific numbers from otherwise neutral topology facts. Translate KFD DMA-BUF placement to an activated endpoint identity inside the backend. Owned VM backing records its endpoint directly, and HSA now matches that identity. Unknown native IDs leave placement unspecified. Verified with the focused known and foreign GPU import cases, all three crate library suites, strict Clippy, rustfmt, and git diff --check.
HSA core code selected KFD topology sources directly and consumed KFD node ordinals outside its Linux adapter. This tied memory and agent queries to one backend even though rocddi records native attachments. Expose native endpoint nodes, use source-neutral host and GPU node kinds, and isolate KFD CPU ordinals in the HSA Linux adapter. Generic driver sources can now report host and GPU nodes with CPU facts. Verified with a generic-driver topology test, 289 rocddi tests (2 ignored), 45 AMDF tests, 127 HSA tests (4 ignored), strict Clippy, and rustfmt.
Generic event subscriptions had only a KFD variant, so frontend-neutral routing could be checked only with a scripted native connection. Give the test driver a retained event stream with independent cursors and a wait hook. Verify two public subscriptions each receive the same event and advance independently through rocddi routing. Verified with 290 rocddi library tests (2 ignored), strict Clippy across all three DDI crates, rustfmt, and git diff --check.
The copy module combined target-specific packet layouts with native queue, resource-pool, and sequence ownership. That made packet changes hard to review against the transport lifetime rules. Move all SDMA encoders and their existing packet tests into a child module. The copy API and packet bytes are unchanged. Verified with 290 rocddi library tests (2 ignored), strict Clippy across the DDI crates, rustfmt, and git diff --check.
The test driver replayed every old event to a late subscriber, while KFD retains undelivered records plus only the latest record of each kind. Repeated same-kind events could pass a test and fail on KFD. Give test subscriptions sequence cursors and prune delivered history using the same latest-per-kind rule as KFD. Verify late replay after several same-kind records and the blocking wakeup path. Verified with all rocddi library tests, strict Clippy across the DDI crates, cargo fmt, and git diff --check.
The test driver covered event routing but could not create a queue through the public GPU path. Add a numeric SDMA transport and a test-only queue variant so native retirement failure and dependency lifetime can be tested without KFD hardware. Verified with the focused queue test, all rocddi library tests, strict Clippy across the three DDI crates, and git diff --check.
The KFD memory module mixed VM ownership with the scratch aperture range allocator. Move that allocator into a private module while keeping the existing reservation, ambiguous-programming retention, and range-reuse behavior. Verified with all rocddi library tests, strict Clippy across the DDI crates, and git diff --check.
AMDF memory ownership and pairwise cache recipes shared one large module. Move pair qualification and transition description into a private cache module while preserving the existing C entry points and host-mapping use of cache maintenance. Verified with all libamdf library tests, strict Clippy across the DDI crates, and git diff --check.
AMDF destruction could form mutable references before returning BUSY for live children. Those children can use shared parent references through their own handles. Check child counts through shared borrows first, and use a shared borrow throughout device destruction, which mutates no device fields. Verified with the libamdf library suite, strict Clippy across the three DDI crates, and git diff --check.
atgutier
force-pushed
the
users/agutierr/ddi-review-fixes
branch
from
October 28, 2026 22:35
f43831e to
ece35e4
Compare
Contributor
TheRock Submodule Bump ActivityNewest first
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.