misc: Add distributed SMT perf runner#936
Conversation
Change-Id: Ib4699ddad80458f69d9b1907bb78446f38ca14b4
📝 WalkthroughWalkthroughAdds distributed gem5 benchmark execution across workflow callers and the reusable performance template, plus a new Python scheduler that launches checkpoint jobs locally or over SSH with idle-capacity probing, marker-based completion, and retry handling. ChangesDistributed simulation support
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
1 similar comment
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
jensen-yan
force-pushed
the
codex/gem5-distributed-smt-ci
branch
from
34944b7 to
6bdbf4e
Compare
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
jensen-yan
force-pushed
the
codex/gem5-distributed-smt-ci
branch
from
6bdbf4e to
04a418c
Compare
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Change-Id: I99bc34f6853ecff73f44c9cfa60a20d4be3a04b3
jensen-yan
force-pushed
the
codex/gem5-distributed-smt-ci
branch
from
04a418c to
1f52a08
Compare
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Change-Id: If37627210f930f67ee11981cb09d1a34b5e83a83
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
There was a problem hiding this comment.
Pull request overview
This PR adds a distributed checkpoint runner for gem5 perf CI to reduce SMT SPEC06 wall time by spreading workload launches across a list of SSH-reachable servers, while preserving the existing spec_all/<workload>/m5out/stats.txt archive layout.
Changes:
- Introduce
util/xs_scripts/distributed_sim.pyto schedule and launch workloads acrosslocalor remote servers via SSH, with status markers and retry handling. - Extend the reusable perf workflow to optionally switch from
parallel_sim.shto the distributed runner whendistributed_serversis provided. - Expose distributed-runner knobs in
manual-perf.ymland enable distributed execution for SMT perf workflows.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| util/xs_scripts/distributed_sim.py | New distributed launcher that mirrors parallel_sim.sh output layout and supports SSH dispatch + idle probing. |
| .github/workflows/gem5-perf-template.yml | Adds distributed-runner inputs and conditionally invokes distributed_sim.py instead of parallel_sim.sh. |
| .github/workflows/manual-perf.yml | Adds workflow_dispatch inputs to pass distributed-runner parameters into the reusable perf workflow. |
| .github/workflows/gem5-smt-spec06-0.3c.yml | Enables the distributed path for SMT SPEC06 perf CI with a concrete server list and dispatch host. |
| .github/workflows/gem5-ideal-btb-perf-weekly.yml | Enables the distributed path for the weekly SMT perf job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8610ef4e3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Change-Id: I38c235ff3688652765ea804d8f17973e113662ee
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 963a547952
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/gem5-perf-template.yml:
- Around line 215-222: The shell script in the workflow is directly
interpolating untrusted GitHub Actions expressions into `run:` text, which
creates a template-injection risk on the self-hosted runner. Move the
distributed-run values behind `env:` indirection first, like the existing
`CONFIG_PATH_INPUT` pattern, and have the script read from those environment
variables instead of using `${{ inputs.distributed_servers }}`, `${{
steps.archive.outputs.distributed_servers }}`, `${{
inputs.distributed_jobs_per_server }}`, `${{
steps.archive.outputs.distributed_require_idle_cpus }}`, and `${{
steps.archive.outputs.distributed_dispatch_host }}` inline. Update the affected
distributed setup and archive steps so the `run` block only consumes safe shell
variables, keeping the logic in the relevant workflow jobs consistent with the
guarded pattern already used elsewhere.
In `@util/xs_scripts/distributed_sim.py`:
- Around line 263-313: The server parsing in parse_server_list currently accepts
tokens that begin with “-”, which can later be interpreted by ssh as options;
update this parser to reject any server token or expanded range entry whose
final target starts with “-”, raising a clear ValueError instead. Also add the
same validation for args.dispatch_host before it is passed into
wrap_with_dispatch_host so both server lists and dispatch hosts are protected.
- Around line 854-860: Clear stale non-completed markers before each launch
attempt so SSH failures can still be retried. Update the launch logic around the
`has_any_marker()` / retry check in `distributed_sim.py` to remove stale `abort`
and `running` markers after the completed-skip path but before deciding whether
`result == 255` is retryable. Apply the same cleanup in the second launch path
referenced by the other affected block so both attempt flows behave
consistently.
- Around line 926-987: The scheduler loop in distributed_sim.py can launch jobs
before all checkpoints are validated, so a later find_checkpoint() failure
leaves already-started work running. Pre-resolve the checkpoints for all
selected workloads before entering the launch loop, using the same checkpoint
lookup logic currently done around find_checkpoint() and build_gem5_command(),
and only start scheduling/launching jobs after every required checkpoint has
been found successfully. Also apply the same pre-resolution approach to the
retry/launch path referenced in the later block so both code paths fail fast
before any job is dispatched.
- Around line 399-407: Validate env override keys in parse_env_overrides before
they reach the shell export path, since malformed keys are currently inserted
unquoted into export statements. Update parse_env_overrides to reject any key
that is not a valid shell variable name and apply the same validation anywhere
the overrides are rendered into exports in the distributed_sim script. Use the
existing parse_env_overrides helper and the export-emitting code near the later
override handling to keep invalid keys from becoming shell syntax.
- Around line 467-480: The shell script built in distributed_sim.py is still
vulnerable because shlex.quote() results are wrapped inside double-quoted echo
strings in the workload/checkpoint/command logging block. Update the script
generation in that return block so each logged value is emitted as a single
safely quoted shell argument without outer double quotes, especially for the
echo lines in the distributed_sim command template.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e188f6ca-67e9-4576-8403-c5fe1e119459
📒 Files selected for processing (5)
.github/workflows/gem5-ideal-btb-perf-weekly.yml.github/workflows/gem5-perf-template.yml.github/workflows/gem5-smt-spec06-0.3c.yml.github/workflows/manual-perf.ymlutil/xs_scripts/distributed_sim.py
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
|
[Generated by GEM5 Performance Robot] Align BTB PerformanceOverall Score
|
Change-Id: I71dc74f9cb1b60e9250be38e4ac0a9a1ea228919
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c682683a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
Change-Id: I75266bf1714f07538ae5cb63bf4b594e210c3b9a
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 407881e34c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🚀 Coremark Smoke Test Results
✅ Difftest smoke test passed! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
util/xs_scripts/distributed_sim.py (1)
1170-1177: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn a non-zero exit code when any workload fails.
failedis counted but ignored, so the GitHub workflow can pass even when distributed jobs failed.Proposed fix
- return 0 + return 1 if failed else 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@util/xs_scripts/distributed_sim.py` around lines 1170 - 1177, The summary in distributed_sim.py currently reports failed workloads but still returns success, so update the final exit path in the main summary/return block to propagate failures by returning a non-zero code whenever failed is greater than zero. Use the existing summary variables in the same scope (failed, completed, launch_attempts, retry_attempts) and keep the successful 0 return only for the all-workloads-passed case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@util/xs_scripts/distributed_sim.py`:
- Around line 502-513: The script built in distributed_sim.py does not fail fast
when preparing the job work directory, so a bad mkdir, cd, marker cleanup, or
touch can still let GEM5 continue in the wrong place. Update the shell prologue
returned by the work-dir setup helper to exit on command failures as well as
unset variables, and keep the directory preparation steps in the same block so
failures in mkdir, cd, rm -f abort completed, or touch running stop execution
immediately. Use the existing work_dir setup code and the shell script template
around shell_exports(env) as the place to apply the fix.
---
Outside diff comments:
In `@util/xs_scripts/distributed_sim.py`:
- Around line 1170-1177: The summary in distributed_sim.py currently reports
failed workloads but still returns success, so update the final exit path in the
main summary/return block to propagate failures by returning a non-zero code
whenever failed is greater than zero. Use the existing summary variables in the
same scope (failed, completed, launch_attempts, retry_attempts) and keep the
successful 0 return only for the all-workloads-passed case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14fdf7e7-49c7-404e-b2df-50a89dbdcee8
📒 Files selected for processing (3)
.github/workflows/gem5-perf-template.yml.github/workflows/manual-perf.ymlutil/xs_scripts/distributed_sim.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/manual-perf.yml
- .github/workflows/gem5-perf-template.yml
Motivation
SMT SPEC06 perf CI currently runs all checkpoints on one self-hosted runner through local GNU parallel. SMT slices have noticeably longer wall time, so the feedback loop can be much slower than non-SMT even when there are idle NFS-visible machines.
Approach
util/xs_scripts/distributed_sim.py, a GEM5-specific distributed launcher that preserves the existingspec_all/<workload>/m5out/stats.txtarchive layout.parallel_sim.shas the default reusable-workflow path whendistributed_serversis empty.node020-node039, 32 jobs/server, and a lightweight idle-CPU probe.manual-perf.ymlso the path can be tested and tuned before replacing the old local parallel path.Validation
python3 -m py_compile util/xs_scripts/distributed_sim.pynode020-node021completedandm5out/stats.txtin the existing layoutRollout
This keeps both paths. Existing non-SMT perf workflows continue to use
parallel_sim.sh; SMT CI and manual perf can exercise the distributed path first.Summary by CodeRabbit