Repository navigation
fix(hrr): make the capture writer safe across fork and in the crash path - #12255
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Address unsafe child reinitialization after fork() and concurrent emergency-buffer access.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
Updates the HRR capture writer for safer fork() handling and crash-time finalization.
Changes:
- Coordinates writer state across
fork(). - Synchronizes crash-buffer access.
- Moves emergency manifest storage off the crashing thread’s stack.
| File | Summary |
|---|---|
projects/clr/hipamd/src/hrr/hip_capture_writer.cpp |
Implements fork synchronization and crash-path buffer changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
17a8f7c to
887def7
Compare
danielbolsson
left a comment
There was a problem hiding this comment.
Reviewed with a focus on fork safety and the crash path. No blocking issues from me. Holding g_blob_mu and g_file_mu across fork() and releasing them in the parent and child handlers is the right shape. I found no path that takes them in the opposite order. glibc also runs the parent handler when fork() fails, so a failed fork does not leave them locked. The other pthread_atfork prepare handlers in the tree (libhsakmt, rocSHMEM) take locks that are never held while calling into the HRR writer, so the prepare chain has no new cycle.
On the crash side, both new function-local statics are constant-initialized (ATOMIC_FLAG_INIT and a zero-initialized array), so they go in .bss with no __cxa_guard_* and are safe in the handler. claim_buf_locked() also fixes a subtle problem in the old guard: the old guard ignored the test_and_set result, and its destructor then cleared a flag that the crash callback held.
I left two small inline items, one non-blocking and one nit.
887def7 to
d9a8d5f
Compare
danielbolsson
left a comment
There was a problem hiding this comment.
Re-reviewed at d9a8d5f. Both of my earlier comments are addressed.
Lock order checks out: open() and index_existing_blobs_locked() take g_blob_mu without g_file_mu held, so atfork_prepare() is the only path that holds both. atfork_child() clears both flags. The emergency manifest flag is released on the HRR_OPEN failure path. The unbounded claim_buf_locked() wait is fine: the crash callback holds the flag only for a single flush.
Non-blocking: the fork and crash cases are only tested manually. A small fork test that checks the child archive and manifest would be worth adding in a follow-up.
Unit_HRR_CaptureCrashOnSmallStack dies of SIGSEGV on a thread with a 64 KiB stack and checks the manifest it leaves says "complete": false. CLR's crash handler runs on the faulting thread's stack, so the old on-stack manifest buffer overflowed there. Unit_HRR_ForkWhileRecording forks 200 times while another thread records and fails if a child does not exit within its deadline. It then expects one archive per child besides the parent's. The lock windows are narrow, so it catches a regression by chance rather than on every run. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The child atfork handler still invokes non-async-signal-safe C++ and filesystem operations that can deadlock after a multithreaded fork.
Review effort: Balanced
Findings: 1
Open (2)
Resolved since last review (1)
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
The atfork child handler reopened the archive itself, but the child of a multithreaded process may make only async-signal-safe calls until it execs, and open() allocates, takes locks and goes through stdio and the filesystem library. The handler now only drops the parent's events fd and paths, and the first record, blob or code object in the child opens its pid-<pid> sub-archive. A child that execs or exits without recording leaves no archive; the fork test now has every other child record a call and counts archives accordingly. Co-Authored-By: Claude <noreply@anthropic.com>
The develop merge brought in hrr_disk_space_test.cc, which defines fsync for the integration test binary to fake the output file system. The fork tests in hrr_workload_test.cc already defined fsync to hold a thread at the writer's checkpoint, so the binary failed to link with a duplicate symbol. The workload file now provides hrr_workload_fsync_hook, which runs the fork tests' hook, and the disk-space fsync calls it before doing what it did before. Co-Authored-By: Claude <noreply@anthropic.com>
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
# Conflicts: # projects/clr/hipamd/src/hrr/hip_capture_writer.cpp
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
The capture shutdown writes the trailer and the manifests, then closes events.bin. A thread that recorded in between, such as a forked child's first record finishing the open of its archive, landed after the trailer: the trailer then undercounted the events, and a resume dropped the record. flush() now marks events.bin finalized, with or without a trailer, and a record after that is dropped, as one after close() is. Unit_HRR_RecordAfterCaptureShutdown holds a second thread inside a recorded call until the shutdown reaches its first manifest. It then checks that events.bin ends in the trailer and that the trailer counts every event. The fork shutdown case checks the same, since load_archive() reads on past a trailer.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
An unresolved fork/shutdown race needs correction, and the cross-platform synchronization changes warrant final human review.
Review effort: Balanced
Findings: 1
Open (1)
Resolved since last review (2)
hip_capture_shutdown() runs flush(), which writes the trailer and the manifests, then close(), which closes events.bin. A thread that forks in between left the child with a reopen flag, because the parent's fd was still open. The child's first record then opened a new archive that nothing finalized: the parent's exiting thread runs the shutdown, so it is no longer pending in the child. atfork_child now arms the reopen only for an archive that is open and not finalized. Unit_HRR_ForkBetweenCaptureFlushAndClose holds the shutdown at the first lock close() takes, through a pthread_mutex_lock hook, while a second thread forks. The child records through the compiler dispatch table. The test checks that only the parent has an archive, and that it is complete.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
Resolve hip_capture_writer.cpp against #12255: the pinned host snapshot fork hooks run around the writer's fork handlers, prepare before the writer takes its locks and parent/child after it releases them, and the snapshot counters reset in open() inside the locked publish block.
TheRock Submodule Bump ActivityNewest first
|
…ath (#12255) * fix(hrr): make the capture writer safe across fork and in the crash path * fix(hrr): clear the emergency manifest flag in a forked child * docs(hrr): state the writer lock order and what bounds the buffer wait * test(hrr): cover fork while recording and a crash on a small stack Unit_HRR_CaptureCrashOnSmallStack dies of SIGSEGV on a thread with a 64 KiB stack and checks the manifest it leaves says "complete": false. CLR's crash handler runs on the faulting thread's stack, so the old on-stack manifest buffer overflowed there. Unit_HRR_ForkWhileRecording forks 200 times while another thread records and fails if a child does not exit within its deadline. It then expects one archive per child besides the parent's. The lock windows are narrow, so it catches a regression by chance rather than on every run. * fix(hrr): reopen a forked child's archive on its first record The atfork child handler reopened the archive itself, but the child of a multithreaded process may make only async-signal-safe calls until it execs, and open() allocates, takes locks and goes through stdio and the filesystem library. The handler now only drops the parent's events fd and paths, and the first record, blob or code object in the child opens its pid-<pid> sub-archive. A child that execs or exits without recording leaves no archive; the fork test now has every other child record a call and counts archives accordingly. * fix(hrr): declare the fork reopen state apart from develop's new globals develop now declares the unreplayable-API set right after g_written_blobs, where the reopen flag and mutex were added, so the branch no longer merged cleanly. Declare them before the blob set instead. No change in behaviour. * fix(hrr): clear the inherited capture-incomplete flag in a forked child The child records into a new archive, so an event the parent dropped before the fork does not make the child's archive incomplete. * fix(hrr): stop the fork-while-recording loop at the first hung child A regression that hung every child would have waited ten seconds for each of them, past the roundtrip case's timeout, and left the later ones behind. The loop now stops after killing and reaping the first. * test(hrr): cover the writer mutex across fork and a crash during fork Unit_HRR_ForkWhileWriterHoldsLock forks at the moment another thread could take the capture writer's events mutex if the writer's prepare handler let it go before fork(), and checks that no thread got it and that the child, which reopens its archive under that mutex, exits. Unit_HRR_CaptureCrashDuringFork forks while the parent's crash callback owns the emergency manifest buffer and checks that the child still writes its own manifest when it crashes. Both stop the writer at an fsync() that the test binary interposes; with no hook set it is libc's fsync. * fix(hrr): hold the unreplayable-API mutex across fork atfork_prepare() now also takes g_unreplayable_mu, after the other writer mutexes, and both fork handlers release it. A thread inside note_unreplayable() at fork() otherwise left the child that mutex locked, and a child that recorded and then exited normally blocked for ever when its capture shutdown wrote the manifest under it. Unit_HRR_ForkWhileNotingUnreplayable holds a thread inside note_unreplayable() with a strlen() that the test binary interposes, forks, and has the child record and exit through its capture shutdown. It checks that the child exits, that fork() waited for the held thread, and that both manifests list the API. * fix(hrr): keep a forked child from opening its archive after close writer::close() now clears g_reopen_after_fork under g_reopen_mu. A forked child that had not recorded when its capture shutdown ran kept the flag set, so a later record, such as the one a fat-binary destructor makes through the compiler dispatch table, opened a new archive that nothing finalized. Unit_HRR_ForkedChildRecordsAfterShutdown forks a child that exits normally without recording and records after its capture shutdown, and checks that only the parent has an archive. * fix(hrr): keep a child forked after close from opening an archive atfork_child() set the reopen flag whenever the writer had a base directory, also in a process that had already closed its writer. A child forked after the parent's capture shutdown then opened its own archive on its first record, and nothing finalized it. The child now sets the flag only when the parent had its events file open. A parent that is itself a child yet to open its archive passes its flag on unchanged. Unit_HRR_ForkAfterCaptureShutdown forks after the capture shutdown, records in the child and checks that only the parent has an archive. * fix(hrr): finalize a forked child's archive only after it is open A forked child opens its archive on its first record, under g_reopen_mu only. open() published g_output_dir and the events fd before the file header was in the buffer, and without the writer mutex. A capture shutdown on another thread of the child could then write the manifest as complete, or the trailer before the header. A checkpoint or the crash callback could flush the buffer while open() reset it. open() now prepares the archive in locals and publishes it under the writer mutex and g_buf_busy, header included. It still takes g_blob_mu before g_file_mu and never both at once. flush() takes g_reopen_mu, so it waits for an open in progress, and clears the reopen flag, so no archive opens after it. Unit_HRR_ShutdownWhileChildOpensArchive holds a child's recording thread in that open, with an mkdir() defined in the test binary, while the child's main thread exits. It checks that both archives end in the clean-shutdown trailer and have a complete manifest. * test(hrr): record in the forked child of the disk-space fork tests A forked child now opens its archive on its first record rather than in atfork_child(). The children of Unit_HRR_DiskSpace_RefusedForkChild and Unit_HRR_DiskSpace_ForkAfterStop exited without recording, so the refused child never reached open() and printed no "Capture disabled", and the fork-after-stop child could no longer show that it ignores the stop. Each child now pushes and pops a launch configuration before it exits, which records through the compiler dispatch table without touching the device. * test(hrr): share one fsync between the fork and disk-space tests The develop merge brought in hrr_disk_space_test.cc, which defines fsync for the integration test binary to fake the output file system. The fork tests in hrr_workload_test.cc already defined fsync to hold a thread at the writer's checkpoint, so the binary failed to link with a duplicate symbol. The workload file now provides hrr_workload_fsync_hook, which runs the fork tests' hook, and the disk-space fsync calls it before doing what it did before. * fix(hrr): drop a record that comes after the capture's trailer The capture shutdown writes the trailer and the manifests, then closes events.bin. A thread that recorded in between, such as a forked child's first record finishing the open of its archive, landed after the trailer: the trailer then undercounted the events, and a resume dropped the record. flush() now marks events.bin finalized, with or without a trailer, and a record after that is dropped, as one after close() is. Unit_HRR_RecordAfterCaptureShutdown holds a second thread inside a recorded call until the shutdown reaches its first manifest. It then checks that events.bin ends in the trailer and that the trailer counts every event. The fork shutdown case checks the same, since load_archive() reads on past a trailer. * fix(hrr): open no archive in a child forked after the capture's flush hip_capture_shutdown() runs flush(), which writes the trailer and the manifests, then close(), which closes events.bin. A thread that forks in between left the child with a reopen flag, because the parent's fd was still open. The child's first record then opened a new archive that nothing finalized: the parent's exiting thread runs the shutdown, so it is no longer pending in the child. atfork_child now arms the reopen only for an archive that is open and not finalized. Unit_HRR_ForkBetweenCaptureFlushAndClose holds the shutdown at the first lock close() takes, through a pthread_mutex_lock hook, while a second thread forks. The child records through the compiler dispatch table. The test checks that only the parent has an archive, and that it is complete.
…ddresses (#13186) ## Motivation Replay gives every allocation a new address. A kernel that reads a device address the application stored in memory, rather than received as an argument, then reads the capture-time address and faults. vLLM's MRV2 path copies the block table's address to the device as plain data, and `_compute_slot_mappings_kernel` faults on replay (ROCM-31827). This change places `hipMalloc` and `hipMallocAsync` allocations at the addresses capture recorded. An allocation that cannot be placed falls back to the usual allocator. ## Technical Details - Before `hipInit`, replay holds the recorded `hipMalloc` and `hipMallocAsync` ranges with `PROT_NONE` placeholders, so the runtime cannot take them. Allocations that hold an address exported with `hipIpcGetMemHandle`, and any allocation over the same pages, are left out, because IPC refuses VMM memory. - Each allocation drops its placeholder, then reserves its recorded address with `hipMemAddressReserve`, creates and maps the memory, and grants access to its device and to every peer that can reach it. If a step misses, the allocation falls back and is counted. Replay prints `Placement : N placed at capture address, M fell back`. - `hipFree` unmaps at once. `hipFreeAsync` synchronizes its stream first, or keeps the mapping while a graph capture is open. The range is then held again for the next allocation recorded there. - gfx1201 on Linux 7.0.0-34 with the in-box amdgpu driver keeps a stale GPU translation after `hipMemUnmap`. After each unmap, replay allocates and frees 4 MiB, which makes KFD flush the TLBs. It skips the flush while a graph capture is open. - `--kernel-filter` releases placed allocations after the warm-up pass, so the timed pass places them again. - Placement is off with `--no-placement`, with `--guard-segments`, and on Windows. With it off, replay behaves as on `develop`. - Not placed: `hipExtMallocWithFlags`, pool allocations, region segments, and the application's own VMM reservations. They keep today's behaviour. `DESIGN.md` lists these limits. ## Issue Tracking JIRA ID: AIRUNTIME-2893 (defect ROCM-31827) ## Test Plan - `Unit_HRR_VaPlacement_StoredPointer`: a kernel dereferences a device pointer the application stored with a host-to-device copy. The test replays with placement on, with `--no-placement`, and with `--kernel-filter`. - `Unit_HRR_Playback_PlacementPlan` and `Unit_HRR_Playback_PlacementHeldRanges`: the placement plan, including the IPC exclusion, and the held ranges. - The existing `Unit_HRR_MemcpyPeerRoundtrip` covers peer access on two GPUs. - Built `amdhip64` with `-DHIP_HRR_CAPTURE_ENABLED`, `hrr-playback` and the tests for gfx950. Ran every Catch2 case, hidden ones included, on an MI350X with one GPU, then the whole suite with two GPUs visible. - Negative control: the same build with the placement code removed. - Merged into Daniel's HRR CI branch and ran its matrix. ## Test Result | Check | With this change | Without it | | --- | --- | --- | | Unit cases, MI350X | 25 of 25 pass | both placement cases fail | | Integration cases, hidden ones included, MI350X | All pass except cases that fail on `develop` too: `Unit_HRR_FirstMallocRoundtrip`, `Unit_HRR_ApiMatrix_T2_Roundtrip`, `Unit_HRR_ApiMatrix_T4_Roundtrip`, the 11 `Unit_HRR_DiskSpace_*_Direct` workloads, and six hidden fork, crash and capture workloads from #12253 and #12255 that fail when run without their visible test | `StoredPointer` fails | | Whole suite, two MI350X GPUs | same as above; `MemcpyPeerRoundtrip` passes | | | HRR CI matrix | gfx94x, gfx950, gfx103x, gfx110x, gfx120x, gfx1150, gfx1151, gfx1153 and both unit jobs pass. `MemcpyPeerRoundtrip` ran and passed on gfx950 and on a two-GPU gfx110x host. gfx90a stopped at `NullStreamMemsetOrdering` on the smci250 hosts, as on the base | `StoredPointer` fails on every family | ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.


Motivation
The capture writer installs
pthread_atforkhandlers so that a forked child writes its ownpid-<pid>archive. Ondevelop(89103b1c577) the prepare handler held the writer's file mutex only while it flushed, and did not take the blob mutex at all, so a child could inherit either mutex locked by another thread of the parent and then block for ever when it reopened its archive. The crash path had two smaller problems:emergency_finalize()kept a 129 KiB buffer on the stack of the thread that crashed, and a writer thread did not wait when the crash callback was already flushing the event buffer.Technical Details
All changes are in
projects/clr/hipamd/src/hrr/hip_capture_writer.cpp.atfork_prepare()takesg_reopen_mu,g_blob_muandg_file_muin that order and flushes the event buffer. It then takesg_unreplayable_muand keeps all four mutexes locked acrossfork(). The newatfork_parent()and the start ofatfork_child()unlock them, which is the usualpthread_atforkpattern. No other code path holds two of them in another order, so the new order cannot invert an existing one.writer::close()clearsg_reopen_after_forkunderg_reopen_mu. A forked child that had not recorded before its shutdown would otherwise open a new archive when a fat-binary destructor records later. Nothing would finalize that archive.atfork_child()setsg_reopen_after_forkonly when the parent had its events file open. A child forked after the parent's capture shutdown opens no archive. A parent that is itself a child yet to open its archive passes its flag on unchanged.claim_buf_locked()raisesg_buf_busyfor a thread that already holdsg_file_mu, and yields while the crash callback holds the flag.BufWriteGuardandatfork_prepare()use it. The crash callback still takesg_buf_busywith onetest_and_setand never touches the mutex.emergency_finalize()builds the manifest in a static buffer at file scope instead of on the stack. CLR runs the crash callback on the stack of the thread that faulted (on Windows also forEXCEPTION_STACK_OVERFLOW), where 129 KiB may not be left.g_emergency_manifest_busykeeps concurrent crash callbacks out of that buffer, and a second entrant skips the manifest.atfork_child()also clearsg_buf_busyandg_emergency_manifest_busy. A crash callback running on another thread at the moment offork()would otherwise leave them raised in a child that has no thread to clear them: the child's writers would wait ong_buf_busyfor ever, and the child's own crash callback would skip its manifest.open()prepares the archive in locals and publishes it underg_file_muandg_buf_busy, file header included. A forked child opens its archive on another thread's first record, and a checkpoint, the crash callback or the shutdown could otherwise see the events fd before the header.flush()takesg_reopen_mu, so the shutdown waits for that open, and clearsg_reopen_after_fork, so no archive opens after it.A process that neither forks nor crashes behaves as before. A
fork()now also waits for a blob-set update in progress on another thread, as it already waited for an event write.Issue Tracking
JIRA ID: AIRUNTIME-2831
Test Plan
-Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -Wno-missing-field-initializers -Wno-unknown-attributes -Wno-inconsistent-missing-override, with and withoutHIP_HRR_CAPTURE_ENABLED.hipMemcpyin a loop while the main thread forks 2000 times. Each child exits at once and counts as hung if it has not exited after 3 seconds. Three runs against this branch and three against the capture runtime ofdevelop89103b1c577, whose writer and CLR crash handler are identical to this branch's base.SIGSEGV, andpid-<pid>/manifest.jsonis checked afterwards. Three runs against each runtime.SIGSEGV, and anfsync()defined in the test binary holds the crash callback inside the manifest write while a second thread forks. The child then raisesSIGABRT: CLR installs its handlers withSA_RESETHAND, so the parent'sSIGSEGVhandler is already reset by then. The child'spid-<pid>/manifest.jsonis checked afterwards. Three runs against each runtime, and three against the first commit of this branch alone.Seven cases now cover this PR in
projects/hrr/tests/integration. Each ran on Daniel's CI branch,users/danielbolsson/hrr-development, merged with this branch.Unit_HRR_CaptureCrashOnSmallStack, the small-stack crash above. It also addsUnit_HRR_ForkWhileRecording, the fork stress cut to 200 forks with a 10-second deadline per child. That one checks that every child opened its own archive. Both pass in run 36909753240.Unit_HRR_ForkWhileWriterHoldsLock. It holds a recording thread in its checkpointfsync()and forks at the moment that thread would take the events mutex. It checks that the child exits and that both archives exist. It also addsUnit_HRR_CaptureCrashDuringFork, the crash-during-fork check above, with the samefsync()defined in the test binary. It checks both manifests. All four pass in run 37287880456. With the writer change reverted (run 37287883463), the first three fail on gfx94x, gfx110x, gfx1151 and gfx120x.Unit_HRR_CaptureCrashDuringForkpasses there, becausedevelophas no manifest flag to inherit.Unit_HRR_ForkWhileNotingUnreplayable. Astrlen()defined in the test binary holds a thread insidenote_unreplayable(), and the main thread forks. It checks that the child exits and that both manifests namehipUserObjectCreate.Unit_HRR_ForkedChildRecordsAfterShutdown. A forked child records through the compiler dispatch table after its ownhip_capture_shutdown(). It checks that the child leaves no archive. Both pass on six GPU families in run 37303308008. With 3daf63e and 09c9d27 reverted (run 37303311499), both fail on five of them.Unit_HRR_ForkAfterCaptureShutdown. The parent forks after its own capture shutdown, and the child records before it exits. It checks that only the parent has an archive. It passes on five GPU families in run 37305365330. With a5fb668 reverted (run 37305368949), it fails on the same five.Unit_HRR_ShutdownWhileChildOpensArchive. Anmkdir()defined in the test binary holds a child's second thread inside the open of the child's archive while the child's main thread exits. It checks that both archives end in the clean-shutdown trailer and have a complete manifest. It passes on gfx94x, gfx103x, gfx120x, gfx1150 and gfx1151 in run 37444441420. With the writer change of 353fa9f reverted (run 37444444869), the child's archive has no trailer on gfx94x, gfx110x, gfx120x, gfx1150 and gfx1151. Runners with no visible GPU failed gfx110x in the first run and gfx103x in the second.In these runs, the other failures are ones that Daniel's branch has without this PR. Those are gfx1150 runners with no visible GPU and the
Unit_HRR_Regions_*fatal on gfx103x and gfx90a. gfx94x lost its runner during the suite in run 37305365330. The table below holds the checks by hand and predates these cases.Test Result
develop89103b1c577HIP_HRR_CAPTURE_ENABLEDmanifest.jsonmanifest.jsonwritten with"complete": falsemanifest.json, 3 runs"complete": false"complete": falseThe
developfork-stress count varies from run to run: a second set of three runs gave 0, 0, 1 hung children, and this branch 0, 0, 0 again.With the first commit of this branch alone, the child's
manifest.jsonwas missing in all 3 runs of the crash-during-fork check, because the child inherited the manifest flag raised. The second commit clears it inatfork_child().develophas no such flag.In both crash checks every process ends on the signal it raised (
SIGSEGVfor the parent,SIGABRTfor the forked child). Where the callback completes, the crash handler re-raises the signal once it returns. Ondevelopthe small-stack callback does not complete: its 129 KiB buffer does not fit on the 64 KiB stack, so it faults before it flushes anything (events.binstays empty) and that secondSIGSEGVends the process. With a 256 KiB stackdevelopwrites the manifest as well.Windows was not built here. The fork handlers are POSIX only, and
claim_buf_locked(), the manifest flag and the static buffer use only standard C++.Submission Checklist