feat(memory): apply kernel memory tunables before memory-mode runs - #507
feat(memory): apply kernel memory tunables before memory-mode runs#507not-matthias wants to merge 2 commits into
Conversation
The sysctl read-then-write-with-sudo primitive is not walltime-specific; memory mode needs it too.
Merging this PR will not alter performance
Comparing 
Footnotes
|
febfce2 to
a219b76
Compare
Disables transparent huge pages, sets vm.compaction_proactiveness, vm.swappiness and kernel.numa_balancing to 0, disables swap and drops the page cache, so benchmark repos no longer need a hand-written CI step. Applied only in CI, best-effort: a knob that cannot be set is a warning. swapoff is skipped on zram devices and whenever the swapped pages would not fit in available memory.
a219b76 to
3568cec
Compare
Greptile SummaryThe PR adds best-effort kernel memory tuning around memory-mode benchmark execution and moves shared sysctl support into executor helpers. It also adds CI gating and restoration guards, but CircleCI is omitted and two host-state restoration paths remain unsafe.
Confidence Score: 2/5The PR should not merge until CircleCI receives the intended tuning and host swap/kernel state is reliably recoverable after partial swapoff failure and forced termination. The new feature silently skips a supported CI provider and can leave persistent runners with altered host-global memory settings through two reachable cleanup gaps. Files Needing Attention: src/executor/memory/tunables.rs, src/run_environment/mod.rs
|
| Filename | Overview |
|---|---|
| src/executor/memory/tunables.rs | Adds the complete tuning guard, but aggregate swapoff failure can lose restoration state and process termination bypasses the only cleanup path. |
| src/run_environment/mod.rs | Adds the CI predicate but omits the already-supported CircleCI provider, disabling the feature there. |
| src/executor/memory/executor.rs | Correctly scopes the guard around each memory run, including ordinary error returns, but necessarily depends on the guard's cleanup guarantees. |
| src/executor/helpers/linux_sysctl.rs | Generalizes sysctl mutation to return previous values while preserving existing wall-time setup behavior. |
| src/executor/wall_time/profiler/perf/mod.rs | Updates the profiling sysctl import after moving the helper. |
| src/executor/wall_time/profiler/samply/mod.rs | Updates the profiling sysctl import after moving the helper. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[MemoryExecutor::run] --> B{Supported CI detected?}
B -- No --> C[Run without tunables]
B -- Yes --> D{Passwordless elevation?}
D -- No --> C
D -- Yes --> E[Capture and apply THP/sysctl/swap state]
E --> F[Drop page cache]
F --> G[Run memtrack benchmark]
G --> H[MemoryTunables::drop]
H --> I[Restore swap, sysctls, and THP]
G -. process termination .-> J[Restoration bypassed]
Prompt To Fix All With AI
### Issue 1
src/run_environment/mod.rs:58-60
**CircleCI bypasses memory tuning**
When a memory-mode run executes on CircleCI, `is_ci_environment()` returns false because it omits the supported CircleCI provider, causing all THP, sysctl, swap, and page-cache stabilization to be skipped.
```suggestion
pub fn is_ci_environment() -> bool {
BuildkiteProvider::detect()
|| CircleCIProvider::detect()
|| GitHubActionsProvider::detect()
|| GitLabCIProvider::detect()
}
```
### Issue 2
src/executor/memory/tunables.rs:145-148
**Failed swapoff loses restoration state**
If `swapoff -a` disables one swap entry and then fails on another, this branch discards the previously captured entry list, so `Drop` never re-enables the entry already disabled and leaves the host partially swap-disabled.
### Issue 3
src/executor/memory/tunables.rs:48-53
**Forced termination bypasses restoration**
If a memory run is cancelled or killed on a persistent self-hosted CI runner after these host-global settings are applied, process termination bypasses the sole `Drop` restoration path, causing later jobs to inherit disabled swap and altered THP, compaction, swappiness, or NUMA-balancing settings.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: a219b76 | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub fn is_ci_environment() -> bool { | ||
| BuildkiteProvider::detect() || GitHubActionsProvider::detect() || GitLabCIProvider::detect() | ||
| } |
There was a problem hiding this comment.
When a memory-mode run executes on CircleCI, is_ci_environment() returns false because it omits the supported CircleCI provider, causing all THP, sysctl, swap, and page-cache stabilization to be skipped.
| pub fn is_ci_environment() -> bool { | |
| BuildkiteProvider::detect() || GitHubActionsProvider::detect() || GitLabCIProvider::detect() | |
| } | |
| pub fn is_ci_environment() -> bool { | |
| BuildkiteProvider::detect() | |
| || CircleCIProvider::detect() | |
| || GitHubActionsProvider::detect() | |
| || GitLabCIProvider::detect() | |
| } |
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/run_environment/mod.rs
Line: 58-60
Comment:
**CircleCI bypasses memory tuning**
When a memory-mode run executes on CircleCI, `is_ci_environment()` returns false because it omits the supported CircleCI provider, causing all THP, sysctl, swap, and page-cache stabilization to be skipped.
```suggestion
pub fn is_ci_environment() -> bool {
BuildkiteProvider::detect()
|| CircleCIProvider::detect()
|| GitHubActionsProvider::detect()
|| GitLabCIProvider::detect()
}
```
**Knowledge Base Used:**
- [Executor: benchmark run orchestration](https://app.greptile.com/codspeed/-/custom-context/knowledge-base/codspeedhq/codspeed/-/docs/executor.md)
- [Run environment detection and providers](https://app.greptile.com/codspeed/-/custom-context/knowledge-base/codspeedhq/codspeed/-/docs/run-environment.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if let Err(error) = run_with_sudo("swapoff", ["-a"]) { | ||
| warn!("Failed to disable swap: {error}"); | ||
| return Vec::new(); | ||
| } |
There was a problem hiding this comment.
If swapoff -a disables one swap entry and then fails on another, this branch discards the previously captured entry list, so Drop never re-enables the entry already disabled and leaves the host partially swap-disabled.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/executor/memory/tunables.rs
Line: 145-148
Comment:
**Failed swapoff loses restoration state**
If `swapoff -a` disables one swap entry and then fails on another, this branch discards the previously captured entry list, so `Drop` never re-enables the entry already disabled and leaves the host partially swap-disabled.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| /// Drops the page cache. Nothing to restore: the node is a write-only | ||
| /// trigger and the kernel refills the cache on demand. | ||
| fn drop_page_cache() { | ||
| nix::unistd::sync(); |
There was a problem hiding this comment.
Why is this call here? Could benefit from a comment
| pub fn apply() -> Self { | ||
| if !crate::run_environment::is_ci_environment() { | ||
| debug!("Not running in CI, skipping kernel memory tunables"); | ||
| return Self::default(); |
There was a problem hiding this comment.
I think it's better to return an option here rather than relying on the default satisfies the condition of the drop guard. This way if we dont do anything, we are SURE we'll not run any command when dropping the tunables guard
| //! Kernel knobs that stabilise memory measurements: transparent huge pages, | ||
| //! compaction/swap/NUMA-balancing sysctls, swap and the page cache. | ||
| //! | ||
| //! [`MemoryTunables`] captures the previous value of every knob it changes and | ||
| //! restores it on drop, so a host that only looks like CI — `CI=true` inside a | ||
| //! container sharing the host's non-namespaced knobs, say — is left as it was. |
There was a problem hiding this comment.
Could we have some traceability and documentation about these tuneables?
Ideally, each should be individually tested for its variance effect. This is to make sure we are not vibe-disabling swap because opus/fable felt like it was a good idea.
| /// Applies the knobs on a best-effort basis: a knob that cannot be set is | ||
| /// warned about, never fatal. | ||
| pub fn apply() -> Self { | ||
| if !crate::run_environment::is_ci_environment() { |
There was a problem hiding this comment.
We are not doing this kind of guard for walltime, could we unify behavior? This feels very ad-hoc for now
2 participants
Benchmark repos running
codspeed --mode memorycurrently need a hand-written CI step to stabilise kernel memory behaviour before measuring: disable THP, zerovm.compaction_proactiveness/vm.swappiness/kernel.numa_balancing,swapoff -a, and drop the page cache before each suite.The runner now does this itself at the start of every memory-mode execution.
Behaviour
MemoryTunables::apply()returns a guard holding the previous value of every knob it changed, and restores it on drop — including on the error paths, whereteardown()never runs. Only knobs that were not already at the target are captured, and THP is restored to the exact mode it held (madvisestaysmadvise).swapoff -ais guarded: skipped on zram devices (it permanently resets theirdisksize) and whenever the swapped pages would not fit inMemAvailable. Restoring re-enables the recorded entries one by one, sinceswapon -awould miss a swap file absent from/etc/fstab.run()rather thansetup(), so the page cache is dropped before every suite and--skip-setupdoes not bypass it. The sysctl/THP writes are no-ops on later suites sinceensure_sysctlcompares before writing.Commits
refactor(executor): move linux_sysctl into executor helpers— pure move, the sysctl primitive is no longer walltime-specific.feat(memory): apply kernel memory tunables before memory-mode runsVerification
cargo clippy --all-targetsclean,cargo test --lib(pre-existing failures on this host are the sudo-requiring tests, which writekernel.kptr_restrictand are identical onmain).Not running in CI, skipping kernel memory tunables; host THP unchanged.enabled/defragbecome[never],vm.compaction_proactiveness = 0,vm.swappiness = 0, and the swap guard correctly loggedLeaving swap enabled: swapped pages do not fit in available memory.