Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,19 @@ pub fn ensure_linux_profiling_sysctls() -> Result<()> {
Ok(())
}

/// Sets a sysctl, returning the value it held before, or `None` when it was
/// already at `target_value` and nothing was written.
#[cfg(target_os = "linux")]
fn ensure_sysctl(name: &str, target_value: i64) -> Result<()> {
if sysctl_read(name)? == target_value {
return Ok(());
pub(crate) fn ensure_sysctl(name: &str, target_value: i64) -> Result<Option<i64>> {
let current_value = sysctl_read(name)?;
if current_value == target_value {
return Ok(None);
}

let assignment = format!("{name}={target_value}");
run_with_sudo("sysctl", ["-w", assignment.as_str()])
run_with_sudo("sysctl", ["-w", assignment.as_str()])?;

Ok(Some(current_value))
}

#[cfg(target_os = "linux")]
Expand Down
1 change: 1 addition & 0 deletions src/executor/helpers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ pub mod harvest_perf_maps_for_pids;
pub mod homebrew;
pub mod introspected_golang;
pub mod introspected_nodejs;
pub mod linux_sysctl;
pub mod profile_folder;
pub mod run_command_with_log_pipe;
pub mod run_with_env;
Expand Down
3 changes: 3 additions & 0 deletions src/executor/memory/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use crate::executor::helpers::get_bench_command::get_bench_command;
use crate::executor::helpers::run_command_with_log_pipe::run_command_with_log_pipe_and_callback;
use crate::executor::helpers::run_with_env::prefix_command_with_env;
use crate::executor::helpers::run_with_sudo::is_root_user;
use crate::executor::memory::tunables::MemoryTunables;
use crate::executor::shared::fifo::RunnerFifo;
use crate::executor::{ExecutionContext, Executor};
use crate::instruments::mongo_tracer::MongoTracer;
Expand Down Expand Up @@ -159,6 +160,8 @@ impl Executor for MemoryExecutor {
execution_context: &ExecutionContext,
_mongo_tracer: &Option<MongoTracer>,
) -> Result<()> {
let _tunables = MemoryTunables::apply();

// Create the results/ directory inside the profile folder to avoid having memtrack create it with wrong permissions
std::fs::create_dir_all(execution_context.profile_folder.join("results"))?;

Expand Down
1 change: 1 addition & 0 deletions src/executor/memory/mod.rs
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
pub mod executor;
pub(crate) mod setup;
pub(crate) mod tunables;
324 changes: 324 additions & 0 deletions src/executor/memory/tunables.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,324 @@
//! 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.
Comment on lines +1 to +6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


use crate::executor::helpers::linux_sysctl::ensure_sysctl;
use crate::executor::helpers::run_with_sudo::{can_elevate_without_prompt, run_with_sudo};
use crate::prelude::*;
use std::fs::read_to_string;
use std::path::Path;

/// Guard holding the previous value of every knob that was actually changed.
/// Empty when the knobs were not applied at all, making [`Drop`] a no-op.
#[derive(Debug, Default)]
#[must_use = "the knobs are restored as soon as the guard is dropped"]
pub struct MemoryTunables {
/// THP knob path -> the mode it held before.
thp: Vec<(String, String)>,
sysctls: Vec<(&'static str, i64)>,
/// Swap entries that were active before `swapoff -a`.
swap: Vec<String>,
}

impl MemoryTunables {
/// 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() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not doing this kind of guard for walltime, could we unify behavior? This feels very ad-hoc for now

debug!("Not running in CI, skipping kernel memory tunables");
return Self::default();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// Blocking the run on an interactive password prompt would be worse than
// measuring without the knobs.
if !can_elevate_without_prompt() {
warn!(
"Cannot elevate privileges without a password prompt, skipping kernel memory tunables"
);
return Self::default();
}

start_group!("Applying kernel memory tunables");
let tunables = Self {
thp: Self::set_thp("never"),
sysctls: Self::set_sysctls(0),
swap: Self::set_swap(false, &[]),
};
Comment thread
GuillaumeLagrange marked this conversation as resolved.
Self::drop_page_cache();
end_group!();

tunables
}

/// 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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this call here? Could benefit from a comment

if let Err(error) = write_root_file("/proc/sys/vm/drop_caches", "3") {
warn!("Failed to drop the page cache: {error}");
}
}

/// Writes `value` to every THP knob, returning the modes they held before,
/// keyed by path. Knobs that are absent, unreadable or already at `value`
/// are left out.
fn set_thp(value: &str) -> Vec<(String, String)> {
let mut previous = Vec::new();

for knob in ["enabled", "defrag"] {
let path = format!("/sys/kernel/mm/transparent_hugepage/{knob}");
let Some(active) = read_thp_mode(&path) else {
debug!("{path} is missing or has no active mode, skipping");
continue;
};
if active == value {
continue;
}

match write_root_file(&path, value) {
Ok(()) => previous.push((path, active)),
Err(error) => warn!("Failed to set transparent huge pages ({path}): {error}"),
}
}

previous
}

/// Sets every stabilising sysctl to `value`, returning the previous value of
/// the ones that were not already there.
fn set_sysctls(value: i64) -> Vec<(&'static str, i64)> {
let mut names = vec!["vm.compaction_proactiveness", "vm.swappiness"];
// Absent on single-node hosts.
if Path::new("/proc/sys/kernel/numa_balancing").exists() {
names.push("kernel.numa_balancing");
}

let mut previous = Vec::new();
for name in names {
match ensure_sysctl(name, value) {
Ok(Some(before)) => previous.push((name, before)),
Ok(None) => {}
Err(error) => warn!("Failed to set {name}={value}: {error}"),
}
}

previous
}

/// Disables swap, returning the entries that were active, or re-enables
/// exactly `paths`.
///
/// Restoring path by path rather than with `swapon -a` covers a swap file
/// that was activated manually and is absent from `/etc/fstab`.
fn set_swap(enabled: bool, paths: &[String]) -> Vec<String> {
if enabled {
for path in paths {
if let Err(error) = run_with_sudo("swapon", [path]) {
warn!("Failed to re-enable swap on {path}: {error}");
}
}
return Vec::new();
}

let (Ok(swaps), Ok(meminfo)) = (
read_to_string("/proc/swaps"),
read_to_string("/proc/meminfo"),
) else {
debug!("Leaving swap enabled: could not read /proc/swaps or /proc/meminfo");
return Vec::new();
};

let active = match swapoff_decision(&swaps, &meminfo) {
SwapoffDecision::Skip(reason) => {
debug!("Leaving swap enabled: {reason}");
return Vec::new();
}
SwapoffDecision::Proceed(active) => active,
};

if let Err(error) = run_with_sudo("swapoff", ["-a"]) {
warn!("Failed to disable swap: {error}");
return Vec::new();
}
Comment on lines +145 to +148

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

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.

Fix in Claude Code Fix in Codex


active
}
}

impl Drop for MemoryTunables {
fn drop(&mut self) {
if self.thp.is_empty() && self.sysctls.is_empty() && self.swap.is_empty() {
return;
}

start_group!("Restoring kernel memory tunables");
Self::set_swap(true, &self.swap);

for (name, value) in &self.sysctls {
if let Err(error) = ensure_sysctl(name, *value) {
warn!("Failed to restore {name}={value}: {error}");
}
}

for (path, value) in &self.thp {
if let Err(error) = write_root_file(path, value) {
warn!("Failed to restore transparent huge pages ({path}) to {value}: {error}");
}
}
end_group!();
}
}

/// The active mode of a THP knob, whose value reads as `always [madvise] never`.
fn read_thp_mode(path: &str) -> Option<String> {
let content = read_to_string(path).ok()?;
let mode = content
.split_whitespace()
.find_map(|token| token.strip_prefix('[')?.strip_suffix(']'))?;

Some(mode.to_string())
}

/// Write to a root-owned /proc or /sys node. `run_with_sudo` cannot pipe stdin,
/// so the redirect happens inside a shell instead of `sudo tee`.
fn write_root_file(path: &str, value: &str) -> Result<()> {
run_with_sudo("sh", ["-c", &format!("printf '%s' {value} > {path}")])
}

#[derive(Debug, PartialEq, Eq)]
enum SwapoffDecision {
/// Swap can be disabled; carries the active entries, to re-enable them later.
Proceed(Vec<String>),
Skip(String),
}

/// `swapoff -a` faults every swapped page back into RAM and permanently breaks a
/// zram device (its `disksize` resets to 0 and a later `swapon` fails), so it is
/// only safe on a plain swap file/partition whose used pages fit in free memory.
///
/// `/proc/swaps` columns: `Filename Type Size Used Priority`, first line is a header.
fn swapoff_decision(swaps: &str, meminfo: &str) -> SwapoffDecision {
let skip = |reason: &str| SwapoffDecision::Skip(reason.to_string());

let rows: Vec<&str> = swaps
.lines()
.skip(1)
.filter(|l| !l.trim().is_empty())
.collect();
if rows.is_empty() {
return skip("no swap is active");
}

let mut used_kib: u64 = 0;
let mut active = Vec::with_capacity(rows.len());
for row in rows {
let fields: Vec<&str> = row.split_whitespace().collect();
let (Some(filename), Some(used)) = (fields.first(), fields.get(3)) else {
return skip("could not determine swap usage");
};
if filename.contains("zram") {
return skip("zram swap device present");
}
let Ok(used) = used.parse::<u64>() else {
return skip("could not determine swap usage");
};
used_kib += used;
active.push(filename.to_string());
}

let Some(available_kib) = parse_mem_available_kib(meminfo) else {
return skip("could not determine swap usage");
};

if used_kib >= available_kib {
return skip("swapped pages do not fit in available memory");
}

SwapoffDecision::Proceed(active)
}

fn parse_mem_available_kib(meminfo: &str) -> Option<u64> {
meminfo
.lines()
.find_map(|line| line.strip_prefix("MemAvailable:"))?
.split_whitespace()
.next()?
.parse()
.ok()
}

#[cfg(test)]
mod tests {
use super::*;

const MEMINFO_8G: &str = "MemTotal: 16000000 kB\nMemAvailable: 8000000 kB\n";

#[test]
fn skips_when_no_swap_is_active() {
let swaps = "Filename\t\t\t\tType\t\tSize\t\tUsed\t\tPriority\n";
assert_eq!(
swapoff_decision(swaps, MEMINFO_8G),
SwapoffDecision::Skip("no swap is active".to_string())
);
}

#[test]
fn skips_zram_swap_devices() {
let swaps = "Filename\tType\tSize\tUsed\tPriority\n/dev/zram0 partition 8000000 1024 100\n";
assert_eq!(
swapoff_decision(swaps, MEMINFO_8G),
SwapoffDecision::Skip("zram swap device present".to_string())
);
}

#[test]
fn skips_when_swapped_pages_do_not_fit_in_memory() {
let swaps = "Filename\tType\tSize\tUsed\tPriority\n/swapfile file 16000000 8000000 -2\n";
assert_eq!(
swapoff_decision(swaps, "MemAvailable: 4000000 kB\n"),
SwapoffDecision::Skip("swapped pages do not fit in available memory".to_string())
);
}

#[test]
fn skips_malformed_rows() {
let swaps = "Filename\tType\tSize\tUsed\tPriority\n/swapfile file\n";
assert_eq!(
swapoff_decision(swaps, MEMINFO_8G),
SwapoffDecision::Skip("could not determine swap usage".to_string())
);
}

#[test]
fn reports_the_active_entries_to_restore() {
let swaps = "Filename\tType\tSize\tUsed\tPriority\n/swapfile file 16000000 1024 -2\n/swap2 file 16000000 512 -3\n";
assert_eq!(
swapoff_decision(swaps, MEMINFO_8G),
SwapoffDecision::Proceed(vec!["/swapfile".to_string(), "/swap2".to_string()])
);
}

#[test]
fn reads_the_active_thp_mode() {
let dir = tempfile::tempdir().unwrap();
let path = dir.path().join("enabled");
std::fs::write(&path, "always [madvise] never\n").unwrap();

assert_eq!(
read_thp_mode(path.to_str().unwrap()),
Some("madvise".to_string())
);
}

#[test]
fn reports_no_thp_mode_when_none_is_active() {
let dir = tempfile::tempdir().unwrap();
let path = dir.path().join("enabled");
std::fs::write(&path, "always madvise never\n").unwrap();

assert_eq!(read_thp_mode(path.to_str().unwrap()), None);
}
}
1 change: 0 additions & 1 deletion src/executor/wall_time/profiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
//! (perf, samply, instruments, ...) and produces a unified set of artifacts
//! in the profile folder.

mod linux_sysctl;
pub mod perf;
pub mod samply;

Expand Down
Loading
Loading