Repository navigation
Conversation
ihhethan
requested review from
atgutier,
cfreeamd,
dayatsin-amd,
kentrussell and
shwetagkhatri
as code owners
August 25, 2026 23:40
ihhethan
changed the base branch from
users/ihhuang/rocr-dxg-topology-lifecycle
to
develop
August 25, 2026 23:41
ihhethan
requested review from
a team,
chrispaquot,
fcui-amd,
gandryey and
yuliashi-amd
as code owners
August 25, 2026 23:41
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
cfreeamd
reviewed
Aug 26, 2026
cfreeamd
left a comment
cfreeamd
left a comment
Contributor
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
Reviewed the failed-init unwind / DXG fork-safety rework. The staged KfdLifecycle ownership (drop-ownership-before-the-call, so a failed release is never retried into a double release), the DestroyTopology/DestroyLoaderAndExtensions/DestroyThunkLoader rollback guards in AMD::Load()/Runtime::Load(), the thunkLoader_ raw→unique_ptr change with the now-= nullptr thunk table backing LoadThunkApiTable()'s null-detection, and the libhsakmt DXG snapshot-refcount + atfork handling all look correct and are well-documented in place. New-file license headers are consistent with each directory's convention (NCSA for hsa-runtime/core, SPDX-MIT for the new libhsakmt header) — no action there. One clarifying question inline on the Close()/ShutDown() aliasing.
cfreeamd
reviewed
Aug 27, 2026
cfreeamd
left a comment
cfreeamd
left a comment
Contributor
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after author reply
Thanks for the clarification on the Close()/ShutDown() aliasing — that resolves the only open item. Confirmed: KfdDriver::Close() has no callers, the failure-guard path now uses ShutDown(), and delegating the pure-virtual Close() to the idempotent ShutDown() keeps KfdLifecycle ownership consistent (no separate handle-only teardown to drift or double-release). No other concerns; the staged-ownership unwind, scope-guard rollbacks, thunk-table = nullptr null-detection, and DXG snapshot-refcount/atfork handling all check out.
cfreeamd
approved these changes
Aug 28, 2026
bgopesh
reviewed
Aug 31, 2026
github-actions
Bot
added
the
github actions
A KFD driver acquires in three steps - open the device, take the topology snapshot, enable the runtime - and initialization can fail between any two of them. Those failures unwound out through AMD::Load() and Runtime::Load() to Runtime::Acquire(), which only drops the runtime reference count. Unload() never ran, so nothing gave back what the earlier steps had taken and the thunk's session stayed up for the life of the process. Introduce KfdLifecycle as the single owner of those references. It records which steps actually succeeded, and ShutDown() gives back exactly those, in the order a fully initialized runtime uses, whatever stage the failure happened at. Every owned step runs even if an earlier one fails, since stopping at the first error would strand the references the remaining steps give back. That makes ShutDown() correct on a half-built driver and idempotent afterwards, so an unwind on the failure path and a later normal shutdown cannot both release the same reference. Keeps projects/rocr-runtime/tests/unit_test_harness.h, which the sibling topology PR removes because the refcount test was its last consumer there; here tests/dxg/kfd_lifecycle_test.cpp still uses it.
… count hsaKmtCloseKFD() decremented dxg_open_count inside the if condition, so the last-close teardown ran with the count already at zero. That teardown releases GPU memory through hsaKmtFreeMemoryInternal(), whose first statement is CHECK_DXG_OPEN(), and that rejects every call once the count reads zero. So topology_drop_snapshot_at_last_close() reaching trim_suballocator() walked the block cache, failed every free with a pr_err, and cleared the cache regardless, leaking each block's GpuMemory without ever running its destructor. Recognize the last reference without consuming it, and decrement after the teardown instead. Count transitions and return values are unchanged; only the observation point moves. This also makes the close path agree with hsaKmtReleaseSystemProperties(), which already reaches topology_reset_snapshot_state() with the count at one or more, so the degraded path now converges on the sequence the healthy path exercises. The ordering came in with the shared snapshot-refcount commit, so it is present on both this branch and the topology-snapshot PR. It is fixed here because that PR merges first and should not be disturbed. Triggering it needs both a snapshot still live at last close, which topology_drop_snapshot_at_last_close() already warns is the caller getting the order wrong, and WSL_ENABLE_THUNK_SUB_ALLOCATOR set, since the block cache is otherwise never populated. Fixed rather than left latent because the ordering hazard should not depend on a default staying off.
a_complete_lifecycle_releases_in_reverse_order asserted the disable, release, close sequence on a fully acquired lifecycle. So does shutdown_continues_after_errors_and_reports_the_first_one, which asserts the same vector and additionally pins down what ShutDown() returns when every step fails, so the first case only restated a weaker form of the second and is removed. AcquireAll() wrapped its three acquiring calls in CHECK_EQ against HSA_STATUS_SUCCESS while passing lambdas that return exactly that. Those assertions could not fail, and the ownership they stood in for is already covered: a stage that failed to record itself leaves its entry out of the recorded call list, which every case using the helper checks directly. The calls stay, the assertions go. Recorder::Ops() built KfdLifecycleOps by positional brace initialization and labelled each lambda by the position it happened to occupy, so reordering the fields of KfdLifecycleOps would have relabelled all three and left every case asserting a sequence that no longer matched what it named. Assign by field name, as KfdDriver::ThunkOps() already does. What these cases still cannot see is ThunkOps() itself, which decides which thunk entry point lands in which field. A file comment now records that gap and why closing it is not worth the cost: ThunkOps() is private to KfdDriver, HSAKMT_CALL() resolves through the Runtime singleton whose constructor and thunk loader are both private, and amd_kfd_driver.cpp pulls in the GPU agent, the memory regions and the code-object loader.
Both comments named the librocdxg refcount test, which went away with the SnapshotRefcount class, so each described a file that no longer exists. check_dynamic_exports.cmake cited that test as its evidence that nothing else in the directory reaches the built artifact. The claim it supports is still true and still worth stating, so it now cites what is actually there: the lifecycle test links amd_kfd_lifecycle.cpp out of hsa-runtime and never loads librocdxg, leaving this the only test that speaks for what ROCr's dlsym() will find. The BUILD_ROCDXG_TESTS comment in CMakeLists_wsl.txt described a test that compiles a header out of src/dxg on its own, which is wrong twice over: the refcount test is gone, and the test that replaced it drives the KFD lifecycle state machine with injected thunk calls instead. Reworded to the two targets that exist.
KfdDriver::Close() had become { return lifecycle_.ShutDown(); }, byte for
byte the same as KfdDriver::ShutDown(). core::Driver draws the line the
other way: Close() closes a connection to an open driver, and ShutDown()
releases the driver's resources and closes the kernel-mode driver. All
three implementors read it that way. On develop KfdDriver::Close() was
hsaKmtCloseKFD() alone, KfdVirtioDriver::Close() is vhsaKmtCloseKFD()
alone, and XdnaDriver::Close() only closes its fd. In both KFD drivers
ShutDown() then ended by calling Close(), which is the shape restored
here.
Add KfdLifecycle::Close(), which gives back the open reference and nothing
else, and point the driver at it. ShutDown()'s close stage now goes
through the same method rather than repeating the ownership drop and the
call, so that sequence exists once while the two public methods stay
distinct: Close() narrow, ShutDown() broad. Close() keeps the ownership
discipline the other stages already follow, clearing owns_open_ before the
thunk call, so a close that fails is not retried into a double close by a
later ShutDown().
No caller changes behaviour. Nothing in runtime/ reaches
KfdDriver::Close() today: the only polymorphic core::Driver::Close() call
site is in XdnaDriver::DiscoverDriver, which never sees a KfdDriver, and
the KFD teardown path goes through ShutDown(). That holds on native Linux
KFD as well as on the WSL DXG path, since neither routes shutdown through
Close().
Worth the few lines anyway, because Close() is pure virtual and so cannot
simply be dropped. Leaving it as a full teardown means the next caller to
reach for it through the base class silently loses the runtime enable and
the topology snapshot when all it asked for was to close a handle, and the
symptom would surface far from here.
ihhethan
force-pushed
the
users/ihhuang/rocr-hsa-init-lifecycle
branch
from
August 31, 2026 19:47
d5787b5 to
d11f584
Compare
cfreeamd
reviewed
Aug 31, 2026
cfreeamd
left a comment
cfreeamd
left a comment
Contributor
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up triggered by bgopesh's comment
Independently verified bgopesh's finding against the diff. The open/close count imbalance is real: KfdDriver::Open() passes a lambda to lifecycle_.Open() that returns HSA_STATUS_SUCCESS for both a genuine open and for HSAKMT_STATUS_KERNEL_ALREADY_OPENED (DXG path); lifecycle_.Open() then sets owns_open_ = true; but openclose.cpp's already-opened branch doesn't increment dxg_open_count. When the second consumer tears down, Close() → hsaKmtCloseKFD() decrements the count from 1 → 0, triggering topology_drop_snapshot_at_last_close() + HeapFini() + close(dxg_fd) while the first consumer is still alive. This is the multi-consumer ROCr + rocprofiler-sdk scenario this PR is designed to support. Flagging for @cfreeamd to reconsider before merge.
Resolves one conflict, in core/inc/thunk_loader.h. Both sides had rewritten the block of pfn_* member declarations. This branch, in 565fcfa, gave every member an explicit = nullptr initializer and wrapped the declarations that then ran past the 100 column limit, so that a member the loader never bound reads as null rather than as an indeterminate value. develop meanwhile added a new entry point in the same block, hsaKmtGetDefaultHostGpu. Kept this branch's = nullptr form and wrapping for every member, and added develop's new member in the position develop gives it, between hsaKmtAvailableMemory and hsaKmtRegisterMemory, carrying the same initializer. Rather than read the hunk, the two sides were compared by extracting the HSAKMT_PFN and DRM_PFN identifiers from each and diffing the sets: develop had exactly one member this branch lacked, this branch had none that develop lacked, and the resolved block now has the same 125 declarations in the same order as develop's. The matching typedef sits above the conflict region, merged cleanly on its own, and was left as develop wrote it. The added entry point is required rather than optional, so it is worth saying why that is safe. thunk_loader.cpp merged cleanly and binds the symbol with the same goto LOAD_ERROR treatment develop gives it, meaning a thunk that fails to export it fails to load. librocdxg does export it: the name is listed in librocdxg.ver, declared in hsakmt.h and defined in src/dxg/memory.cpp. The merged loader also classifies all 128 bound symbols exactly as develop does, leaving the same eight allowed to remain null, so this branch imposes no new requirement on the DXG thunk and the WSL path is unaffected.
KfdDriver::Open() treats HSAKMT_STATUS_KERNEL_ALREADY_OPENED as success on the DXG path, which is safe only because the thunk increments dxg_open_count on its already-opened branch just as it does on the first open. That increment is one line in libhsakmt/src/dxg/openclose.cpp, a different file in a different project directory, and nothing at the call site pointed to it. Two reviewers independently read the accepting branch as a refcount bug, each concluding that the already-opened path takes no reference and that recording ownership here therefore double-counts. Both conclusions were wrong: the increment has been there since the DXG thunk's initial commit and is present on develop as well as on both PR branches. But two experienced readers making the same mistake is a fair verdict on the code rather than on the readers, so record the invariant at the point that relies on it. Also notes the native-KFD asymmetry, because it is the kind of detail a later reader removes while tidying. libhsakmt/src/openclose.c increments hsakmt_kfd_open_count on its already-opened branch too, so the same argument would justify accepting the status there, yet this path still reports an error for non-DXG and a second native open strands a reference. That is pre-existing on develop and deliberately left alone here. Comment only; no functional change.
cfreeamd
reviewed
Sep 1, 2026
cfreeamd
left a comment
cfreeamd
left a comment
Contributor
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after 2026-08-31 author commits
Six real author commits landed after the last cfreeamd comment. The SnapshotRefcount → std::atomic refactor, topology_drop_snapshot_at_last_close, fork-handler cleanup (dxg_open_count = 0 + topology_clear_snapshot_refs() in the child, lock_guard moved after clear_after_fork() to avoid unlock-on-freed-memory), hsaKmtCloseKFD teardown order (count == 1 check before decrement so HeapFini can call into the thunk), snapshot refcounting in hsaKmtAcquireSystemProperties / hsaKmtReleaseSystemProperties, and the new DXG unit tests (lifecycle state machine + dynamic-exports nm check) all check out.
One [Important] item remains: the multi-consumer DXG open/close imbalance (bgopesh's bug) is not fixed.
See inline comment on amd_kfd_driver.cpp.
A shared thunk is allowed not to export five entry points: hsaKmtSetSigbusDelay and the four external-semaphore calls. Until now LoadThunkApiTable() left each unresolved one null and every caller was obliged to test it first - an invariant kept up by five call sites remembering to, and one a sixth caller would have had to be told about. Bind them instead. On the shared-library path an entry point that does not resolve now gets a stub returning HSAKMT_STATUS_NOT_SUPPORTED, which is already the status libhsakmt returns for an entry point it exports but does not implement (see libhsakmt/src/extsem.c). Every pointer in the table is callable, the call sites drop their null checks, and the two states a caller had to distinguish collapse into one status. The statically linked path is left alone: there every symbol is bound to the real thing, so a stub would only mask a build problem. Import and destroy are bound as a pair. ImportExternalSemaphore() used to refuse an import when the destroy half was missing, since a semaphore it could not give back would be stranded; with stubs in place it can no longer see that from the pointer, so the loader enforces the pairing and stubs both when either is missing. The rule moves rather than disappears. The status each call site returns is otherwise unchanged. SetSigbusDelay() maps NOT_SUPPORTED to HSA_STATUS_ERROR_NOT_SUPPORTED, which is what its null check returned; DestroyExternalSemaphore() maps it to HSA_STATUS_ERROR_INVALID_AGENT, which is what lets hsa_amd_external_semaphore_handle_close() keep polling the other drivers; Signal and Wait already routed it through MapQueueExtSemStatus(). Destroy gains that mapping on native KFD too, where extsem.c's unimplemented stub used to surface as HSA_STATUS_ERROR and stop the poll early. Nothing was compiled; this machine has no compiler. clang-format 18 ran over the touched lines only, so the long GetExportAddress assignments keep the one-line form the rest of the table uses.
ShutDown() tested InheritedAcrossFork() and cleared all three claims itself, so a driver object in a forked child did nothing when any one stage was called directly. Close() tested it a second time and cleared kfd_opened_; DisableRuntime() and ReleaseTopologySnapshot() did not test it at all, which was the only reason ShutDown() needed the branch. Reached through ShutDown(), Close()'s own check was unreachable - the same rule written twice with one copy dead. Move the check into the two stages that lacked it. Each now gives up its claim and skips its thunk when the claim came across a fork, so each is correct called directly or through ShutDown(), and ShutDown() collapses to its three calls with no special case. The end state is unchanged: all three flags clear, HSA_STATUS_SUCCESS, no thunk called. None of the three can safely call its thunk in a child, so all three keep a guard rather than losing one. hsaKmtRuntimeDisable() has no fork check and would issue AMDKFD_IOC_RUNTIME_ENABLE with the disable mask on a descriptor the child inherited rather than opened. Only DXG's hsaKmtReleaseSystemProperties() refuses a child on its own, and only while is_forked is set - once the child reopens, that guard is gone and a stale release consumes the child's own reference; the KFD one has no check and would destroy process apertures and doorbells this process never built. Fixing the implementations underneath, as the review asked, would mean libhsakmt: the open count child_fork_handler() resets is process-global rather than per-consumer, which is what makes a second close destructive rather than merely wasteful. That is a different component. Moving the check down one level meets the requirement without crossing that boundary. Nothing was compiled; this machine has no compiler.
ihhethan
force-pushed
the
users/ihhuang/rocr-hsa-init-lifecycle
branch
from
September 18, 2026 20:23
8c59dd7 to
c4a2d0b
Compare
dayatsin-amd
requested changes
Sep 21, 2026
A Load() that does not succeed leaves the singleton half built - drivers
opened and registered, agents published, the thunk held open - and
Unload() never runs, because the reference count never stayed at one.
The rollback added earlier in this branch existed to hand that state
back so a second hsa_init() could be run over it.
There is no second hsa_init(). A failed one is fatal for the process and
the process has to exit, so ROCr owes the caller a consistent error, not
a singleton prepared for another attempt. Acquire() now latches the
failure - an error return or an exception, out of the singleton's
construction as much as out of Load() - and returns the same
HSA_STATUS_ERROR_OUT_OF_RESOURCES to every later call without building
on what the failed attempt left behind. hsa_shut_down() cannot reach
Unload() either: it gates on IsOpen(), and the reference count is back
at zero.
The latch is armed only for the call that performs the initialization,
which is the call that finds a null singleton: Release() is the only
other writer and it nulls the pointer on the same transition that
deletes it, so a non-null singleton belongs to an initialization that
finished. Nested hsa_init(), HSA_STATUS_ERROR_REFCOUNT_OVERFLOW on a
live runtime, and a fresh hsa_init() after a balanced hsa_shut_down()
are therefore unaffected.
With no second attempt to serve, the rollback goes:
- Runtime::DestroyTopology() and the AMD::Load() guard that ran it.
- Runtime::Load()'s loadGuard and thunkGuard, with their Dismiss()
calls on the success path.
- DestroyLoaderAndExtensions() and DestroyThunkLoader(), each left
with a single caller; their bodies go back inline in Unload().
What stays is the unwinding one attempt owes itself: InitializeDriver()'s
ShutDown() over the driver whose Init() failed, KfdDriver::Init()'s scope
guards, and AMD::Unload() shutting every driver down rather than stopping
at the first error.
hsaKmt is a closed API and ROCr is the only component that should call hsaKmtOpenKFD(), so the number of opens is ROCr's to know rather than something to defend against. ROCr opens once: KfdDriver::DiscoverDriver() is the only Open() call site, and it registers the driver only when that call succeeds. The early return this removes defended against a second Open() that cannot happen, and kfd_opened_ existed because the rollback for a failed load called ShutDown() twice on the same driver - once from InitializeDriver()'s guard and once through AMD::Unload(). That rollback is gone, so the state it needed goes too. Every remaining path closes at most once: one Open() and one Close() over a successful lifecycle, one of each when Init() fails, and none at all when DiscoverDriver()'s Open() fails, because the driver is never registered. Open() goes back to develop's body, including the DXG HSAKMT_STATUS_KERNEL_ALREADY_OPENED branch, which predates this branch and is unrelated to the removed state. Close() keeps the forked-child check: a child holds none of the claims it inherited, and neither hsaKmtCloseKFD() implementation tests for a fork on its own.
Contributor
|
And please change the PR description. This is not fixing a defect in ROCr initialization. This is changing initialization behavior when profiling is enabled. |
dayatsin-amd
requested changes
Oct 1, 2026
Resolve thunk_loader.h against develop's thunk-loader platform split (#7125) and the GL2 residency entry point (#10183): keep this branch's nullptr-initialized table and add hsaKmtSetPersistingCacheSize to it. Bind hsaKmtSetPersistingCacheSize as an optional entry point, like hsaKmtSetSigbusDelay. librocdxg does not export it, so as a required export it would make LoadThunkApiTable() fail and hsa_init() refuse to start under WSL. The static path defaults it to the NOT_SUPPORTED stub and the Linux BindPlatformStaticApis() binds the libhsakmt implementation, which was otherwise left unbound.
The DXG hsaKmtOpenKFD() stores the descriptor it opens in dxg_runtime->dxg_fd before it loads libdxcore. When that load fails, the error path closes the descriptor but leaves the cached copy behind, and the open count stays at zero. The next open then finds dxg_fd >= 0, skips opening /dev/dxg, and adopts the closed descriptor number. By then the number may belong to an unrelated file: amdgpu_device_get_fd() hands it out, and the eventual last hsaKmtCloseKFD() closes it. Reset dxg_fd together with closing the descriptor, and only for a descriptor this call opened.
…opens hsaKmtOpenKFD() checks is_forked_child() and runs clear_after_fork() before it takes hsakmt_mutex, because clear_after_fork() deletes and replaces *dxg_runtime, and the mutex with it. The comment justified running that pair unlocked by saying only the forking thread survives into the child. That holds at the instant of fork(), not at the child's first open. A child that has started threads by then can make several first opens at once, and so can any process forked from one that merely loaded librocdxg, since a pid change is all is_forked_child() can see. Each open sees the fork, each runs clear_after_fork(), and the second deletes the runtime the first is about to lock. With four threads released together into their first open in such a child, the unmodified library crashed or deadlocked in 44 of 150 iterations and returned a wrong status in 5 more. Give the pair a file-scope mutex of its own. It lives outside hsakmtRuntime, so clear_after_fork() does not destroy it, and the atfork handlers take it before hsakmt_mutex and reset it in the child the way they reset hsakmt_mutex. No path takes it while holding hsakmt_mutex, and clear_after_fork() does not take hsakmt_mutex, so the lock order is fixed. This orders first opens against each other only. Another entry point running on a second thread while a child's first open recovers can still reach the runtime being replaced; the comment now says so instead of claiming there is nothing to exclude.
The DXG thunk counts hsaKmtOpenKFD() and hsaKmtAcquireSystemProperties() references per process, so more than one component can use the topology entry points at once: ROCr, and on WSL amdsmi and rocprofiler-sdk. Neither hsaKmtCloseKFD() nor hsaKmtReleaseSystemProperties() says which reference it gives back. That is enough within one process, but not across fork(). The child inherits every component's references along with that component's state, and each component may give its reference back there; ROCr's KfdDriver does so from hsa_shut_down() and from its destructor at exit. Today the child handler zeroes the counts, so: - before the child reopens, those inherited closes and releases fail with KERNEL_IO_CHANNEL_NOT_OPENED; - after a component in the child reopens and acquires, the first inherited release or close is counted against the child's own reference and tears down the snapshot or DXCore session it is still using; - when the fork bypassed pthread_atfork, hsaKmtCloseKFD() and hsaKmtReleaseSystemProperties() never notice it and run the last-close and last-release teardown over the parent's WDDMDevice objects. Move the inherited counts aside instead of zeroing them: into a file-scope inherited_open_count, which has to survive clear_after_fork() replacing hsakmtRuntime, and into _topology_props::inherited_snapshot_refs_. child_fork_handler() does this as before, and is_forked_child() now does it too on the first call that notices a fork the handler did not see. hsaKmtCloseKFD() and hsaKmtReleaseSystemProperties() call is_forked_child() under hsakmt_mutex and match a close or release to an inherited reference before a live one. An inherited reference owns nothing in this process, so giving one back tears nothing down. Matching inherited references first is what makes a count without identity safe here. Teardown needs one close per inherited open plus one per live open. While any caller in the child still holds a live open, fewer closes than that have arrived, so no stale close can reach the child's teardown. The cost is that teardown waits until every inherited holder has closed, or until the child exits. hsaKmtAcquireSystemProperties() and hsaKmtRuntimeDisable() also call is_forked_child() first, so a child that has not reopened refuses to take a reference on the parent's snapshot and has nothing to disable, with or without the atfork handler. topology_drop_snapshot_at_last_close() loses its forked-child branch: a child can no longer reach the last close without having opened. Add dxg_lifecycle_test under BUILD_ROCDXG_TESTS. It drives the built library through its public entry points, each case in its own child of a runner that never opens the thunk: unmatched calls, nested opens and acquires, a last close with a snapshot outstanding, concurrent consumers, and a child giving back inherited references or closing them after reopening, with libc fork() and with a raw fork system call. A second invocation loads a libdxcore.so with no D3DKMT exports, for the open that fails after opening /dev/dxg. Both report themselves skipped (77) without /dev/dxg and a DXCore adapter.
KfdDriver recorded the pid that constructed it, and DisableRuntime(), ReleaseTopologySnapshot() and Close() each skipped the thunk when called from another process. That had ROCr track ownership the thunk is in a position to track itself, and review asked for it to live in the DXG implementation of hsaKmtOpenKFD()/hsaKmtCloseKFD() and hsaKmtAcquireSystemProperties()/hsaKmtReleaseSystemProperties() instead. The DXG thunk now does that: it counts inherited references apart from live ones and gives inherited ones back first, so a forked child's KfdDriver::ShutDown() gives back exactly what it inherited without touching an open or snapshot a component made in the child. Remove owner_pid_, InheritedAcrossFork() and the three skips. The three stages and ShutDown() go back to develop's fork behavior. Nothing here makes the native libhsakmt calls multi-consumer: Open() still accepts HSAKMT_STATUS_KERNEL_ALREADY_OPENED only from the DXG thunk. On native KFD this returns a forked child to develop's behavior rather than adding an exposure. KFD refuses ioctls on a descriptor used from a process other than the one that opened it, so hsaKmtRuntimeDisable() fails in a child without reaching the parent's enable, and the release and last-close teardown in libhsakmt/src act on the child's own copy of the mappings and bookkeeping.
Resolve hsa/signal_pool.{hpp,cpp} against develop's fix for the same
pooled-signal leak (#12283), which also reuses an existing handle and
resets its value. Keep a single implementation: develop's
reset_hsa_signal() helper, handle comparison and warning about ignored
creation arguments, with this branch's null check on
hsa_signal_store_screlease_fn moved into the helper and its rationale
comments kept. destroy_hsa_signal() and clear() retirement are
unchanged, and the header keeps this branch's construct_hsa_signal()
documentation, which already covers develop's.
Resolve the header comment of libhsakmt/tests/dxg/CMakeLists.txt
against #11897: the lifecycle test here needs a live adapter and skips
without one, while runtime-level tests such as
rocrtstFunc.Heap_Reservation stay in rocrtst.
Contributor
|
Changes to libhsakmt and ROCr look ok to me. Can you either:
These tests will not run as part of the CI's with the way they are implementated at the moment. |
Resolve the ThunkLoader conflicts against #12845 and #11606, which add the optional hsaKmtGetClockCountersPrecise and hsaKmtQueryDmaBufInfo entry points. Both are declared in this branch's nullptr-initialized member list and stay null when a shared thunk does not export them, because their callers in KfdDriver test the pointer. They are not bound to a NOT_SUPPORTED stub like hsaKmtSetSigbusDelay and the four external-semaphore entry points.
Per review, the open/snapshot lifecycle cases are not moved into KFDTest; they are removed instead. Delete lifecycle_test.cpp and its libdxcore stub, and return tests/dxg/CMakeLists.txt, check_dynamic_exports.cmake and the BUILD_ROCDXG_TESTS comment in CMakeLists_wsl.txt to develop's text, so tests/dxg is unchanged by this branch. The dynamic export check stays as it is on develop.
Contributor
Author
Thanks, David. We went with your second option. The DXG lifecycle and partial-open tests are removed and nothing was added to KFDTests, so tests/dxg in this PR is now identical to develop. |
Resolve the XdnaDriver::ShutDown() conflict against #13740, which adds the same teardown this branch did: FreeDeviceHeap() while fd_ is still open, then Close(), both run regardless of the first result, with the heap error ranked ahead of the close error. Take develop's body, so this branch no longer changes amd_xdna_driver.cpp. InitializeDriver()'s guard and amd::Unload() still reach the device only through ShutDown(), and a repeated ShutDown() is a no-op because FreeDeviceHeap() and Close() reset dev_heap_bo and fd_.
This branch has not been deployed
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.
Motivation
This PR updates initialization and teardown behavior when profiling is enabled, particularly on the WSL/DXG path, where driver, topology, thunk, and signal lifetimes can overlap.
The profiling path exposed three related ownership and lifecycle issues:
These issues were identified while validating WSL profiling support. The changes were split out of [#10034] to keep that PR focused on topology snapshot support.
Technical Details
ROCr initialization and teardown
hsa_init()attempt instead of retrying initialization on a partially initialized singleton.hsa_init()calls and allow fresh initialization after a balancedhsa_shut_down().KfdDriver::ShutDown()attempt all teardown stages—disabling the runtime, releasing topology, and closing the driver—even if an earlier stage fails.ShutDown()when a driver'sInit()fails to release resources acquired during the failed initialization attempt.AMD::Unload()shut down all registered drivers instead of stopping at the first error.XdnaDriver::ShutDown()to close the device after releasing its heap.agents_by_gpuid_when agents are destroyed.fork()to the thunk rather than tracking process ownership in ROCr.Thunk loading
ThunkLoaderwithstd::unique_ptr.HSAKMT_STATUS_NOT_SUPPORTEDstubs:hsaKmtSetSigbusDelayhsaKmtSetPersistingCacheSizeDtifCreateandDtifDestroybefore creating a DTIF instance.librocdxg lifecycle ownership
hsaKmtOpenKFD()/hsaKmtCloseKFD()hsaKmtAcquireSystemProperties()/hsaKmtReleaseSystemProperties()fork()separately from references acquired by the child.pthread_atfork./dev/dxgdescriptor if DXCore initialization fails.rocprofiler-sdk signal and object pools
signal_t::matches().clear()instead of freeing objects that may still be reachable by asynchronous handlers.Test Plan
Issue Tracking
JIRA ID: ROCM-23912
Test Result
Submission Checklist