Repository navigation
feat(hrr): cap capture size and blob size at a limit the user sets - #13895
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.
🟡 Changes recommended
Fat-binary exclusion and shutdown synchronization need correction to preserve replay data and accurate archive completeness.
2 open issues
What changed in this PR
Adds user-configurable HRR capture limits alongside the existing free-space reserve.
Changes:
- Adds per-process archive and per-blob size settings.
- Reuses the capture-stop path for archive limits and skips oversized blobs.
- Documents the settings and adds integration coverage.
| File | Description |
|---|---|
| projects/hrr/tests/integration/hrr_disk_space_test.cc | Tests archive limits and oversized-blob handling. |
| projects/hrr/README.md | Documents both size settings. |
| projects/clr/rocclr/utils/flags.hpp | Declares the new runtime flags. |
| projects/clr/hipamd/src/hrr/hip_capture_writer.h | Updates writer contract comments. |
| projects/clr/hipamd/src/hrr/hip_capture_writer.cpp | Implements accounting, limits, and shared stop handling. |
Review effort: Balanced
danielbolsson
left a comment
danielbolsson
left a comment
There was a problem hiding this comment.
Checked ffe85b0: events are charged by full record size, blobs and code objects by whole blocks, a write that would pass HIP_HRR_CAPTURE_MAX_MB is not made and capture stops through the free-space path, and HIP_HRR_CAPTURE_MAX_BLOB_MB skips a blob (not code objects) and marks the archive incomplete. Nothing blocking; red NVIDIA build is the develop check_config rule, not this PR.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
danielbolsson
left a comment
danielbolsson
left a comment
There was a problem hiding this comment.
Re-approving the new head: fat binaries are exempt from the blob cap, the size-cap stop and trailer decision share one lock hold, and a child forked after the stop gets a fresh archive budget.
Two runtime flags, both 0 (no limit) by default, bound how large a capture grows, on top of the free-space reserve: - HIP_HRR_CAPTURE_MAX_MB caps each process archive: events.bin plus every blob and code object, counted in file system blocks as the reserve counts them. The write that would pass the limit is not made and, under the same hold of g_file_mu, capture stops as it does for space (events.bin flushed and closed, active marker removed, archive marked incomplete, one stderr line); the application runs on. A child forked after that stop opens an archive of its own, held to the same limit, while a stop for space carries over to the child, as the file system is shared. - HIP_HRR_CAPTURE_MAX_BLOB_MB caps one blob. A larger copy is recorded without its contents (zero hash), the archive is marked incomplete with one stderr line, and capture goes on. Code objects and the fat binaries HIP registers are not held to it; the archive limit still counts them. Replay refuses a host-to-device hipMemcpy, hipMemcpyAsync, hipMemcpyHtoD, hipMemcpyHtoDAsync or hipMemcpyWithStream recorded without its contents, naming the API and the size, with hipErrorNotFound. Both stops go through stop_capture_locked(), whose incomplete reason names no path. New integration cases, added to integration.yaml: Unit_HRR_CaptureLimit_StopsAtSize, Unit_HRR_CaptureLimit_ForkAfterStop, Unit_HRR_CaptureLimit_SkipsLargeBlob and Unit_HRR_FatBinOverBlobLimit, each with its _Direct workload. Their capture directories are per-process (hrr_temp_dir()). Jira: AIRUNTIME-2732
4880be9 to
0468fa5
Compare
# Conflicts: # projects/hrr/playback/hip_playback.cpp
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
…checks - write_blob() and write_fat_binary() only forwarded to store_blob(). write_blob() now takes `capped`, true by default, and the fat binary path passes false. - Unit_HRR_FatBinOverBlobLimit was a near-copy of Unit_HRR_FatBinHeaderChecks. That driver now sets HIP_HRR_CAPTURE_MAX_BLOB_MB=1, and its workload registers the 2 MiB bundle before HIP initialises. The bundle registered after went through the same write, so it is gone. - Unit_HRR_CaptureLimit_ForkAfterStop: the child's events.bin and manifest.json checks go. The driver's count of two processes needs both. - Unit_HRR_CaptureLimit_StopsAtSize: `copies > 0` goes. It passes with the limit reverted. AIRUNTIME-2732: scope unchanged; one writer entry point and one test case fewer, every limit and its negative control kept.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
With HIP_HRR_CAPTURE_MAX_BLOB_MB set, replay skipped a 2D or 3D host-to-device copy whose contents capture had left out and returned hipSuccess, and a launch silently lost the pinned host snapshot the cap dropped. Replay now refuses such a copy by name, as it already did for the 1D copies, and capture records a left-out snapshot with a zero hash so replay refuses the launch that reads it. The hip_capture.cpp comment on which capture knobs live in flags.hpp matches the new flags. AIRUNTIME-2732
…hrr-capture-size-limits
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
…t memory the blob cap left out
danielbolsson
left a comment
danielbolsson
left a comment
There was a problem hiding this comment.
Checked the delta since 4277328: replay now refuses 2D/3D H2D copies, pinned snapshots, hipHostRegister and to-symbol graph copies whose blob the cap left out, matching what capture records (zero hash, same memcpy_src_is_host rule), and hipLinkAddData still refuses via its missing blob. The rocprofiler-sdk failure is unrelated.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
…OB_MB in HRR instead of the CLR flag registry The two size limits leave rocclr/utils/flags.hpp. writer::open() reads them with getenv, parsed as the registry parses a size_t, once per process, as hip_capture.cpp reads HIP_HRR_HOST_SNAPSHOTS and HIP_HRR_HOST_SNAPSHOT_MAX_MB. Same names, same default of 0 (no limit), same meaning. JIRA ID: AIRUNTIME-2732
# Conflicts: # projects/hrr/tests/include/hrr_test_common.hh
danielbolsson
left a comment
danielbolsson
left a comment
There was a problem hiding this comment.
Review — needs work
Review of changes since my earlier review at 671081f (one new PR commit, 4011ace, plus develop merges), with the PR's CLR diff against its merge-base also read in full. I did not re-read hip_playback.cpp or the test files.
Intent: AIRUNTIME-2732, implemented in full by the code. The description's account of how it is implemented is out of date (first Blocking item).
Blocking
- The description states the old design: "Two new CLR flags in
flags.hpp" — PR body, Technical Details first bullet, Test Plan, Test Result.- 4011ace deletes both
release(size_t, ...)entries fromprojects/clr/rocclr/utils/flags.hpp. The PR no longer touchesflags.hppat all.writer::open()now reads the two variables withgetenv/atolinenv_limit_bytes()(hip_capture_writer.cpp:1041-1047,1270-1271). - The body still says the flags are in
flags.hpp. It also namesflags.hppamong the runtime files in theffe85b0bbanegative control, which now describes a diff that does not exist. - The Test Plan and Test Result table are pinned to head
671081faeeand gatep13895-671081f. The current head isac2005c752. The "GREEN, no new failures" claim does not cover the reworked read path (static constonce-per-process parsing). - Fix: rewrite the first bullet to say the limits are read from the environment by the writer. Drop
flags.hppfrom the control descriptions. Re-run the gate on the current head, or say the results are for 671081f and that 4011ace is untested. Confidence 90 · logic.
- 4011ace deletes both
- Check
rocprofiler-sdk(gfx94X-dcgpu) failed, andtherock-pr-botfailed on this commit —statusCheckRollup.- I read no log that attributes either failure, and the HRR checks pass. I could not tell whether the failures are related to this change.
- Method rule: a failing check is blocking until shown unrelated. Show it is a baseline failure on develop, or fix it. The
rcclgfx94X test was still pending, so it is not counted. Confidence 70 · mechanical.
Non-blocking
- The limits are fixed at the first
open()and cannot change in-process —hip_capture_writer.cpp:1270-1271.- Behaviour changed from the flag registry. A test or tool that sets
HIP_HRR_CAPTURE_MAX_MBin-process after the firstopen()is silently ignored. Every use I found sets it in a subprocess environment, so nothing breaks today. The comment says only "forked child keeps the limits", not that later environment changes are ignored. Confidence 70 · logic. This is a documentation point, not a defect.
- Behaviour changed from the flag registry. A test or tool that sets
Decisions for the author
- Environment read instead of a registered flag — tradeoff.
- For: keeps HRR-only knobs in HRR code, consistent with
HIP_HRR_HOST_SNAPSHOTSandHIP_HRR_HOST_SNAPSHOT_MAX_MBinhip_capture.cpp:246-254. It also keeps the PR out of rocclr. - Against: the knobs lose the registry's single place for flags, listing and config-file handling.
atolmatches the registry'sTsize_tparsing (flags.cpp:183), so a negative or garbage value behaves as before: it clamps to the largest limit, effectively none. Confirm this was intended.
- For: keeps HRR-only knobs in HRR code, consistent with
Positive signals
- The stale-comment sweep in 4011ace is complete.
hip_capture.cpp:135-142is reworded, andgit grepfinds no remaining reference to the limits living inflags.hpp. reserve_space()andwrite_event_raw()charge the limit and callstop_at_limit_locked()under the sameg_file_muhold thatflush()takes to decide on the trailer, so an archive that dropped a write is never finalized as complete.
Deployment notes
HIP_HRR_CAPTURE_MAX_MBandHIP_HRR_CAPTURE_MAX_BLOB_MBno longer appear inflags.hpp, so they are not in the runtime's flag listing. The name, default (0, no limit) and meaning are unchanged, and the README table documents them.
What this covered
- Read: the full CLR-side PR diff (
hip_capture.cpp,hip_capture_writer.cpp,hip_capture_writer.h) against merge-base cd8bec0 through head ac2005c. I also read commit 4011ace and the PR description, and compared the changed files against the pinned results. - Degraded:
hip_playback.cpp, the tests and DESIGN.md were not re-read this round. No worker fan-out was done, so this is a single-context pass. The earlier findings are not carried into it. - Not run: the history and prior-changes passes; I attempted no build or test.
- Check not made: I could not attribute the
rocprofiler-sdkortherock-pr-botfailures; the pulled logs were not useful. - Not reconciled against the PR discussion or the earlier report. I could not check whether my earlier findings were answered, so whether the new commit resolves them is open.
|
🤖 AI-assisted pre-review (for @mangupta) Re-reviewed the updated head ( |
danielbolsson
left a comment
danielbolsson
left a comment
There was a problem hiding this comment.
Not approving yet: the PR description is still stale and I could not tie the failing checks to develop. Two items:
- PR body, Technical Details bullet 1, Test Plan, Test Result: still says the limits are CLR flags in
projects/clr/rocclr/utils/flags.hppand pins the gate to 671081f; the PR no longer touches flags.hpp (hip_capture_writer.cpp:1041-1047,:1270-1271read them with getenv) and the head is ac2005c. Fix: say the writer reads the env vars, drop flags.hpp from the ffe85b0 control, and re-run the gate on the current head or state 4011ace is untested. rocprofiler-sdk(gfx94X) andtherock-pr-botfail at this head; show they also fail on develop (the HRR checks pass).
PR Pundit review(Testing PR Pundit Code Review agent. Not all comments may be relevant. This is a one-time automated pass: please address what is useful, no need to request a re-review. Please reply to each point, whether it applies or not: we harvest these replies, and both kinds are enormously helpful for tuning the agent. Reach out to @peymanr for suggestions.)
1. 🟠 Should fix: Replay silently skips D2H read-back validation: this PR capped D2H blobs with hash {0,0}
With Who is affected: Users who set Checked against the code by an independent pass. 2. 🟠 Should fix: Default-kind pageable to-symbol copy over blob cap reports misleading 'source not mapped' and continues: this PR added left-out check after translate_ptr
In Who is affected: Replays of archives captured with Checked against the code by an independent pass. 3. 🟠 Should fix: Mistyped cap env value runs capture unbounded with no warning: this PR parses with unchecked std::atol
Who is affected: Users who set Checked against the code by an independent pass. 4. 🟠 Should fix: Capture stops early at HIP_HRR_CAPTURE_MAX_MB after failed writes: this PR charges g_archive_bytes with no refund
Who is affected: Users who set Checked against the code by an independent pass. 5. 🟠 Should fix: Each capped launch wastes a pinned-memory copy under g_pinned_mu: this PR enforces the blob cap only inside write_blob
With Who is affected: Users who opt into Checked against the code by an independent pass. Shared from a pilot of PR Pundit, an agentic code reviewer that builds and tests the change and reads the project's past reviews. Reviewed at |
TheRock Submodule Bump ActivityNewest first
|
Motivation
HRR capture grows its archive for as long as the process runs. Today only the free-space reserve from #12256 bounds it. That reserve protects the file system, not the user's budget. The security-gap review asked for a size the user can set, and for a cap on a single blob. Both stop the same way the reserve does: the archive is marked incomplete and the application keeps running. The sliding-window ring buffer stays the mechanism for long runs. These limits apply whether or not it has landed.
Technical Details
HIP_HRR_CAPTURE_MAX_MBandHIP_HRR_CAPTURE_MAX_BLOB_MB. They are not CLR flags:writer::open()reads them from the environment once per process, inhip_capture_writer.cpp, so a forked child keeps its parent's limits.HIP_HRR_CAPTURE_MAX_MBcaps each process archive (pid-<pid>/). It countsevents.bin, every blob and every code object. Blobs and code objects are counted in whole file system blocks, as the reserve counts them. Manifests and the trailer are not counted.flush()takes to decide on the trailer: a cap-stopped archive is never finalized as complete. So, manifests aside, the archive never exceeds the limit.events.binis flushed and closed, theactivemarker is removed, the archive is marked incomplete, and one[HRR capture] Capture stoppedline goes to stderr.stop_capture_locked(). The stop flag is renamed fromg_out_of_spacetog_capture_stopped, because it now has two causes, andg_stopped_at_limitrecords which one it was. The incomplete reason names no path; only the stderr line names the archive, read under the writer's file mutex.HIP_HRR_CAPTURE_MAX_BLOB_MBcaps one blob. A larger copy is not hashed or stored. Its event is recorded with a zero blob hash, which is the existing "no blob" convention. The archive is marked incomplete, one line goes to stderr per archive, and capture continues. Replay refuses work recorded without its contents and returnshipErrorNotFound. That covers a host-to-devicehipMemcpy,hipMemcpyAsync,hipMemcpyHtoD,hipMemcpyHtoDAsyncorhipMemcpyWithStream, checked inreplay_memcpy_impl(); a 2D or 3D host-to-device copy (hipMemcpy2D/2DAsync,hipMemcpy3D/3DAsyncand their_sptforms,hipDrvMemcpy3D/Async,hipDrvMemcpy2DUnaligned,hipMemcpyParam2D/Async), checked indrvmemcpy_h2d_src_blob(); ahipHostRegister, checked in its handler; and a launch whose pinned host snapshot the cap left out, checked inrestore_host_snapshots()unless the launch goes into a stream capture. A to-symbol graph copy from host memory (hipGraphAddMemcpyNodeToSymbol,hipGraphMemcpyNodeSetParamsToSymbol,hipGraphExecMemcpyNodeSetParamsToSymbol) is refused too, into_symbol_source(), and returnshipErrorInvalidValue. A copy or registration refusal names the API and the size.writer::write_blob()withcappedfalse, stored as blobs as before. The archive cap still bounds both. AhipLinkAddDataimage over the per-blob cap is left out, and replay refuses its link.open(), so a forked child gets its own limit. A child forked after a cap stop still opens its ownpid-<pid>archive with a fresh budget. After a space stop it opens none, as the file system is shared.pid-<pid>archive, not to the archive root across processes.projects/hrr/README.mddocuments both variables.hrr_disk_space_test.ccreuse its workload driver, with free space left at its default so only the limit can stop capture:Unit_HRR_CaptureLimit_StopsAtSize: a 64 MiB cap and 128 distinct 1 MiB copies. Capture stops once and the archive is incomplete but not torn. Every recorded blob exists, and the archive holds at most 64 MiB and more than 62 MiB. The application still reads back correct data.Unit_HRR_CaptureLimit_SkipsLargeBlob: a 1 MiB blob cap. Two 4 MiB copies are recorded without a blob, with one stderr line. A 4 MiBhipMemcpy2D, a 4 MiBhipHostRegisterand two 4 MiBhipGraphAddMemcpyNodeToSymbolcopies from pinned memory (hipMemcpyHostToDeviceandhipMemcpyDefault) go over the cap too. A 256 KiB copy and its read-back are stored, and capture does not stop. Replaying the archive with--continue-on-errorfails, with 2hipMemcpyrefusals, 1hipMemcpy2Drefusal, 1hipHostRegisterrefusal and 2hipGraphAddMemcpyNodeToSymbolrefusals.Unit_HRR_CaptureLimit_ForkAfterStop: the parent reaches a 64 MiB cap, then forks a child that records once. The child opens and finalizes its ownpid-<child>archive, and the root manifest lists two processes.Unit_HRR_FatBinHeaderChecksinhrr_fatbin_header_test.ccnow runs with a 1 MiB blob cap. A 2 MiB bundle registered before HIP initialises is stored whole, and the archive stays complete.Unit_HRR_PinnedHostBlobRefusedinhrr_pinned_host_test.ccalso captures its workload withHIP_HRR_CAPTURE_MAX_BLOB_MB=1. Replay must fail with the pinned host snapshot refusal and launch no kernel.hrr_host_budget_test.cc, thehipHostRegisterrecord ofUnit_HRR_HostBudget_HostAllocationHandlersRefuseOverBudgetgets a non-zero blob hash, as capture gives it, so it still reaches the budget refusal._Directworkloads skip when run without their driver's environment, asUnit_HRR_CaptureCapabilitiesImport_Directdoes.Issue Tracking
JIRA ID: AIRUNTIME-2732
Test Plan
flags.hpp; gatep13895-ac2005cGREEN against developgdevcd8bec, with every non-passing case also failing on that develop. The table below and the negative controls are from the earlier head 671081f (gatep13895-671081f, against develop 4eb1b0f,gdev4eb1b0f).hipHostRegisterand to-symbol refusals taken out of replay (p13895-neg-2b33af2), which fails onlyCaptureLimit_SkipsLargeBlob, athrr_disk_space_test.cc:1177-1178; ffe85b0 with the runtime files (hip_capture_writer.cpp,hip_capture_writer.hand the two limits) and the README put back to develop (airuntime-2732-u-neg-dd23c1c), which failsCaptureLimit_SkipsLargeBlobandCaptureLimit_StopsAtSize; 30bf275 with fat binaries sent back throughwrite_blob()(airuntime-2732-cf-neg-b715ebc), which failsFatBinOverBlobLimit; 4880be9 without the stop reset in a forked child (p13895-neg-b34523d), which failsCaptureLimit_ForkAfterStop; 0468fa5 without the replay refusal (p13895-neg-46a8317), which failsCaptureLimit_SkipsLargeBlob; 4277328 with fat binaries capped and no stop reset in a forked child (p13895-trimneg-bd8415c), which failsFatBinHeaderChecksandCaptureLimit_ForkAfterStop; 4277328 with a fat binary over the cap marking the archive incomplete (p13895-trimneg-e350e57), which failsFatBinHeaderChecks; afea389a43, 302841e with its fix reverted and the tests kept (nc13895c-afea389), which failsCaptureLimit_SkipsLargeBlobandPinnedHostBlobRefused.gdev4eb1b0f).Unit_HRR_DiskSpace_CrashAfterStopcovers. Aflush()racing a cap stop has no test: no call a test can hook runs between the refusal and the stop, and they now share one critical section.Test Result
hipHostRegisteror to-symbol refusalCaptureLimit_SkipsLargeBlobApart from the six new cases, every case has the same status as on develop. The 26 hidden cases that do not pass also fail on develop. Most are
_Directworkloads that need their driver's setup. The three extra hidden skips are the new_Directworkloads, which skip without their driver's environment.The gate also runs the
hrr-generator-rulesctest. It passes on this head and on develop 4eb1b0f.Submission Checklist