From f6ee97af10d37610ba168412acd0d354e1e043a4 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Tue, 7 Jul 2026 03:58:16 +0000 Subject: [PATCH 01/10] Extend host tuning and fix runner run CPU isolation Add new host tuning measures to reduce benchmark run-to-run variance: - Disable NUMA balancing, timer migration, soft watchdog, and KSM - Hold CPUs out of deep C-states via /dev/cpu_dma_latency (PM QoS) - Steer device IRQs and unbound workqueues to housekeeping cores Fix a gap where one-shot runner run applied tuning but never detected the CPU layout, so cpuset pinning only happened under runner up. Add static preflight warnings at startup for conditions the runner cannot fix at runtime: irqbalance running, no cgroup v2, virtualized host, and missing isolcpus/nohz_full boot args. All new measures are on by default with individual CLI disable flags, skip gracefully when the underlying files are missing, and restore their original values on shutdown. --- plus/bencher_runner/src/cpu.rs | 91 +++++- plus/bencher_runner/src/run.rs | 38 ++- plus/bencher_runner/src/tuning/dma_latency.rs | 95 ++++++ plus/bencher_runner/src/tuning/kernel_work.rs | 238 ++++++++++++++ plus/bencher_runner/src/tuning/mod.rs | 151 ++++++++- plus/bencher_runner/src/tuning/preflight.rs | 295 ++++++++++++++++++ plus/bencher_runner/src/up/mod.rs | 14 +- services/runner/src/parser/tuning.rs | 106 +++++++ 8 files changed, 1014 insertions(+), 14 deletions(-) create mode 100644 plus/bencher_runner/src/tuning/dma_latency.rs create mode 100644 plus/bencher_runner/src/tuning/kernel_work.rs create mode 100644 plus/bencher_runner/src/tuning/preflight.rs diff --git a/plus/bencher_runner/src/cpu.rs b/plus/bencher_runner/src/cpu.rs index c0fdbc4cbd..ad19562639 100644 --- a/plus/bencher_runner/src/cpu.rs +++ b/plus/bencher_runner/src/cpu.rs @@ -200,11 +200,60 @@ fn format_cpuset(cpus: &[usize]) -> String { result } +/// Format a list of CPU IDs as a kernel hex bitmask string. +/// +/// Produces the comma-separated 32-bit word format used by files like +/// `/proc/irq/default_smp_affinity` and +/// `/sys/devices/virtual/workqueue/cpumask`. Words are ordered most +/// significant first; all words after the first are zero-padded to +/// 8 hex digits (e.g., `[0, 1]` -> `"3"`, `[32]` -> `"1,00000000"`). +#[must_use] +#[expect( + clippy::integer_division, + reason = "dividing CPU IDs into 32-bit mask words" +)] +pub fn format_cpumask(cpus: &[usize]) -> String { + use std::fmt::Write as _; + + let Some(&max) = cpus.iter().max() else { + return "0".to_owned(); + }; + + let mut words = vec![0u32; max / 32 + 1]; + for &cpu in cpus { + if let Some(word) = words.get_mut(cpu / 32) { + *word |= 1u32 << (cpu % 32); + } + } + + let mut result = String::new(); + for (index, word) in words.iter().rev().enumerate() { + if index == 0 { + // write! to String is infallible + _ = write!(result, "{word:x}"); + } else { + _ = write!(result, ",{word:08x}"); + } + } + + result +} + /// Pin the current thread to the specified CPU cores. /// /// Uses `sched_setaffinity` on Linux via the `nix` crate. #[cfg(target_os = "linux")] pub fn pin_current_thread(cpus: &[usize]) -> io::Result<()> { + pin_tid(0, cpus) +} + +/// Pin a thread by its kernel thread ID (TID) to the specified CPU cores. +/// +/// Unlike [`pin_thread`], this works on threads of other processes +/// (e.g., Firecracker vCPU threads found under `/proc//task`). +/// A TID of 0 targets the calling thread. +#[cfg(target_os = "linux")] +pub fn pin_tid(tid: libc::pid_t, cpus: &[usize]) -> io::Result<()> { use nix::sched::{CpuSet, sched_setaffinity}; use nix::unistd::Pid; @@ -218,7 +267,7 @@ pub fn pin_current_thread(cpus: &[usize]) -> io::Result<()> { .map_err(|e| io::Error::new(io::ErrorKind::InvalidInput, e))?; } - sched_setaffinity(Pid::from_raw(0), &set).map_err(io::Error::other) + sched_setaffinity(Pid::from_raw(tid), &set).map_err(io::Error::other) } /// Pin the current thread to the specified CPU cores. @@ -334,6 +383,46 @@ mod tests { assert_eq!(format_cpuset(&[5, 3, 4, 2]), "2-5"); } + #[test] + fn format_cpumask_empty() { + assert_eq!(format_cpumask(&[]), "0"); + } + + #[test] + fn format_cpumask_single() { + assert_eq!(format_cpumask(&[0]), "1"); + } + + #[test] + fn format_cpumask_low_cores() { + assert_eq!(format_cpumask(&[0, 1]), "3"); + } + + #[test] + fn format_cpumask_full_byte() { + assert_eq!(format_cpumask(&[0, 1, 2, 3, 4, 5, 6, 7]), "ff"); + } + + #[test] + fn format_cpumask_high_bit() { + assert_eq!(format_cpumask(&[31]), "80000000"); + } + + #[test] + fn format_cpumask_second_word() { + assert_eq!(format_cpumask(&[32]), "1,00000000"); + } + + #[test] + fn format_cpumask_spanning_words() { + assert_eq!(format_cpumask(&[0, 32, 33]), "3,00000001"); + } + + #[test] + fn format_cpumask_third_word() { + assert_eq!(format_cpumask(&[64, 1]), "1,00000000,00000002"); + } + #[test] fn benchmark_cpuset_string() { let layout = CpuLayout::with_core_count(8); diff --git a/plus/bencher_runner/src/run.rs b/plus/bencher_runner/src/run.rs index 2f11d98987..7bff2133e2 100644 --- a/plus/bencher_runner/src/run.rs +++ b/plus/bencher_runner/src/run.rs @@ -13,7 +13,7 @@ use camino::{Utf8Path, Utf8PathBuf}; use bencher_json::Iteration; use crate::error::RunnerError; -use crate::tuning::TuningConfig; +use crate::tuning::{TuningConfig, preflight}; /// Output from a benchmark run. #[derive(Debug)] @@ -128,11 +128,41 @@ fn build_config_from_run_args(args: &RunArgs) -> Result Result<(), RunnerError> { - // Apply host tuning — guard restores settings on drop (no-op on non-Linux) - let _tuning_guard = crate::tuning::apply(&args.tuning); + // Warn about host conditions that limit benchmark accuracy (Linux only) + preflight::print_host_warnings(); - let config = build_config_from_run_args(args)?; + // Apply host tuning — guard restores settings on drop (no-op on non-Linux) + let mut tuning_guard = crate::tuning::apply(&args.tuning); + + let mut config = build_config_from_run_args(args)?; + + // Detect the CPU layout after tuning (disabling SMT changes the core + // count), steer kernel work off the benchmark cores, and pin the run + // to them. Mirrors the `runner up` path. + #[cfg(target_os = "linux")] + { + let cpu_layout = crate::cpu::CpuLayout::detect(); + crate::tuning::apply_cpu_scoped(&args.tuning, &cpu_layout, &mut tuning_guard); + if cpu_layout.has_isolation() { + println!( + " CPU isolation: housekeeping={}, benchmark={}", + cpu_layout.housekeeping_cpuset(), + cpu_layout.benchmark_cpuset() + ); + config = config.with_cpu_layout(cpu_layout); + } else { + println!(" CPU isolation: disabled (insufficient cores)"); + } + } let iter_count = args.iter.as_usize(); for iteration in 0..iter_count { diff --git a/plus/bencher_runner/src/tuning/dma_latency.rs b/plus/bencher_runner/src/tuning/dma_latency.rs new file mode 100644 index 0000000000..f3ce897cdb --- /dev/null +++ b/plus/bencher_runner/src/tuning/dma_latency.rs @@ -0,0 +1,95 @@ +//! C-state control via the PM `QoS` interface. +//! +//! Writing a latency value (in microseconds) to `/dev/cpu_dma_latency` and +//! holding the file descriptor open asks the kernel to keep CPU exit latency +//! at or below that value. A value of 0 keeps all CPUs out of deep C-states, +//! removing wakeup latency jitter from benchmark runs. The constraint is +//! released automatically when the fd closes, so a crashed runner cannot +//! leave the host stuck in polling mode. This works on both x86 and ARM, +//! unlike per-cpu `cpuidle/state*/disable` writes. + +use super::TuningGuard; + +/// PM `QoS` device node. +pub(super) const CPU_DMA_LATENCY: &str = "/dev/cpu_dma_latency"; + +/// The kernel expects a native-endian i32 latency in microseconds. +/// The target value 0 has the same byte representation on all endiannesses. +const ZERO_LATENCY: [u8; 4] = [0; 4]; + +/// Request a maximum CPU exit latency of 0 us and hold the constraint. +/// +/// The opened file is pushed onto the guard so the constraint stays active +/// until the guard drops. Missing device nodes (e.g., kernels without +/// `CONFIG_CPU_IDLE`) are skipped with an informational message. +pub(super) fn hold_dma_latency(guard: &mut TuningGuard, path: &str) { + use std::io::Write as _; + + if !std::path::Path::new(path).exists() { + println!(" Tuning: C-states - skipped (path not found)"); + return; + } + + let mut file = match std::fs::OpenOptions::new().write(true).open(path) { + Ok(file) => file, + Err(e) => { + println!(" Tuning: C-states - skipped (open failed: {e})"); + return; + }, + }; + + if let Err(e) = file.write_all(&ZERO_LATENCY) { + println!(" Tuning: C-states - skipped (write failed: {e})"); + return; + } + + println!(" Tuning: C-states - max exit latency held at 0 us (released on exit)"); + guard.held_fds.push(file); +} + +#[cfg(test)] +mod tests { + use super::*; + + fn empty_guard() -> TuningGuard { + TuningGuard { + saved: Vec::new(), + held_fds: Vec::new(), + } + } + + #[test] + fn holds_fd_and_writes_zero_latency() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cpu_dma_latency"); + std::fs::write(&path, [0xffu8; 4]).unwrap(); + + let mut guard = empty_guard(); + hold_dma_latency(&mut guard, path.to_str().unwrap()); + + assert_eq!(guard.held_fds.len(), 1); + // The write starts at offset 0 and covers all 4 original bytes. + let contents = std::fs::read(&path).unwrap(); + assert_eq!(contents, ZERO_LATENCY); + } + + #[test] + fn skips_missing_path() { + let mut guard = empty_guard(); + hold_dma_latency(&mut guard, "/nonexistent/cpu_dma_latency"); + assert!(guard.held_fds.is_empty()); + } + + #[test] + fn guard_drop_releases_fd() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cpu_dma_latency"); + std::fs::write(&path, []).unwrap(); + + let mut guard = empty_guard(); + hold_dma_latency(&mut guard, path.to_str().unwrap()); + assert_eq!(guard.held_fds.len(), 1); + // Dropping the guard must not panic and releases the fd. + drop(guard); + } +} diff --git a/plus/bencher_runner/src/tuning/kernel_work.rs b/plus/bencher_runner/src/tuning/kernel_work.rs new file mode 100644 index 0000000000..e7ec819a4d --- /dev/null +++ b/plus/bencher_runner/src/tuning/kernel_work.rs @@ -0,0 +1,238 @@ +//! Steer device IRQs and unbound kernel workqueues to housekeeping cores. +//! +//! Device interrupts and unbound workqueue workers landing on benchmark +//! cores steal cycles mid-measurement. This module points the IRQ default +//! affinity, every movable IRQ, and the unbound workqueue cpumask at the +//! housekeeping cores instead. Some IRQs are not movable (the kernel +//! returns EIO); those are skipped and summarized in a single line. + +use camino::Utf8Path; + +use super::{TuningGuard, write_sysctl}; +use crate::cpu::{CpuLayout, format_cpumask}; + +/// Steer kernel work (IRQs and unbound workqueues) to housekeeping cores. +/// +/// `root` is the filesystem root (`/` in production); tests pass a +/// tempdir tree containing `proc/` and `sys/` subtrees. All writes are +/// saved on the guard and restored in reverse order on drop. +pub(super) fn steer_kernel_work(guard: &mut TuningGuard, layout: &CpuLayout, root: &Utf8Path) { + let housekeeping_mask = format_cpumask(&layout.housekeeping); + + // New IRQs default to housekeeping cores. + write_sysctl( + guard, + root.join("proc/irq/default_smp_affinity").as_str(), + &housekeeping_mask, + "default IRQ affinity", + ); + + // Unbound workqueue workers run on housekeeping cores. + write_sysctl( + guard, + root.join("sys/devices/virtual/workqueue/cpumask").as_str(), + &housekeeping_mask, + "workqueue cpumask", + ); + + steer_existing_irqs(guard, layout, root); +} + +/// Move every movable IRQ to the housekeeping cores. +/// +/// Iterates `proc/irq//smp_affinity_list`, skipping per-IRQ failures +/// (unmovable IRQs fail with EIO), and prints one summary line. +fn steer_existing_irqs(guard: &mut TuningGuard, layout: &CpuLayout, root: &Utf8Path) { + let irq_dir = root.join("proc/irq"); + let Ok(entries) = std::fs::read_dir(irq_dir.as_std_path()) else { + println!(" Tuning: IRQ steering - skipped (cannot read {irq_dir})"); + return; + }; + + let housekeeping_list = layout.housekeeping_cpuset(); + let mut total = 0usize; + let mut moved = 0usize; + + for entry in entries.flatten() { + let name = entry.file_name(); + let Some(name_str) = name.to_str() else { + continue; + }; + // Only numeric directories are IRQs (skips default_smp_affinity etc.) + if name_str.is_empty() || !name_str.chars().all(|c| c.is_ascii_digit()) { + continue; + } + + let affinity_path = irq_dir.join(name_str).join("smp_affinity_list"); + let Ok(current) = std::fs::read_to_string(affinity_path.as_std_path()) else { + continue; + }; + let current = current.trim().to_owned(); + total += 1; + + if current == housekeeping_list { + moved += 1; + continue; + } + + if std::fs::write(affinity_path.as_std_path(), &housekeeping_list).is_err() { + // Unmovable IRQ (EIO) or insufficient permissions - skip. + continue; + } + + moved += 1; + guard.saved.push(super::SavedSetting { + path: affinity_path, + value: current, + label: format!("IRQ {name_str} affinity"), + }); + } + + println!( + " Tuning: IRQ steering - moved {moved} of {total} IRQs to housekeeping cores ({housekeeping_list})" + ); +} + +#[cfg(test)] +mod tests { + use std::fs; + + use camino::Utf8PathBuf; + + use super::*; + + fn empty_guard() -> TuningGuard { + TuningGuard { + saved: Vec::new(), + held_fds: Vec::new(), + } + } + + /// Build a fake `/proc` + `/sys` tree with two movable IRQs. + fn fake_root() -> (tempfile::TempDir, Utf8PathBuf) { + let dir = tempfile::tempdir().unwrap(); + let root = Utf8PathBuf::try_from(dir.path().to_path_buf()).unwrap(); + + fs::create_dir_all(root.join("proc/irq/10")).unwrap(); + fs::create_dir_all(root.join("proc/irq/11")).unwrap(); + fs::create_dir_all(root.join("sys/devices/virtual/workqueue")).unwrap(); + + fs::write(root.join("proc/irq/default_smp_affinity"), "ff").unwrap(); + fs::write(root.join("proc/irq/10/smp_affinity_list"), "0-7\n").unwrap(); + fs::write(root.join("proc/irq/11/smp_affinity_list"), "0-7\n").unwrap(); + fs::write(root.join("sys/devices/virtual/workqueue/cpumask"), "ff").unwrap(); + + (dir, root) + } + + #[test] + fn steers_irqs_and_workqueues_to_housekeeping() { + let (_dir, root) = fake_root(); + let layout = CpuLayout::with_core_count(8); + + let mut guard = empty_guard(); + steer_kernel_work(&mut guard, &layout, &root); + + assert_eq!( + fs::read_to_string(root.join("proc/irq/default_smp_affinity")).unwrap(), + "3" + ); + assert_eq!( + fs::read_to_string(root.join("sys/devices/virtual/workqueue/cpumask")).unwrap(), + "3" + ); + assert_eq!( + fs::read_to_string(root.join("proc/irq/10/smp_affinity_list")).unwrap(), + "0-1" + ); + assert_eq!( + fs::read_to_string(root.join("proc/irq/11/smp_affinity_list")).unwrap(), + "0-1" + ); + // default affinity + workqueue + 2 IRQs + assert_eq!(guard.saved.len(), 4); + } + + #[test] + fn guard_drop_restores_originals() { + let (_dir, root) = fake_root(); + let layout = CpuLayout::with_core_count(8); + + { + let mut guard = empty_guard(); + steer_kernel_work(&mut guard, &layout, &root); + } + + assert_eq!( + fs::read_to_string(root.join("proc/irq/default_smp_affinity")).unwrap(), + "ff" + ); + assert_eq!( + fs::read_to_string(root.join("sys/devices/virtual/workqueue/cpumask")).unwrap(), + "ff" + ); + assert_eq!( + fs::read_to_string(root.join("proc/irq/10/smp_affinity_list")).unwrap(), + "0-7" + ); + } + + #[test] + fn skips_unmovable_irq() { + let (_dir, root) = fake_root(); + // A directory in place of the affinity file forces the write to + // fail, mimicking an unmovable IRQ (EIO) even when running as root. + fs::create_dir_all(root.join("proc/irq/12/smp_affinity_list")).unwrap(); + let layout = CpuLayout::with_core_count(8); + + let mut guard = empty_guard(); + steer_kernel_work(&mut guard, &layout, &root); + + // The unmovable IRQ is skipped; the movable ones are still steered. + assert_eq!( + fs::read_to_string(root.join("proc/irq/10/smp_affinity_list")).unwrap(), + "0-1" + ); + assert_eq!(guard.saved.len(), 4); + } + + #[test] + fn already_steered_irq_not_saved() { + let (_dir, root) = fake_root(); + fs::write(root.join("proc/irq/10/smp_affinity_list"), "0-1").unwrap(); + let layout = CpuLayout::with_core_count(8); + + let mut guard = empty_guard(); + steer_kernel_work(&mut guard, &layout, &root); + + // IRQ 10 was already on housekeeping cores: not saved for restore. + assert_eq!(guard.saved.len(), 3); + } + + #[test] + fn skips_missing_tree() { + let dir = tempfile::tempdir().unwrap(); + let root = Utf8PathBuf::try_from(dir.path().to_path_buf()).unwrap(); + let layout = CpuLayout::with_core_count(8); + + let mut guard = empty_guard(); + steer_kernel_work(&mut guard, &layout, &root); + assert!(guard.saved.is_empty()); + } + + #[test] + fn ignores_non_numeric_irq_dirs() { + let (_dir, root) = fake_root(); + fs::create_dir_all(root.join("proc/irq/not-an-irq")).unwrap(); + fs::write(root.join("proc/irq/not-an-irq/smp_affinity_list"), "0-7\n").unwrap(); + let layout = CpuLayout::with_core_count(8); + + let mut guard = empty_guard(); + steer_kernel_work(&mut guard, &layout, &root); + + assert_eq!( + fs::read_to_string(root.join("proc/irq/not-an-irq/smp_affinity_list")).unwrap(), + "0-7\n" + ); + } +} diff --git a/plus/bencher_runner/src/tuning/mod.rs b/plus/bencher_runner/src/tuning/mod.rs index afcae7ba61..7ad696ecd1 100644 --- a/plus/bencher_runner/src/tuning/mod.rs +++ b/plus/bencher_runner/src/tuning/mod.rs @@ -11,12 +11,19 @@ expect(clippy::print_stdout, reason = "tuning prints applied settings") )] +#[cfg(target_os = "linux")] +mod dma_latency; +#[cfg(target_os = "linux")] +mod kernel_work; mod perf_event_paranoid; +pub mod preflight; mod swappiness; pub use perf_event_paranoid::PerfEventParanoid; pub use swappiness::Swappiness; +use crate::cpu::CpuLayout; + #[cfg(target_os = "linux")] use camino::{Utf8Path, Utf8PathBuf}; @@ -46,6 +53,19 @@ pub struct TuningConfig { pub disable_smt: bool, /// Disable turboboost (default: true). pub disable_turbo: bool, + /// Disable automatic NUMA balancing page migration (default: true). + pub disable_numa_balancing: bool, + /// Disable timer migration between CPUs (default: true). + pub disable_timer_migration: bool, + /// Disable the soft lockup watchdog (default: true). + pub disable_soft_watchdog: bool, + /// Disable kernel samepage merging scanning (default: true). + pub disable_ksm: bool, + /// Hold CPUs out of deep C-states via `/dev/cpu_dma_latency` (default: true). + pub disable_cstates: bool, + /// Steer device IRQs and unbound workqueues to housekeeping cores + /// (default: true; only applied when the CPU layout has isolation). + pub steer_kernel_work: bool, } impl Default for TuningConfig { @@ -58,6 +78,12 @@ impl Default for TuningConfig { governor: Some("performance".to_owned()), disable_smt: true, disable_turbo: true, + disable_numa_balancing: true, + disable_timer_migration: true, + disable_soft_watchdog: true, + disable_ksm: true, + disable_cstates: true, + steer_kernel_work: true, } } } @@ -73,6 +99,12 @@ impl TuningConfig { governor: None, disable_smt: false, disable_turbo: false, + disable_numa_balancing: false, + disable_timer_migration: false, + disable_soft_watchdog: false, + disable_ksm: false, + disable_cstates: false, + steer_kernel_work: false, } } } @@ -92,6 +124,10 @@ struct SavedSetting { #[cfg(target_os = "linux")] pub struct TuningGuard { saved: Vec, + /// File descriptors held open for the lifetime of the guard + /// (e.g., the PM `QoS` constraint on `/dev/cpu_dma_latency`). + /// Dropped after the saved settings are restored. + held_fds: Vec, } #[cfg(target_os = "linux")] @@ -106,7 +142,10 @@ impl Drop for TuningGuard { /// Apply host tuning. Returns a guard that restores settings on drop. #[cfg(target_os = "linux")] pub fn apply(config: &TuningConfig) -> TuningGuard { - let mut guard = TuningGuard { saved: Vec::new() }; + let mut guard = TuningGuard { + saved: Vec::new(), + held_fds: Vec::new(), + }; if config.disable_aslr { write_sysctl( @@ -156,9 +195,56 @@ pub fn apply(config: &TuningConfig) -> TuningGuard { set_turbo(&mut guard); } + if config.disable_numa_balancing { + write_sysctl( + &mut guard, + "/proc/sys/kernel/numa_balancing", + "0", + "NUMA balancing", + ); + } + + if config.disable_timer_migration { + write_sysctl( + &mut guard, + "/proc/sys/kernel/timer_migration", + "0", + "timer migration", + ); + } + + if config.disable_soft_watchdog { + write_sysctl( + &mut guard, + "/proc/sys/kernel/soft_watchdog", + "0", + "soft watchdog", + ); + } + + if config.disable_ksm { + write_sysctl(&mut guard, "/sys/kernel/mm/ksm/run", "0", "KSM"); + } + + if config.disable_cstates { + dma_latency::hold_dma_latency(&mut guard, dma_latency::CPU_DMA_LATENCY); + } + guard } +/// Apply CPU-layout-scoped tuning (IRQ and workqueue steering). +/// +/// Must be called after [`apply`] and after [`CpuLayout::detect`], because +/// [`apply`] may disable SMT and change the core count. Settings are saved +/// on the same guard and restored on drop. +#[cfg(target_os = "linux")] +pub fn apply_cpu_scoped(config: &TuningConfig, layout: &CpuLayout, guard: &mut TuningGuard) { + if config.steer_kernel_work && layout.has_isolation() { + kernel_work::steer_kernel_work(guard, layout, Utf8Path::new("/")); + } +} + /// Read current value, write new value, and push restore entry onto the guard. #[cfg(target_os = "linux")] fn write_sysctl(guard: &mut TuningGuard, path: &str, value: &str, label: &str) { @@ -319,6 +405,10 @@ pub fn apply(_config: &TuningConfig) -> TuningGuard { TuningGuard } +/// No-op on non-Linux. +#[cfg(not(target_os = "linux"))] +pub fn apply_cpu_scoped(_config: &TuningConfig, _layout: &CpuLayout, _guard: &mut TuningGuard) {} + #[cfg(test)] mod tests { use super::*; @@ -336,6 +426,12 @@ mod tests { assert_eq!(config.governor.as_deref(), Some("performance")); assert!(config.disable_smt); assert!(config.disable_turbo); + assert!(config.disable_numa_balancing); + assert!(config.disable_timer_migration); + assert!(config.disable_soft_watchdog); + assert!(config.disable_ksm); + assert!(config.disable_cstates); + assert!(config.steer_kernel_work); } #[test] @@ -348,6 +444,34 @@ mod tests { assert_eq!(config.governor, None); assert!(!config.disable_smt); assert!(!config.disable_turbo); + assert!(!config.disable_numa_balancing); + assert!(!config.disable_timer_migration); + assert!(!config.disable_soft_watchdog); + assert!(!config.disable_ksm); + assert!(!config.disable_cstates); + assert!(!config.steer_kernel_work); + } + + #[test] + fn apply_cpu_scoped_disabled_saves_nothing() { + let config = TuningConfig::disabled(); + let layout = CpuLayout::with_core_count(8); + let mut guard = apply(&config); + apply_cpu_scoped(&config, &layout, &mut guard); + #[cfg(target_os = "linux")] + assert!(guard.saved.is_empty()); + } + + #[test] + fn apply_cpu_scoped_no_isolation_saves_nothing() { + let config = TuningConfig::default(); + let layout = CpuLayout::with_core_count(1); + // Empty guard from a disabled config; the single-core layout means + // apply_cpu_scoped must not touch anything. + let mut guard = apply(&TuningConfig::disabled()); + apply_cpu_scoped(&config, &layout, &mut guard); + #[cfg(target_os = "linux")] + assert!(guard.saved.is_empty()); } #[test] @@ -383,7 +507,10 @@ mod tests { fs::write(&file_path, "original").unwrap(); { - let mut guard = TuningGuard { saved: Vec::new() }; + let mut guard = TuningGuard { + saved: Vec::new(), + held_fds: Vec::new(), + }; guard.saved.push(SavedSetting { path: file_path.clone(), value: "original".to_owned(), @@ -410,7 +537,10 @@ mod tests { fs::write(&path2, "b").unwrap(); { - let mut guard = TuningGuard { saved: Vec::new() }; + let mut guard = TuningGuard { + saved: Vec::new(), + held_fds: Vec::new(), + }; guard.saved.push(SavedSetting { path: path1.clone(), value: "a_orig".to_owned(), @@ -430,7 +560,10 @@ mod tests { #[cfg(target_os = "linux")] #[test] fn write_sysctl_skips_missing_path() { - let mut guard = TuningGuard { saved: Vec::new() }; + let mut guard = TuningGuard { + saved: Vec::new(), + held_fds: Vec::new(), + }; write_sysctl(&mut guard, "/nonexistent/path/value", "0", "test"); assert!( guard.saved.is_empty(), @@ -447,7 +580,10 @@ mod tests { let path = dir.path().join("value"); fs::write(&path, "0").unwrap(); - let mut guard = TuningGuard { saved: Vec::new() }; + let mut guard = TuningGuard { + saved: Vec::new(), + held_fds: Vec::new(), + }; write_sysctl(&mut guard, path.to_str().unwrap(), "0", "test"); assert!( guard.saved.is_empty(), @@ -464,7 +600,10 @@ mod tests { let path = dir.path().join("value"); fs::write(&path, "60").unwrap(); - let mut guard = TuningGuard { saved: Vec::new() }; + let mut guard = TuningGuard { + saved: Vec::new(), + held_fds: Vec::new(), + }; write_sysctl(&mut guard, path.to_str().unwrap(), "10", "test"); assert_eq!(guard.saved.len(), 1); diff --git a/plus/bencher_runner/src/tuning/preflight.rs b/plus/bencher_runner/src/tuning/preflight.rs new file mode 100644 index 0000000000..e0ddd535f7 --- /dev/null +++ b/plus/bencher_runner/src/tuning/preflight.rs @@ -0,0 +1,295 @@ +//! Static preflight checks for host conditions that limit benchmark accuracy. +//! +//! These checks detect conditions the runner cannot fix at runtime +//! (virtualization, missing cgroup v2) or that would fight the runner's +//! own tuning (irqbalance). They are informational only: the runner +//! proceeds either way, falling back to its runtime isolation measures. + +use camino::Utf8Path; + +/// A host condition that may increase benchmark variance. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PreflightWarning { + /// The irqbalance daemon is running and will fight IRQ steering. + IrqBalanceRunning, + /// cgroup v2 is not available, so cpuset-based CPU isolation is not possible. + NoCgroupV2, + /// The host appears to be virtualized; hypervisor steal time adds variance. + VirtualizedHost, + /// The kernel booted without CPU isolation boot args; the runtime cpuset + /// partition is used as a fallback. + NoIsolationBootArgs, +} + +impl std::fmt::Display for PreflightWarning { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::IrqBalanceRunning => write!( + f, + "irqbalance is running and will fight IRQ steering; consider `systemctl disable --now irqbalance`" + ), + Self::NoCgroupV2 => write!( + f, + "cgroup v2 is not available; cpuset-based CPU isolation will be skipped" + ), + Self::VirtualizedHost => write!( + f, + "host appears to be virtualized; hypervisor steal time adds variance, prefer bare metal" + ), + Self::NoIsolationBootArgs => write!( + f, + "kernel booted without `isolcpus=`/`nohz_full=`; using a runtime cpuset partition as fallback (for full isolation add boot args `isolcpus= nohz_full= rcu_nocbs=` listing the benchmark cores)" + ), + } + } +} + +/// Detect preflight warnings under the given filesystem root. +/// +/// `root` is `/` in production; tests pass a tempdir tree containing +/// `proc/` and `sys/` subtrees. +#[must_use] +pub fn detect(root: &Utf8Path) -> Vec { + let mut warnings = Vec::new(); + + if irqbalance_running(root) { + warnings.push(PreflightWarning::IrqBalanceRunning); + } + + if !root.join("sys/fs/cgroup/cgroup.controllers").exists() { + warnings.push(PreflightWarning::NoCgroupV2); + } + + if virtualized_host(root) { + warnings.push(PreflightWarning::VirtualizedHost); + } + + // Only warn when the cmdline is readable and lacks the isolation args; + // an unreadable cmdline gives no signal either way. + if let Ok(cmdline) = std::fs::read_to_string(root.join("proc/cmdline").as_std_path()) + && !cmdline.contains("isolcpus=") + && !cmdline.contains("nohz_full=") + { + warnings.push(PreflightWarning::NoIsolationBootArgs); + } + + warnings +} + +/// Detect and print preflight warnings for the live host (Linux only). +pub fn print_host_warnings() { + #[cfg(target_os = "linux")] + for warning in detect(Utf8Path::new("/")) { + println!(" Preflight: {warning}"); + } +} + +/// Check whether an irqbalance process is running by scanning `proc/*/comm`. +fn irqbalance_running(root: &Utf8Path) -> bool { + let Ok(entries) = std::fs::read_dir(root.join("proc").as_std_path()) else { + return false; + }; + + for entry in entries.flatten() { + let name = entry.file_name(); + let Some(name_str) = name.to_str() else { + continue; + }; + if name_str.is_empty() || !name_str.chars().all(|c| c.is_ascii_digit()) { + continue; + } + + if let Ok(comm) = std::fs::read_to_string(entry.path().join("comm")) + && comm.trim() == "irqbalance" + { + return true; + } + } + + false +} + +/// Hypervisor strings found in DMI system vendor or product name. +const HYPERVISOR_MARKERS: &[&str] = &["QEMU", "KVM", "VMware", "Xen", "innotek", "Virtual Machine"]; + +/// Heuristic virtualization detection. +/// +/// Checks the x86 `hypervisor` cpuinfo flag and, as a cross-arch fallback, +/// well-known hypervisor strings in the DMI system vendor and product name. +fn virtualized_host(root: &Utf8Path) -> bool { + if let Ok(cpuinfo) = std::fs::read_to_string(root.join("proc/cpuinfo").as_std_path()) + && cpuinfo.lines().any(|line| { + line.starts_with("flags") && line.split_whitespace().any(|flag| flag == "hypervisor") + }) + { + return true; + } + + for dmi_file in ["sys_vendor", "product_name"] { + if let Ok(value) = std::fs::read_to_string( + root.join("sys/devices/virtual/dmi/id") + .join(dmi_file) + .as_std_path(), + ) && HYPERVISOR_MARKERS + .iter() + .any(|marker| value.contains(marker)) + { + return true; + } + } + + false +} + +#[cfg(test)] +mod tests { + use std::fs; + + use camino::Utf8PathBuf; + + use super::*; + + /// A root that raises none of the warnings. + fn quiet_root() -> (tempfile::TempDir, Utf8PathBuf) { + let dir = tempfile::tempdir().unwrap(); + let root = Utf8PathBuf::try_from(dir.path().to_path_buf()).unwrap(); + + fs::create_dir_all(root.join("proc")).unwrap(); + fs::create_dir_all(root.join("sys/fs/cgroup")).unwrap(); + fs::write(root.join("sys/fs/cgroup/cgroup.controllers"), "cpuset cpu").unwrap(); + fs::write(root.join("proc/cmdline"), "isolcpus=2-7 nohz_full=2-7\n").unwrap(); + + (dir, root) + } + + #[test] + fn quiet_host_no_warnings() { + let (_dir, root) = quiet_root(); + assert_eq!(detect(&root), Vec::new()); + } + + #[test] + fn detects_irqbalance() { + let (_dir, root) = quiet_root(); + fs::create_dir_all(root.join("proc/123")).unwrap(); + fs::write(root.join("proc/123/comm"), "irqbalance\n").unwrap(); + + assert!(detect(&root).contains(&PreflightWarning::IrqBalanceRunning)); + } + + #[test] + fn other_processes_not_flagged() { + let (_dir, root) = quiet_root(); + fs::create_dir_all(root.join("proc/123")).unwrap(); + fs::write(root.join("proc/123/comm"), "bash\n").unwrap(); + + assert!(!detect(&root).contains(&PreflightWarning::IrqBalanceRunning)); + } + + #[test] + fn detects_missing_cgroup_v2() { + let (_dir, root) = quiet_root(); + fs::remove_file(root.join("sys/fs/cgroup/cgroup.controllers")).unwrap(); + + assert!(detect(&root).contains(&PreflightWarning::NoCgroupV2)); + } + + #[test] + fn detects_hypervisor_cpuinfo_flag() { + let (_dir, root) = quiet_root(); + fs::write( + root.join("proc/cpuinfo"), + "processor : 0\nflags : fpu vme hypervisor sse2\n", + ) + .unwrap(); + + assert!(detect(&root).contains(&PreflightWarning::VirtualizedHost)); + } + + #[test] + fn bare_metal_cpuinfo_not_flagged() { + let (_dir, root) = quiet_root(); + fs::write( + root.join("proc/cpuinfo"), + "processor : 0\nflags : fpu vme sse2\n", + ) + .unwrap(); + + assert!(!detect(&root).contains(&PreflightWarning::VirtualizedHost)); + } + + #[test] + fn detects_dmi_vendor() { + let (_dir, root) = quiet_root(); + fs::create_dir_all(root.join("sys/devices/virtual/dmi/id")).unwrap(); + fs::write(root.join("sys/devices/virtual/dmi/id/sys_vendor"), "QEMU\n").unwrap(); + + assert!(detect(&root).contains(&PreflightWarning::VirtualizedHost)); + } + + #[test] + fn detects_dmi_product_name() { + let (_dir, root) = quiet_root(); + fs::create_dir_all(root.join("sys/devices/virtual/dmi/id")).unwrap(); + fs::write( + root.join("sys/devices/virtual/dmi/id/sys_vendor"), + "Microsoft Corporation\n", + ) + .unwrap(); + fs::write( + root.join("sys/devices/virtual/dmi/id/product_name"), + "Virtual Machine\n", + ) + .unwrap(); + + assert!(detect(&root).contains(&PreflightWarning::VirtualizedHost)); + } + + #[test] + fn detects_missing_isolation_boot_args() { + let (_dir, root) = quiet_root(); + fs::write(root.join("proc/cmdline"), "root=/dev/sda1 quiet\n").unwrap(); + + assert!(detect(&root).contains(&PreflightWarning::NoIsolationBootArgs)); + } + + #[test] + fn nohz_full_alone_counts_as_isolation() { + let (_dir, root) = quiet_root(); + fs::write(root.join("proc/cmdline"), "nohz_full=2-7\n").unwrap(); + + assert!(!detect(&root).contains(&PreflightWarning::NoIsolationBootArgs)); + } + + #[test] + fn unreadable_cmdline_gives_no_boot_arg_warning() { + let (_dir, root) = quiet_root(); + fs::remove_file(root.join("proc/cmdline")).unwrap(); + + assert!(!detect(&root).contains(&PreflightWarning::NoIsolationBootArgs)); + } + + #[test] + fn warning_display_is_human_readable() { + assert!( + PreflightWarning::IrqBalanceRunning + .to_string() + .contains("irqbalance") + ); + assert!( + PreflightWarning::NoCgroupV2 + .to_string() + .contains("cgroup v2") + ); + assert!( + PreflightWarning::VirtualizedHost + .to_string() + .contains("virtualized") + ); + assert!( + PreflightWarning::NoIsolationBootArgs + .to_string() + .contains("isolcpus") + ); + } +} diff --git a/plus/bencher_runner/src/up/mod.rs b/plus/bencher_runner/src/up/mod.rs index 9301e625d5..e8abafb718 100644 --- a/plus/bencher_runner/src/up/mod.rs +++ b/plus/bencher_runner/src/up/mod.rs @@ -5,7 +5,7 @@ use url::Url; use crate::cpu::CpuLayout; use crate::log_level::SandboxLogLevel; -use crate::tuning::TuningConfig; +use crate::tuning::{TuningConfig, preflight}; mod api_client; mod error; @@ -79,7 +79,11 @@ impl Up { #[expect(clippy::print_stdout, reason = "runner CLI startup output")] #[cfg_attr( not(target_os = "linux"), - expect(unused_mut, reason = "mut needed on Linux for CPU layout detection") + expect( + unused_mut, + unused_variables, + reason = "mut and tuning guard needed on Linux for CPU layout detection" + ) )] pub fn run(mut self) -> Result<(), UpError> { install_signal_handlers(); @@ -95,10 +99,13 @@ impl Up { println!(" Non-sandboxed execution: allowed"); } + // Warn about host conditions that limit benchmark accuracy (Linux only) + preflight::print_host_warnings(); + // Apply host tuning — guard restores settings on drop (no-op on non-Linux). // This must happen before CPU layout detection so that SMT changes // are reflected in the core count. - let _tuning_guard = crate::tuning::apply(&self.config.tuning); + let mut tuning_guard = crate::tuning::apply(&self.config.tuning); // Re-detect CPU layout after tuning (SMT may have changed core count). // Linux-only: CpuLayout::detect() reads /sys/devices which only exists on Linux. @@ -106,6 +113,7 @@ impl Up { { self.config.cpu_layout = Some(CpuLayout::detect()); if let Some(cpu_layout) = &self.config.cpu_layout { + crate::tuning::apply_cpu_scoped(&self.config.tuning, cpu_layout, &mut tuning_guard); if cpu_layout.has_isolation() { println!( " CPU isolation: housekeeping={}, benchmark={}", diff --git a/services/runner/src/parser/tuning.rs b/services/runner/src/parser/tuning.rs index 62d41595d7..5c65d31243 100644 --- a/services/runner/src/parser/tuning.rs +++ b/services/runner/src/parser/tuning.rs @@ -28,6 +28,30 @@ pub struct CliTuning { #[arg(long)] pub turbo: bool, + /// Keep automatic NUMA balancing enabled (default: disabled for benchmarks). + #[arg(long)] + pub numa_balancing: bool, + + /// Keep timer migration enabled (default: disabled for benchmarks). + #[arg(long)] + pub timer_migration: bool, + + /// Keep the soft lockup watchdog enabled (default: disabled for benchmarks). + #[arg(long)] + pub soft_watchdog: bool, + + /// Keep kernel samepage merging (KSM) enabled (default: disabled for benchmarks). + #[arg(long)] + pub ksm: bool, + + /// Keep deep C-states enabled (default: disabled for benchmarks). + #[arg(long)] + pub cstates: bool, + + /// Do not steer device IRQs and kernel workqueues to housekeeping cores. + #[arg(long)] + pub no_irq_steering: bool, + /// Set swappiness value (default: 10). #[arg(long)] pub swappiness: Option, @@ -64,6 +88,88 @@ impl TryFrom for TuningConfig { governor: cli.governor.or_else(|| Some("performance".to_owned())), disable_smt: !cli.smt, disable_turbo: !cli.turbo, + disable_numa_balancing: !cli.numa_balancing, + disable_timer_migration: !cli.timer_migration, + disable_soft_watchdog: !cli.soft_watchdog, + disable_ksm: !cli.ksm, + disable_cstates: !cli.cstates, + steer_kernel_work: !cli.no_irq_steering, }) } } + +#[cfg(test)] +mod tests { + use super::*; + + fn default_cli() -> CliTuning { + CliTuning { + no_tuning: false, + aslr: false, + nmi_watchdog: false, + smt: false, + turbo: false, + numa_balancing: false, + timer_migration: false, + soft_watchdog: false, + ksm: false, + cstates: false, + no_irq_steering: false, + swappiness: None, + governor: None, + perf_event_paranoid: None, + } + } + + #[test] + fn default_flags_enable_all_tuning() { + let config = TuningConfig::try_from(default_cli()).unwrap(); + assert!(config.disable_aslr); + assert!(config.disable_nmi_watchdog); + assert!(config.disable_smt); + assert!(config.disable_turbo); + assert!(config.disable_numa_balancing); + assert!(config.disable_timer_migration); + assert!(config.disable_soft_watchdog); + assert!(config.disable_ksm); + assert!(config.disable_cstates); + assert!(config.steer_kernel_work); + assert_eq!(config.swappiness, Some(Swappiness::DEFAULT)); + assert_eq!(config.perf_event_paranoid, Some(PerfEventParanoid::DEFAULT)); + assert_eq!(config.governor.as_deref(), Some("performance")); + } + + #[test] + fn no_tuning_disables_everything() { + let cli = CliTuning { + no_tuning: true, + ..default_cli() + }; + let config = TuningConfig::try_from(cli).unwrap(); + assert!(!config.disable_aslr); + assert!(!config.disable_numa_balancing); + assert!(!config.disable_cstates); + assert!(!config.steer_kernel_work); + assert_eq!(config.swappiness, None); + assert_eq!(config.governor, None); + } + + #[test] + fn individual_keep_flags_disable_single_knobs() { + let cli = CliTuning { + numa_balancing: true, + cstates: true, + no_irq_steering: true, + ..default_cli() + }; + let config = TuningConfig::try_from(cli).unwrap(); + assert!(!config.disable_numa_balancing); + assert!(!config.disable_cstates); + assert!(!config.steer_kernel_work); + // Everything else stays enabled + assert!(config.disable_aslr); + assert!(config.disable_timer_migration); + assert!(config.disable_soft_watchdog); + assert!(config.disable_ksm); + } +} From f3125fbaf774beba51e86a8e8be74f6330209da4 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Tue, 7 Jul 2026 04:04:20 +0000 Subject: [PATCH 02/10] Isolate non-sandboxed benchmark runs on benchmark cores The local (no sandbox) execution path previously ran the benchmark with no CPU isolation at all: the child shared cores with the runner's own threads. Bring it in line with the Firecracker path: - Create a per-run cgroup with a cpuset restricted to the benchmark cores, reusing CgroupManager - Attach the child race-free via Command::pre_exec: the child writes itself into the pre-opened cgroup.procs fd between fork and exec, so it never runs startup work on the wrong cores - Fall back to sched_setaffinity when cgroup setup fails (non-root, no cgroup v2); isolation is best-effort and never blocks the run - Emit RunMetrics (wall clock plus cgroup CPU and memory usage) from the local path with transport "local"; it previously emitted none --- plus/bencher_runner/src/lib.rs | 2 + plus/bencher_runner/src/local.rs | 38 +++- plus/bencher_runner/src/local_isolation.rs | 243 +++++++++++++++++++++ 3 files changed, 282 insertions(+), 1 deletion(-) create mode 100644 plus/bencher_runner/src/local_isolation.rs diff --git a/plus/bencher_runner/src/lib.rs b/plus/bencher_runner/src/lib.rs index fd365c706e..acc131ef1c 100644 --- a/plus/bencher_runner/src/lib.rs +++ b/plus/bencher_runner/src/lib.rs @@ -37,6 +37,8 @@ pub mod kernel; #[cfg(feature = "plus")] mod local; #[cfg(feature = "plus")] +mod local_isolation; +#[cfg(feature = "plus")] mod log_level; #[cfg(feature = "plus")] pub mod metrics; diff --git a/plus/bencher_runner/src/local.rs b/plus/bencher_runner/src/local.rs index 87e8eeda09..1c21410d61 100644 --- a/plus/bencher_runner/src/local.rs +++ b/plus/bencher_runner/src/local.rs @@ -26,6 +26,8 @@ use std::time::{Duration, Instant}; use camino::{Utf8Path, Utf8PathBuf}; use crate::error::RunnerError; +use crate::local_isolation::LocalIsolation; +use crate::metrics::{self, RunMetrics}; use crate::run::{RunOutput, prepare_oci_workspace}; /// Execute a single benchmark run locally on the host system. @@ -92,6 +94,13 @@ pub fn local_execute( }, }; + // Isolate the benchmark on the benchmark cores (cgroup cpuset with a + // CPU affinity fallback), mirroring the Firecracker path. Best-effort: + // failures degrade to no isolation with a warning. + let isolation = LocalIsolation::prepare(config.cpu_layout.as_ref()); + isolation.configure_command(&mut cmd); + + let start = Instant::now(); let child = cmd .spawn() .map_err(|e| crate::error::ConfigError::BinaryNotFound { @@ -99,7 +108,18 @@ pub fn local_execute( hint: format!("Failed to spawn process: {e}"), })?; - let output = wait_with_timeout(child, config.timeout_secs, cancel_flag)?; + let output = match wait_with_timeout(child, config.timeout_secs, cancel_flag) { + Ok(output) => output, + Err(e) => { + let timed_out = matches!( + e, + RunnerError::Execution(crate::error::ExecutionError::Timeout(_)) + ); + emit_run_metrics(start.elapsed(), timed_out, &isolation); + return Err(e); + }, + }; + emit_run_metrics(start.elapsed(), false, &isolation); // Collect output files from the unpacked rootfs let output_files = collect_output_files( @@ -116,6 +136,22 @@ pub fn local_execute( }) } +/// Output run metrics to stderr in the standard marker format. +/// +/// Reads cgroup metrics before the isolation (and with it the cgroup) +/// is dropped. +fn emit_run_metrics(elapsed: Duration, timed_out: bool, isolation: &LocalIsolation) { + let run_metrics = RunMetrics { + wall_clock_ms: u64::try_from(elapsed.as_millis()).unwrap_or(u64::MAX), + timed_out, + transport: "local".to_owned(), + cgroup: isolation.read_metrics(), + }; + if let Some(line) = metrics::format_metrics(&run_metrics) { + eprintln!("{line}"); + } +} + /// Output from waiting on a child process. struct WaitOutput { exit_code: i32, diff --git a/plus/bencher_runner/src/local_isolation.rs b/plus/bencher_runner/src/local_isolation.rs new file mode 100644 index 0000000000..489773d833 --- /dev/null +++ b/plus/bencher_runner/src/local_isolation.rs @@ -0,0 +1,243 @@ +//! CPU isolation for non-sandboxed (host) execution. +//! +//! Mirrors the Firecracker path: the benchmark child process is placed in +//! a per-run cgroup with a cpuset restricted to the benchmark cores, so it +//! does not share cores with the runner's own threads. Attachment happens +//! race-free in the child between `fork` and `exec` (via `pre_exec`), so +//! the benchmark never runs a single instruction on the wrong cores. +//! +//! Everything here is best-effort: without root or cgroup v2 the cgroup +//! steps are skipped with a warning and only CPU affinity is applied. + +#![cfg_attr( + target_os = "linux", + expect( + clippy::print_stdout, + clippy::print_stderr, + reason = "isolation setup prints status and best-effort warnings" + ) +)] + +use std::process::Command; + +use crate::cpu::CpuLayout; +use crate::metrics::CgroupMetrics; + +// --------------------------------------------------------------------------- +// Linux implementation +// --------------------------------------------------------------------------- + +/// CPU isolation state for a single non-sandboxed run. +#[cfg(target_os = "linux")] +pub(crate) struct LocalIsolation { + /// Per-run cgroup; `None` when cgroup setup failed or was skipped. + /// Dropping this removes the cgroup. + cgroup: Option, + /// Pre-opened `cgroup.procs` file, inherited by the child so it can + /// move itself into the cgroup before exec. + procs: Option, + /// Benchmark cores for the `sched_setaffinity` fallback. + benchmark: Vec, +} + +#[cfg(target_os = "linux")] +impl LocalIsolation { + /// Prepare CPU isolation for a non-sandboxed run. + /// + /// Never fails: on any setup error (non-root, no cgroup v2) it warns + /// and falls back to affinity-only isolation, or no isolation at all + /// when the layout has none. + pub(crate) fn prepare(layout: Option<&CpuLayout>) -> Self { + let Some(layout) = layout.filter(|layout| layout.has_isolation()) else { + return Self { + cgroup: None, + procs: None, + benchmark: Vec::new(), + }; + }; + + let benchmark = layout.benchmark.clone(); + let run_id = format!("local-{}", uuid::Uuid::new_v4()); + + let cgroup = match crate::jail::CgroupManager::new(&run_id) { + Ok(cgroup) => { + if let Err(e) = cgroup.apply_cpuset(layout) { + eprintln!("Warning: failed to apply cpuset for local run: {e}"); + } + Some(cgroup) + }, + Err(e) => { + eprintln!( + "Warning: failed to create cgroup for local run (falling back to CPU affinity only): {e}" + ); + None + }, + }; + + let procs = cgroup.as_ref().and_then(|cgroup| { + match std::fs::OpenOptions::new() + .write(true) + .open(cgroup.path().join("cgroup.procs")) + { + Ok(file) => Some(file), + Err(e) => { + eprintln!("Warning: failed to open cgroup.procs for local run: {e}"); + None + }, + } + }); + + if procs.is_some() { + println!( + "CPU isolation: benchmark pinned to cores {}", + layout.benchmark_cpuset() + ); + } + + Self { + cgroup, + procs, + benchmark, + } + } + + /// Configure the benchmark command to attach itself to the cgroup (or + /// pin its CPU affinity) in the child, between `fork` and `exec`. + /// + /// Attaching in the child instead of after `spawn` avoids the race + /// where the benchmark runs startup work on the wrong cores. + pub(crate) fn configure_command(&self, cmd: &mut Command) { + use std::os::fd::AsRawFd; + use std::os::unix::process::CommandExt as _; + + let procs_fd = self.procs.as_ref().map(AsRawFd::as_raw_fd); + let benchmark = self.benchmark.clone(); + if procs_fd.is_none() && benchmark.is_empty() { + return; + } + + #[expect( + unsafe_code, + clippy::multiple_unsafe_ops_per_block, + reason = "pre_exec and the async-signal-safe syscalls in it require unsafe" + )] + // SAFETY: The pre_exec closure runs in the forked child before exec, + // so it may only use async-signal-safe operations. It performs no + // allocation: `benchmark` was moved in before the fork, and the + // closure only issues `write` and `sched_setaffinity` syscalls plus + // stack memory manipulation (CPU_ZERO / CPU_SET). `procs_fd` stays + // valid because `self.procs` outlives the spawn (the fd is inherited + // across fork). All failures are ignored: isolation is best-effort + // and must never prevent the benchmark from running. + unsafe { + cmd.pre_exec(move || { + if let Some(fd) = procs_fd { + // Writing "0" to cgroup.procs moves the writing task. + _ = libc::write(fd, b"0".as_ptr().cast(), 1); + } + if !benchmark.is_empty() { + let mut set: libc::cpu_set_t = std::mem::zeroed(); + libc::CPU_ZERO(&mut set); + for &cpu in &benchmark { + if cpu < libc::CPU_SETSIZE as usize { + libc::CPU_SET(cpu, &mut set); + } + } + _ = libc::sched_setaffinity(0, size_of::(), &raw const set); + } + Ok(()) + }); + } + } + + /// Read cgroup resource metrics for this run, if a cgroup was created. + /// + /// Must be called before the isolation is dropped (dropping removes + /// the cgroup). + pub(crate) fn read_metrics(&self) -> Option { + self.cgroup + .as_ref() + .and_then(|cgroup| crate::metrics::read_cgroup_metrics(cgroup.path())) + } +} + +// --------------------------------------------------------------------------- +// Non-Linux stub +// --------------------------------------------------------------------------- + +/// No-op isolation for non-Linux platforms. +#[cfg(not(target_os = "linux"))] +pub(crate) struct LocalIsolation; + +#[cfg(not(target_os = "linux"))] +#[expect( + clippy::unused_self, + reason = "non-Linux stub mirrors the Linux method signatures" +)] +impl LocalIsolation { + pub(crate) fn prepare(_layout: Option<&CpuLayout>) -> Self { + Self + } + + pub(crate) fn configure_command(&self, _cmd: &mut Command) {} + + pub(crate) fn read_metrics(&self) -> Option { + None + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn no_layout_is_noop() { + let isolation = LocalIsolation::prepare(None); + assert!(isolation.read_metrics().is_none()); + #[cfg(target_os = "linux")] + { + assert!(isolation.cgroup.is_none()); + assert!(isolation.procs.is_none()); + assert!(isolation.benchmark.is_empty()); + } + } + + #[test] + fn layout_without_isolation_is_noop() { + let layout = CpuLayout::with_core_count(1); + let isolation = LocalIsolation::prepare(Some(&layout)); + assert!(isolation.read_metrics().is_none()); + #[cfg(target_os = "linux")] + { + assert!(isolation.cgroup.is_none()); + assert!(isolation.benchmark.is_empty()); + } + } + + #[test] + fn noop_isolation_leaves_command_spawnable() { + let isolation = LocalIsolation::prepare(None); + let mut cmd = Command::new("echo"); + isolation.configure_command(&mut cmd); + let status = cmd + .stdout(std::process::Stdio::null()) + .status() + .expect("echo must spawn"); + assert!(status.success()); + } + + #[cfg(target_os = "linux")] + #[test] + fn configured_command_spawns_best_effort() { + // Cgroup creation may succeed (root CI) or fall back (unprivileged); + // either way the configured command must still spawn and succeed. + let layout = CpuLayout::with_core_count(2); + let isolation = LocalIsolation::prepare(Some(&layout)); + assert_eq!(isolation.benchmark, vec![1]); + + let mut cmd = Command::new("true"); + isolation.configure_command(&mut cmd); + let status = cmd.status().expect("true must spawn"); + assert!(status.success()); + } +} From f8288ca782b017f375c2828a655f655b4eb0c872 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Tue, 7 Jul 2026 04:12:41 +0000 Subject: [PATCH 03/10] Add cpuset isolated partition and Firecracker vCPU pinning Detach the benchmark cores from the root scheduling domain by turning the parent /sys/fs/cgroup/bencher cgroup into an isolated cpuset partition, the runtime equivalent of the isolcpus= boot arg. The kernel load balancer can no longer pull other tasks onto benchmark cores. Validity is verified by read-back (the kernel reports rejected partitions inline), with a fallback chain isolated -> root -> member. Per-run cgroups inherit the partition automatically. Disable with --no-cpuset-partition. Pin each Firecracker vCPU thread (fc_vcpu N) to its own dedicated benchmark core after InstanceStart, with VMM/API threads pinned to the last benchmark core. This is the required companion to the isolated partition, which does no load balancing between its cores, and also removes vCPU migration noise (cold caches, TLB refills) without it. Also disable swap for the VM cgroup in the live Firecracker path and fill RunMetrics.cgroup from the existing cgroup metrics reader, which previously always emitted None. --- plus/bencher_runner/src/error.rs | 3 + plus/bencher_runner/src/firecracker/mod.rs | 22 +- plus/bencher_runner/src/firecracker/pin.rs | 185 +++++++++++ plus/bencher_runner/src/jail/cgroup.rs | 349 ++++++++++++++++++++- plus/bencher_runner/src/jail/mod.rs | 2 +- plus/bencher_runner/src/tuning/mod.rs | 26 ++ services/runner/src/parser/tuning.rs | 11 + 7 files changed, 591 insertions(+), 7 deletions(-) create mode 100644 plus/bencher_runner/src/firecracker/pin.rs diff --git a/plus/bencher_runner/src/error.rs b/plus/bencher_runner/src/error.rs index 0862a007da..94072dc32d 100644 --- a/plus/bencher_runner/src/error.rs +++ b/plus/bencher_runner/src/error.rs @@ -55,6 +55,9 @@ pub enum JailError { path: Utf8PathBuf, source: std::io::Error, }, + + #[error("Cpuset partition mode '{mode}' rejected by the kernel: {state}")] + PartitionInvalid { mode: String, state: String }, } #[derive(Debug, Error)] diff --git a/plus/bencher_runner/src/firecracker/mod.rs b/plus/bencher_runner/src/firecracker/mod.rs index 0044233a7d..fdbdee111b 100644 --- a/plus/bencher_runner/src/firecracker/mod.rs +++ b/plus/bencher_runner/src/firecracker/mod.rs @@ -14,6 +14,7 @@ mod client; pub mod config; pub mod error; +mod pin; mod process; mod vsock; @@ -121,6 +122,10 @@ pub fn run_firecracker( layout.benchmark_cpuset() ); } + // Keep VM memory resident: swap adds run-to-run variance + if let Err(e) = cg.disable_swap() { + eprintln!("Warning: failed to disable swap for VM cgroup: {e}"); + } Some(cg) }, Err(e) => { @@ -195,6 +200,15 @@ pub fn run_firecracker( action_type: ActionType::InstanceStart, })?; + // Pin vCPU threads (spawned during InstanceStart) to dedicated + // benchmark cores. Required companion to the isolated cpuset + // partition, which has no load balancing between cores. + if let Some(layout) = &config.cpu_layout + && layout.has_isolation() + { + pin::pin_vcpu_threads(fc_process.pid(), layout, config.vcpus); + } + // Step 5: Collect results via vsock let timeout = if config.timeout_secs > 0 { Duration::from_secs(config.timeout_secs) @@ -218,7 +232,9 @@ pub fn run_firecracker( wall_clock_ms: u64::try_from(elapsed.as_millis()).unwrap_or(u64::MAX), timed_out: matches!(e, FirecrackerError::Timeout(_)), transport: "vsock".to_owned(), - cgroup: None, + cgroup: cgroup + .as_ref() + .and_then(|cg| metrics::read_cgroup_metrics(cg.path())), }; if let Some(line) = metrics::format_metrics(&run_metrics) { eprintln!("{line}"); @@ -234,7 +250,9 @@ pub fn run_firecracker( wall_clock_ms: u64::try_from(elapsed.as_millis()).unwrap_or(u64::MAX), timed_out: false, transport: "vsock".to_owned(), - cgroup: None, + cgroup: cgroup + .as_ref() + .and_then(|cg| metrics::read_cgroup_metrics(cg.path())), }; if let Some(line) = metrics::format_metrics(&run_metrics) { eprintln!("{line}"); diff --git a/plus/bencher_runner/src/firecracker/pin.rs b/plus/bencher_runner/src/firecracker/pin.rs new file mode 100644 index 0000000000..4fd2f4b1fa --- /dev/null +++ b/plus/bencher_runner/src/firecracker/pin.rs @@ -0,0 +1,185 @@ +//! Pin Firecracker threads to dedicated benchmark cores. +//! +//! Inside an isolated cpuset partition there is no load balancing, and +//! even without one, vCPU threads migrating between benchmark cores adds +//! run-to-run variance (cold caches, TLB refills). Firecracker names its +//! vCPU threads `fc_vcpu {index}`; each is pinned to its own benchmark +//! core, and the remaining VMM/API threads are pinned to the last +//! benchmark core (housekeeping cores are outside the VM cgroup cpuset, +//! so pinning there would fail with EINVAL). All of this is best-effort +//! with warnings. + +use std::time::{Duration, Instant}; + +use camino::{Utf8Path, Utf8PathBuf}; + +use crate::cpu::{CpuLayout, pin_tid}; + +/// How long to wait for all vCPU threads to appear after `InstanceStart`. +const DISCOVERY_TIMEOUT: Duration = Duration::from_millis(500); + +/// Poll interval while waiting for vCPU threads. +const POLL_INTERVAL: Duration = Duration::from_millis(20); + +/// Pin the Firecracker process's threads to dedicated benchmark cores. +/// +/// Polls `/proc//task/*/comm` until `vcpu_count` vCPU threads appear +/// (they are spawned during `InstanceStart`) or the discovery timeout +/// elapses, then pins whatever was found. +pub(super) fn pin_vcpu_threads(fc_pid: u32, layout: &CpuLayout, vcpu_count: u8) { + let task_dir = Utf8PathBuf::from(format!("/proc/{fc_pid}/task")); + let deadline = Instant::now() + DISCOVERY_TIMEOUT; + + let tasks = loop { + let tasks = read_tasks(&task_dir); + let vcpus_found = tasks + .iter() + .filter(|(_, comm)| parse_vcpu_comm(comm).is_some()) + .count(); + if vcpus_found >= usize::from(vcpu_count) || Instant::now() >= deadline { + break tasks; + } + std::thread::sleep(POLL_INTERVAL); + }; + + if tasks.is_empty() { + eprintln!("Warning: found no Firecracker threads to pin under {task_dir}"); + return; + } + + let mut pinned_vcpus = 0usize; + for (tid, comm) in &tasks { + let Some(core) = assign_core(comm, &layout.benchmark) else { + continue; + }; + match pin_tid(*tid, &[core]) { + Ok(()) => { + if parse_vcpu_comm(comm).is_some() { + pinned_vcpus += 1; + } + }, + Err(e) => { + eprintln!("Warning: failed to pin thread '{comm}' (tid {tid}) to core {core}: {e}"); + }, + } + } + + println!( + "CPU isolation: pinned {pinned_vcpus} of {vcpu_count} vCPU threads to dedicated cores" + ); +} + +/// Read all (tid, comm) pairs under a `/proc//task` directory. +/// +/// Threads that exit mid-scan are silently skipped. +fn read_tasks(task_dir: &Utf8Path) -> Vec<(libc::pid_t, String)> { + let Ok(entries) = std::fs::read_dir(task_dir.as_std_path()) else { + return Vec::new(); + }; + + let mut tasks = Vec::new(); + for entry in entries.flatten() { + let name = entry.file_name(); + let Some(tid) = name.to_str().and_then(|name| name.parse().ok()) else { + continue; + }; + let Ok(comm) = std::fs::read_to_string(entry.path().join("comm")) else { + continue; + }; + tasks.push((tid, comm.trim().to_owned())); + } + + tasks +} + +/// Parse a Firecracker vCPU thread name (`fc_vcpu {index}`) into its index. +fn parse_vcpu_comm(comm: &str) -> Option { + comm.trim().strip_prefix("fc_vcpu ")?.parse().ok() +} + +/// Choose the benchmark core for a Firecracker thread. +/// +/// vCPU `N` gets its own core (`benchmark[N % len]`); all other threads +/// (VMM, API server) share the last benchmark core, keeping them off the +/// cores running the earlier vCPUs. +fn assign_core(comm: &str, benchmark: &[usize]) -> Option { + let &last = benchmark.last()?; + match parse_vcpu_comm(comm) { + Some(index) => benchmark.get(index % benchmark.len()).copied(), + None => Some(last), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_vcpu_comm() { + assert_eq!(parse_vcpu_comm("fc_vcpu 0"), Some(0)); + assert_eq!(parse_vcpu_comm("fc_vcpu 12"), Some(12)); + assert_eq!(parse_vcpu_comm("fc_vcpu 3\n"), Some(3)); + } + + #[test] + fn rejects_non_vcpu_comm() { + assert_eq!(parse_vcpu_comm("firecracker"), None); + assert_eq!(parse_vcpu_comm("fc_api"), None); + assert_eq!(parse_vcpu_comm("fc_vcpu"), None); + assert_eq!(parse_vcpu_comm("fc_vcpu x"), None); + assert_eq!(parse_vcpu_comm(""), None); + } + + #[test] + fn assigns_vcpus_to_dedicated_cores() { + let benchmark = vec![2, 3, 4, 5]; + assert_eq!(assign_core("fc_vcpu 0", &benchmark), Some(2)); + assert_eq!(assign_core("fc_vcpu 1", &benchmark), Some(3)); + assert_eq!(assign_core("fc_vcpu 3", &benchmark), Some(5)); + } + + #[test] + fn wraps_vcpus_beyond_core_count() { + let benchmark = vec![2, 3]; + assert_eq!(assign_core("fc_vcpu 2", &benchmark), Some(2)); + assert_eq!(assign_core("fc_vcpu 3", &benchmark), Some(3)); + } + + #[test] + fn assigns_vmm_threads_to_last_core() { + let benchmark = vec![2, 3, 4, 5]; + assert_eq!(assign_core("firecracker", &benchmark), Some(5)); + assert_eq!(assign_core("fc_api", &benchmark), Some(5)); + } + + #[test] + fn no_benchmark_cores_assigns_nothing() { + assert_eq!(assign_core("fc_vcpu 0", &[]), None); + assert_eq!(assign_core("firecracker", &[]), None); + } + + #[test] + fn reads_tasks_from_fake_proc_tree() { + let dir = tempfile::tempdir().unwrap(); + let root = Utf8PathBuf::try_from(dir.path().to_path_buf()).unwrap(); + std::fs::create_dir_all(root.join("100")).unwrap(); + std::fs::create_dir_all(root.join("101")).unwrap(); + std::fs::write(root.join("100/comm"), "firecracker\n").unwrap(); + std::fs::write(root.join("101/comm"), "fc_vcpu 0\n").unwrap(); + + let mut tasks = read_tasks(&root); + tasks.sort_unstable(); + assert_eq!( + tasks, + vec![ + (100, "firecracker".to_owned()), + (101, "fc_vcpu 0".to_owned()) + ] + ); + } + + #[test] + fn read_tasks_missing_dir_is_empty() { + assert!(read_tasks(Utf8Path::new("/nonexistent/task")).is_empty()); + } +} diff --git a/plus/bencher_runner/src/jail/cgroup.rs b/plus/bencher_runner/src/jail/cgroup.rs index 25221d2efb..b4e7b46c3a 100644 --- a/plus/bencher_runner/src/jail/cgroup.rs +++ b/plus/bencher_runner/src/jail/cgroup.rs @@ -37,11 +37,13 @@ impl CgroupManager { path: parent.clone(), source: e, })?; - - // Enable controllers in parent - Self::enable_controllers(&parent)?; } + // Enable controllers in the parent. Always attempted (idempotent): + // the parent may have been created without controllers, e.g. by + // BencherPartition at startup. + Self::enable_controllers(&parent)?; + // Create this run's cgroup if !cgroup_path.exists() { fs::create_dir_all(&cgroup_path).map_err(|e| JailError::CreateCgroup { @@ -108,7 +110,7 @@ impl CgroupManager { // Disable swap to ensure benchmark memory measurements are accurate // and to prevent swap thrashing from affecting benchmark results. - drop(self.write_file("memory.swap.max", "0")); + drop(self.disable_swap()); } // OOM group kill: when the cgroup hits its memory limit, kill ALL processes @@ -234,6 +236,14 @@ impl CgroupManager { devices } + /// Disable swap for this cgroup. + /// + /// Keeps benchmark memory resident: swap thrashing adds run-to-run + /// variance and distorts memory measurements. + pub fn disable_swap(&self) -> Result<(), RunnerError> { + self.write_file("memory.swap.max", "0") + } + /// Add the current process to this cgroup. pub fn add_self(&self) -> Result<(), RunnerError> { let pid = std::process::id(); @@ -278,6 +288,204 @@ impl Drop for CgroupManager { } } +/// Manages the parent `bencher` cgroup as a cpuset scheduler partition. +/// +/// Turning `/sys/fs/cgroup/bencher` into an `isolated` cpuset partition +/// removes the benchmark cores from the root scheduling domain: the kernel +/// load balancer can no longer pull other tasks onto them. This is the +/// runtime equivalent of the `isolcpus=` boot argument. Per-run cgroups +/// created under the partition inherit it automatically. +/// +/// The partition must be a child of a valid partition root; the cgroup +/// root always qualifies, which is why this lives on the parent `bencher` +/// cgroup and not on a per-run child. +pub struct BencherPartition { + /// The parent `bencher` cgroup path. + path: Utf8PathBuf, + /// The cgroup v2 mount point. + root: Utf8PathBuf, +} + +/// Achieved cpuset partition level, in decreasing order of isolation. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PartitionLevel { + /// Scheduler domain isolation: no load balancing onto benchmark cores. + Isolated, + /// A separate scheduler domain with load balancing inside it. + Root, + /// A plain cgroup (no partition); cpuset pinning still applies. + Member, +} + +impl BencherPartition { + /// Create a partition manager for the given cgroup v2 mount point + /// (`/sys/fs/cgroup` in production; tests pass a tempdir tree). + #[must_use] + pub fn new(cgroup_root: &Utf8Path) -> Self { + Self { + path: cgroup_root.join(BENCHER_CGROUP_BASE), + root: cgroup_root.to_owned(), + } + } + + /// Turn the `bencher` cgroup into a cpuset partition on the benchmark + /// cores, verifying the kernel accepted it. + /// + /// Falls back `isolated` -> `root` -> `member` when the kernel reports + /// the partition as invalid (read-back verification). All writes are + /// recorded on the guard and restored in reverse order on drop. + /// Best-effort: any failure degrades to [`PartitionLevel::Member`], + /// which matches the behavior before partitions were introduced. + pub fn apply( + &self, + layout: &CpuLayout, + guard: &mut crate::tuning::TuningGuard, + ) -> PartitionLevel { + // The bencher cgroup only gets cpuset files once the cpuset + // controller is enabled in the root subtree. Additive and + // idempotent, so it is not saved for restore. + if let Err(e) = fs::write(self.root.join("cgroup.subtree_control"), "+cpuset") { + eprintln!("Warning: failed to enable cpuset controller in cgroup root: {e}"); + return PartitionLevel::Member; + } + + if !self.path.exists() + && let Err(e) = fs::create_dir_all(&self.path) + { + eprintln!("Warning: failed to create cgroup {}: {e}", self.path); + return PartitionLevel::Member; + } + + // A partition needs explicit cpus and mems. + if !save_and_write( + guard, + &self.path.join("cpuset.cpus"), + &layout.benchmark_cpuset(), + "bencher cpuset.cpus", + ) { + return PartitionLevel::Member; + } + if !save_and_write( + guard, + &self.path.join("cpuset.mems"), + "0", + "bencher cpuset.mems", + ) { + return PartitionLevel::Member; + } + + let partition_path = self.path.join("cpuset.cpus.partition"); + let original = match fs::read_to_string(&partition_path) { + Ok(value) => value.trim().to_owned(), + Err(e) => { + // Missing on kernels without cpuset partition support. + eprintln!("Warning: cpuset partitions unavailable ({partition_path}: {e})"); + return PartitionLevel::Member; + }, + }; + + for (mode, level) in [ + ("isolated", PartitionLevel::Isolated), + ("root", PartitionLevel::Root), + ] { + match try_partition_mode(&partition_path, mode) { + Ok(()) => { + guard.save_restore( + partition_path, + original, + "bencher cpuset partition".to_owned(), + ); + return level; + }, + Err(e) => { + eprintln!("Warning: cpuset partition mode '{mode}' not achieved: {e}"); + }, + } + } + + PartitionLevel::Member + } +} + +impl std::fmt::Display for PartitionLevel { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Isolated => write!(f, "isolated"), + Self::Root => write!(f, "root"), + Self::Member => write!(f, "member"), + } + } +} + +/// Write a partition mode and verify the kernel accepted it. +/// +/// The kernel reports a rejected partition in the read-back value +/// (e.g., `isolated invalid (Cpu list in cpuset.cpus not exclusive)`), +/// so a successful write is not enough. +fn try_partition_mode(path: &Utf8Path, mode: &str) -> Result<(), JailError> { + fs::write(path, mode).map_err(|e| JailError::WriteCgroup { + path: path.to_owned(), + source: e, + })?; + + verify_partition_state(path, mode) +} + +/// Read back a partition file and check the kernel reports exactly `mode`. +fn verify_partition_state(path: &Utf8Path, mode: &str) -> Result<(), JailError> { + let state = fs::read_to_string(path).map_err(|e| JailError::WriteCgroup { + path: path.to_owned(), + source: e, + })?; + let state = state.trim(); + + if state == mode { + Ok(()) + } else { + Err(JailError::PartitionInvalid { + mode: mode.to_owned(), + state: state.to_owned(), + }) + } +} + +/// Save the current value of a cgroup file on the guard, then write `value`. +/// +/// An empty current value is saved as a newline so the restore write is +/// not a zero-byte no-op (clearing `cpuset.cpus` requires writing `"\n"`). +/// Returns false (with a warning) when the file cannot be read or written. +fn save_and_write( + guard: &mut crate::tuning::TuningGuard, + path: &Utf8Path, + value: &str, + label: &str, +) -> bool { + let current = match fs::read_to_string(path) { + Ok(current) => current.trim().to_owned(), + Err(e) => { + eprintln!("Warning: failed to read {path}: {e}"); + return false; + }, + }; + + if current == value { + return true; + } + + if let Err(e) = fs::write(path, value) { + eprintln!("Warning: failed to write {path}: {e}"); + return false; + } + + let restore_value = if current.is_empty() { + "\n".to_owned() + } else { + current + }; + guard.save_restore(path.to_owned(), restore_value, label.to_owned()); + true +} + /// Check if cgroup v2 is available. #[expect(dead_code, reason = "utility for future cgroup v2 feature detection")] #[must_use] @@ -286,3 +494,136 @@ pub fn is_cgroup_v2_available() -> bool { .join("cgroup.controllers") .exists() } + +#[cfg(test)] +mod tests { + use camino::Utf8PathBuf; + + use crate::cpu::CpuLayout; + use crate::tuning::{TuningConfig, TuningGuard}; + + use super::*; + + fn empty_guard() -> TuningGuard { + crate::tuning::apply(&TuningConfig::disabled()) + } + + /// A fake cgroup v2 tree mirroring what the kernel exposes. + fn fake_cgroup_root() -> (tempfile::TempDir, Utf8PathBuf) { + let dir = tempfile::tempdir().unwrap(); + let root = Utf8PathBuf::try_from(dir.path().to_path_buf()).unwrap(); + + fs::write(root.join("cgroup.subtree_control"), "").unwrap(); + fs::create_dir_all(root.join("bencher")).unwrap(); + fs::write(root.join("bencher/cpuset.cpus"), "").unwrap(); + fs::write(root.join("bencher/cpuset.mems"), "").unwrap(); + fs::write(root.join("bencher/cpuset.cpus.partition"), "member\n").unwrap(); + + (dir, root) + } + + #[test] + fn partition_applies_isolated() { + let (_dir, root) = fake_cgroup_root(); + let layout = CpuLayout::with_core_count(8); + let mut guard = empty_guard(); + + let level = BencherPartition::new(&root).apply(&layout, &mut guard); + + assert_eq!(level, PartitionLevel::Isolated); + assert_eq!( + fs::read_to_string(root.join("bencher/cpuset.cpus")).unwrap(), + "2-7" + ); + assert_eq!( + fs::read_to_string(root.join("bencher/cpuset.mems")).unwrap(), + "0" + ); + assert_eq!( + fs::read_to_string(root.join("bencher/cpuset.cpus.partition")).unwrap(), + "isolated" + ); + } + + #[test] + fn partition_restores_on_guard_drop() { + let (_dir, root) = fake_cgroup_root(); + let layout = CpuLayout::with_core_count(8); + + { + let mut guard = empty_guard(); + let level = BencherPartition::new(&root).apply(&layout, &mut guard); + assert_eq!(level, PartitionLevel::Isolated); + } + + // Partition demoted back, cpus and mems cleared (newline write). + assert_eq!( + fs::read_to_string(root.join("bencher/cpuset.cpus.partition")).unwrap(), + "member" + ); + assert_eq!( + fs::read_to_string(root.join("bencher/cpuset.cpus")).unwrap(), + "\n" + ); + assert_eq!( + fs::read_to_string(root.join("bencher/cpuset.mems")).unwrap(), + "\n" + ); + } + + #[test] + fn partition_falls_back_to_member_when_unwritable() { + let (_dir, root) = fake_cgroup_root(); + // A directory in place of the partition file forces every mode + // write to fail, exercising the full fallback chain. + fs::remove_file(root.join("bencher/cpuset.cpus.partition")).unwrap(); + fs::create_dir_all(root.join("bencher/cpuset.cpus.partition")).unwrap(); + let layout = CpuLayout::with_core_count(8); + let mut guard = empty_guard(); + + let level = BencherPartition::new(&root).apply(&layout, &mut guard); + + assert_eq!(level, PartitionLevel::Member); + } + + #[test] + fn partition_member_without_cgroup_v2() { + let dir = tempfile::tempdir().unwrap(); + let root = Utf8PathBuf::try_from(dir.path().to_path_buf()).unwrap(); + let layout = CpuLayout::with_core_count(8); + let mut guard = empty_guard(); + + let level = BencherPartition::new(&root).apply(&layout, &mut guard); + + assert_eq!(level, PartitionLevel::Member); + } + + #[test] + fn verify_partition_state_accepts_exact_mode() { + let dir = tempfile::tempdir().unwrap(); + let root = Utf8PathBuf::try_from(dir.path().to_path_buf()).unwrap(); + let path = root.join("cpuset.cpus.partition"); + fs::write(&path, "isolated\n").unwrap(); + + verify_partition_state(&path, "isolated").unwrap(); + } + + #[test] + fn verify_partition_state_rejects_invalid() { + let dir = tempfile::tempdir().unwrap(); + let root = Utf8PathBuf::try_from(dir.path().to_path_buf()).unwrap(); + let path = root.join("cpuset.cpus.partition"); + // The kernel reports a rejected partition in the read-back value. + fs::write(&path, "isolated invalid (Cpu list not exclusive)\n").unwrap(); + + let err = verify_partition_state(&path, "isolated").unwrap_err(); + assert!(err.to_string().contains("invalid")); + } + + #[test] + fn partition_level_display() { + assert_eq!(PartitionLevel::Isolated.to_string(), "isolated"); + assert_eq!(PartitionLevel::Root.to_string(), "root"); + assert_eq!(PartitionLevel::Member.to_string(), "member"); + } +} diff --git a/plus/bencher_runner/src/jail/mod.rs b/plus/bencher_runner/src/jail/mod.rs index 0d29491f15..5e06ef811d 100644 --- a/plus/bencher_runner/src/jail/mod.rs +++ b/plus/bencher_runner/src/jail/mod.rs @@ -7,7 +7,7 @@ mod cgroup; #[cfg(target_os = "linux")] -pub use cgroup::CgroupManager; +pub use cgroup::{BencherPartition, CgroupManager, PartitionLevel}; use serde::{Deserialize, Serialize}; diff --git a/plus/bencher_runner/src/tuning/mod.rs b/plus/bencher_runner/src/tuning/mod.rs index 7ad696ecd1..fe8f703bf6 100644 --- a/plus/bencher_runner/src/tuning/mod.rs +++ b/plus/bencher_runner/src/tuning/mod.rs @@ -66,6 +66,10 @@ pub struct TuningConfig { /// Steer device IRQs and unbound workqueues to housekeeping cores /// (default: true; only applied when the CPU layout has isolation). pub steer_kernel_work: bool, + /// Detach benchmark cores from the root scheduling domain via an + /// isolated cpuset partition, the runtime equivalent of `isolcpus=` + /// (default: true; only applied when the CPU layout has isolation). + pub cpuset_partition: bool, } impl Default for TuningConfig { @@ -84,6 +88,7 @@ impl Default for TuningConfig { disable_ksm: true, disable_cstates: true, steer_kernel_work: true, + cpuset_partition: true, } } } @@ -105,6 +110,7 @@ impl TuningConfig { disable_ksm: false, disable_cstates: false, steer_kernel_work: false, + cpuset_partition: false, } } } @@ -130,6 +136,18 @@ pub struct TuningGuard { held_fds: Vec, } +#[cfg(target_os = "linux")] +impl TuningGuard { + /// Record a file to restore to `value` when the guard drops. + /// + /// Restoration happens in reverse recording order, so dependent writes + /// (e.g., a cpuset partition mode that must be reverted before its + /// cpuset can shrink) are undone correctly. + pub(crate) fn save_restore(&mut self, path: Utf8PathBuf, value: String, label: String) { + self.saved.push(SavedSetting { path, value, label }); + } +} + #[cfg(target_os = "linux")] impl Drop for TuningGuard { fn drop(&mut self) { @@ -240,6 +258,12 @@ pub fn apply(config: &TuningConfig) -> TuningGuard { /// on the same guard and restored on drop. #[cfg(target_os = "linux")] pub fn apply_cpu_scoped(config: &TuningConfig, layout: &CpuLayout, guard: &mut TuningGuard) { + if config.cpuset_partition && layout.has_isolation() { + let partition = crate::jail::BencherPartition::new(Utf8Path::new("/sys/fs/cgroup")); + let level = partition.apply(layout, guard); + println!(" Tuning: cpuset partition - achieved level '{level}'"); + } + if config.steer_kernel_work && layout.has_isolation() { kernel_work::steer_kernel_work(guard, layout, Utf8Path::new("/")); } @@ -432,6 +456,7 @@ mod tests { assert!(config.disable_ksm); assert!(config.disable_cstates); assert!(config.steer_kernel_work); + assert!(config.cpuset_partition); } #[test] @@ -450,6 +475,7 @@ mod tests { assert!(!config.disable_ksm); assert!(!config.disable_cstates); assert!(!config.steer_kernel_work); + assert!(!config.cpuset_partition); } #[test] diff --git a/services/runner/src/parser/tuning.rs b/services/runner/src/parser/tuning.rs index 5c65d31243..f9d8d6fb2c 100644 --- a/services/runner/src/parser/tuning.rs +++ b/services/runner/src/parser/tuning.rs @@ -52,6 +52,11 @@ pub struct CliTuning { #[arg(long)] pub no_irq_steering: bool, + /// Do not detach benchmark cores from the scheduler via an isolated + /// cpuset partition. + #[arg(long)] + pub no_cpuset_partition: bool, + /// Set swappiness value (default: 10). #[arg(long)] pub swappiness: Option, @@ -94,6 +99,7 @@ impl TryFrom for TuningConfig { disable_ksm: !cli.ksm, disable_cstates: !cli.cstates, steer_kernel_work: !cli.no_irq_steering, + cpuset_partition: !cli.no_cpuset_partition, }) } } @@ -115,6 +121,7 @@ mod tests { ksm: false, cstates: false, no_irq_steering: false, + no_cpuset_partition: false, swappiness: None, governor: None, perf_event_paranoid: None, @@ -134,6 +141,7 @@ mod tests { assert!(config.disable_ksm); assert!(config.disable_cstates); assert!(config.steer_kernel_work); + assert!(config.cpuset_partition); assert_eq!(config.swappiness, Some(Swappiness::DEFAULT)); assert_eq!(config.perf_event_paranoid, Some(PerfEventParanoid::DEFAULT)); assert_eq!(config.governor.as_deref(), Some("performance")); @@ -150,6 +158,7 @@ mod tests { assert!(!config.disable_numa_balancing); assert!(!config.disable_cstates); assert!(!config.steer_kernel_work); + assert!(!config.cpuset_partition); assert_eq!(config.swappiness, None); assert_eq!(config.governor, None); } @@ -160,12 +169,14 @@ mod tests { numa_balancing: true, cstates: true, no_irq_steering: true, + no_cpuset_partition: true, ..default_cli() }; let config = TuningConfig::try_from(cli).unwrap(); assert!(!config.disable_numa_balancing); assert!(!config.disable_cstates); assert!(!config.steer_kernel_work); + assert!(!config.cpuset_partition); // Everything else stays enabled assert!(config.disable_aslr); assert!(config.disable_timer_migration); From 2735a74efb26e49f140aa0ccc200ab25749001a4 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Tue, 7 Jul 2026 04:17:16 +0000 Subject: [PATCH 04/10] Add guest kernel determinism params and host THP control Extend the default guest kernel cmdline with determinism params, all stock options on the bundled Firecracker CI 6.1.174 kernel: - norandmaps: guest ASLR off (overridable via kernel_cmdline, giving opt-out parity with the host --aslr flag) - nokaslr: every run boots a fresh VM, so guest KASLR is a per-run variance source unique to the sandbox path - transparent_hugepage=never: deterministic guest memory backing Make the default cmdline arch-conditional: reboot=t is an x86 triple fault; aarch64 now uses reboot=k plus keep_bootcon instead of the previous x86-flavored cmdline. Pin the host transparent hugepage mode to 'never' by default (enabled and defrag), with a --thp override; 'leave' preserves the previous behavior of not touching THP. Note for release: memory-heavy benchmarks may see a one-time baseline step from the THP change. --- plus/bencher_runner/src/config.rs | 52 ++++++++- plus/bencher_runner/src/lib.rs | 2 +- plus/bencher_runner/src/tuning/mod.rs | 162 ++++++++++++++++++++++++++ plus/bencher_runner/src/tuning/thp.rs | 101 ++++++++++++++++ services/runner/src/parser/tuning.rs | 21 +++- 5 files changed, 335 insertions(+), 3 deletions(-) create mode 100644 plus/bencher_runner/src/tuning/thp.rs diff --git a/plus/bencher_runner/src/config.rs b/plus/bencher_runner/src/config.rs index fc92156b2e..9a445747ab 100644 --- a/plus/bencher_runner/src/config.rs +++ b/plus/bencher_runner/src/config.rs @@ -175,8 +175,26 @@ fn default_disk() -> Disk { DEFAULT_DISK } +/// Default guest kernel command line. +/// +/// Shared determinism params on both architectures: `norandmaps` disables +/// guest ASLR (opt-out parity with the host `--aslr` flag via the +/// user-overridable `kernel_cmdline`), `nokaslr` disables guest KASLR +/// (every run boots a fresh VM, so KASLR is per-run variance unique to +/// the sandbox path), and `transparent_hugepage=never` makes guest memory +/// backing deterministic. +/// +/// Arch differences: `reboot=t` (triple fault) is x86-only; aarch64 uses +/// `reboot=k` and needs `keep_bootcon` for console output during boot. fn default_kernel_cmdline() -> String { - "console=ttyS0 reboot=t panic=1 pci=off root=/dev/vda rw init=/init".to_owned() + #[cfg(target_arch = "aarch64")] + { + "keep_bootcon console=ttyS0 reboot=k panic=1 pci=off root=/dev/vda rw init=/init norandmaps nokaslr transparent_hugepage=never".to_owned() + } + #[cfg(not(target_arch = "aarch64"))] + { + "console=ttyS0 reboot=t panic=1 pci=off root=/dev/vda rw init=/init norandmaps nokaslr transparent_hugepage=never".to_owned() + } } const fn default_timeout_secs() -> u64 { @@ -558,4 +576,36 @@ mod tests { fn config_with_empty_token_errors() { Config::new("img").with_token("").unwrap_err(); } + + #[test] + fn default_kernel_cmdline_guest_determinism() { + let config = Config::new("img"); + assert!(config.kernel_cmdline.contains("norandmaps")); + assert!(config.kernel_cmdline.contains("nokaslr")); + assert!(config.kernel_cmdline.contains("transparent_hugepage=never")); + assert!(config.kernel_cmdline.contains("init=/init")); + } + + #[cfg(target_arch = "aarch64")] + #[test] + fn default_kernel_cmdline_aarch64() { + let config = Config::new("img"); + assert!(config.kernel_cmdline.starts_with("keep_bootcon ")); + assert!(config.kernel_cmdline.contains("reboot=k")); + assert!(!config.kernel_cmdline.contains("reboot=t")); + } + + #[cfg(not(target_arch = "aarch64"))] + #[test] + fn default_kernel_cmdline_x86() { + let config = Config::new("img"); + assert!(config.kernel_cmdline.contains("reboot=t")); + assert!(!config.kernel_cmdline.contains("keep_bootcon")); + } + + #[test] + fn kernel_cmdline_override_preserved() { + let config = Config::new("img").with_kernel_cmdline("console=ttyS0 custom=1"); + assert_eq!(config.kernel_cmdline, "console=ttyS0 custom=1"); + } } diff --git a/plus/bencher_runner/src/lib.rs b/plus/bencher_runner/src/lib.rs index acc131ef1c..c9a95a2c55 100644 --- a/plus/bencher_runner/src/lib.rs +++ b/plus/bencher_runner/src/lib.rs @@ -66,4 +66,4 @@ pub use log_level::SandboxLogLevel; #[cfg(feature = "plus")] pub use run::{RunArgs, RunOutput, execute, resolve_oci_image, run_with_args}; #[cfg(feature = "plus")] -pub use tuning::{PerfEventParanoid, Swappiness, TuningConfig}; +pub use tuning::{ParseThpModeError, PerfEventParanoid, Swappiness, ThpMode, TuningConfig}; diff --git a/plus/bencher_runner/src/tuning/mod.rs b/plus/bencher_runner/src/tuning/mod.rs index fe8f703bf6..4bbbfb4e17 100644 --- a/plus/bencher_runner/src/tuning/mod.rs +++ b/plus/bencher_runner/src/tuning/mod.rs @@ -18,9 +18,11 @@ mod kernel_work; mod perf_event_paranoid; pub mod preflight; mod swappiness; +mod thp; pub use perf_event_paranoid::PerfEventParanoid; pub use swappiness::Swappiness; +pub use thp::{ParseThpModeError, ThpMode}; use crate::cpu::CpuLayout; @@ -70,6 +72,9 @@ pub struct TuningConfig { /// isolated cpuset partition, the runtime equivalent of `isolcpus=` /// (default: true; only applied when the CPU layout has isolation). pub cpuset_partition: bool, + /// Host transparent hugepage mode (default: `never` for deterministic + /// memory backing; `leave` preserves the host configuration). + pub thp: ThpMode, } impl Default for TuningConfig { @@ -89,6 +94,7 @@ impl Default for TuningConfig { disable_cstates: true, steer_kernel_work: true, cpuset_partition: true, + thp: ThpMode::Never, } } } @@ -111,6 +117,7 @@ impl TuningConfig { disable_cstates: false, steer_kernel_work: false, cpuset_partition: false, + thp: ThpMode::Leave, } } } @@ -248,6 +255,21 @@ pub fn apply(config: &TuningConfig) -> TuningGuard { dma_latency::hold_dma_latency(&mut guard, dma_latency::CPU_DMA_LATENCY); } + if let Some(value) = config.thp.sysfs_value() { + write_bracketed_sysctl( + &mut guard, + "/sys/kernel/mm/transparent_hugepage/enabled", + value, + "THP enabled", + ); + write_bracketed_sysctl( + &mut guard, + "/sys/kernel/mm/transparent_hugepage/defrag", + value, + "THP defrag", + ); + } + guard } @@ -305,6 +327,63 @@ fn write_sysctl(guard: &mut TuningGuard, path: &str, value: &str, label: &str) { }); } +/// Like [`write_sysctl`], but for files using the bracketed selection +/// format (e.g., `always [madvise] never` under +/// `/sys/kernel/mm/transparent_hugepage/`). The current value is the +/// bracketed token; writes take the plain token, so save and restore +/// work with the parsed value. +#[cfg(target_os = "linux")] +fn write_bracketed_sysctl(guard: &mut TuningGuard, path: &str, value: &str, label: &str) { + let path = Utf8PathBuf::from(path); + + if !path.exists() { + println!(" Tuning: {label} - skipped (path not found)"); + return; + } + + let content = match std::fs::read_to_string(&path) { + Ok(v) => v, + Err(e) => { + println!(" Tuning: {label} - skipped (read failed: {e})"); + return; + }, + }; + let Some(current) = parse_bracketed_value(&content) else { + println!( + " Tuning: {label} - skipped (unrecognized format: {})", + content.trim() + ); + return; + }; + let current = current.to_owned(); + + if current == value { + println!(" Tuning: {label} - already {value}"); + return; + } + + if let Err(e) = std::fs::write(path.as_str(), value) { + println!(" Tuning: {label} - skipped (write failed: {e})"); + return; + } + + println!(" Tuning: {label} - set to {value} (was {current})"); + guard.saved.push(SavedSetting { + path, + value: current, + label: label.to_owned(), + }); +} + +/// Extract the selected token from a bracketed sysfs value like +/// `always [madvise] never`. +#[cfg(target_os = "linux")] +fn parse_bracketed_value(content: &str) -> Option<&str> { + content + .split_whitespace() + .find_map(|token| token.strip_prefix('[')?.strip_suffix(']')) +} + /// Set the CPU scaling governor on all CPUs. #[cfg(target_os = "linux")] fn set_cpu_governor(guard: &mut TuningGuard, target: &str) { @@ -457,6 +536,7 @@ mod tests { assert!(config.disable_cstates); assert!(config.steer_kernel_work); assert!(config.cpuset_partition); + assert_eq!(config.thp, ThpMode::Never); } #[test] @@ -476,6 +556,7 @@ mod tests { assert!(!config.disable_cstates); assert!(!config.steer_kernel_work); assert!(!config.cpuset_partition); + assert_eq!(config.thp, ThpMode::Leave); } #[test] @@ -617,6 +698,87 @@ mod tests { ); } + #[cfg(target_os = "linux")] + #[test] + fn parse_bracketed_value_variants() { + assert_eq!( + parse_bracketed_value("always [madvise] never\n"), + Some("madvise") + ); + assert_eq!( + parse_bracketed_value("[always] madvise never"), + Some("always") + ); + assert_eq!( + parse_bracketed_value("always defer defer+madvise madvise [never]\n"), + Some("never") + ); + assert_eq!(parse_bracketed_value("no brackets here"), None); + assert_eq!(parse_bracketed_value(""), None); + } + + #[cfg(target_os = "linux")] + #[test] + fn write_bracketed_sysctl_saves_and_writes() { + use std::fs; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("enabled"); + fs::write(&path, "always [madvise] never\n").unwrap(); + + let mut guard = TuningGuard { + saved: Vec::new(), + held_fds: Vec::new(), + }; + write_bracketed_sysctl(&mut guard, path.to_str().unwrap(), "never", "test"); + + assert_eq!(guard.saved.len(), 1); + assert_eq!(guard.saved[0].value, "madvise"); + assert_eq!(fs::read_to_string(&path).unwrap(), "never"); + } + + #[cfg(target_os = "linux")] + #[test] + fn write_bracketed_sysctl_skips_if_already_set() { + use std::fs; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("enabled"); + fs::write(&path, "always madvise [never]\n").unwrap(); + + let mut guard = TuningGuard { + saved: Vec::new(), + held_fds: Vec::new(), + }; + write_bracketed_sysctl(&mut guard, path.to_str().unwrap(), "never", "test"); + + assert!(guard.saved.is_empty()); + // The file is untouched (still in the kernel's bracketed format) + assert_eq!( + fs::read_to_string(&path).unwrap(), + "always madvise [never]\n" + ); + } + + #[cfg(target_os = "linux")] + #[test] + fn write_bracketed_sysctl_skips_unrecognized_format() { + use std::fs; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("enabled"); + fs::write(&path, "garbage\n").unwrap(); + + let mut guard = TuningGuard { + saved: Vec::new(), + held_fds: Vec::new(), + }; + write_bracketed_sysctl(&mut guard, path.to_str().unwrap(), "never", "test"); + + assert!(guard.saved.is_empty()); + assert_eq!(fs::read_to_string(&path).unwrap(), "garbage\n"); + } + #[cfg(target_os = "linux")] #[test] fn write_sysctl_saves_and_writes() { diff --git a/plus/bencher_runner/src/tuning/thp.rs b/plus/bencher_runner/src/tuning/thp.rs new file mode 100644 index 0000000000..701736816c --- /dev/null +++ b/plus/bencher_runner/src/tuning/thp.rs @@ -0,0 +1,101 @@ +//! Transparent hugepage (THP) mode for the host. +//! +//! THP collapses regular pages into huge pages in the background, so the +//! memory backing of a benchmark process differs nondeterministically +//! between runs. Pinning the mode to `never` makes backing deterministic; +//! `leave` preserves whatever the host has configured. + +/// Error type for invalid THP mode strings. +#[derive(Debug, thiserror::Error)] +#[error("Invalid transparent hugepage mode: {0} (expected: never, madvise, always, leave)")] +pub struct ParseThpModeError(pub String); + +/// Host transparent hugepage mode applied to +/// `/sys/kernel/mm/transparent_hugepage/{enabled,defrag}`. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub enum ThpMode { + /// Disable THP for deterministic memory backing (the default). + #[default] + Never, + /// THP only for regions that request it via `madvise`. + Madvise, + /// THP for all anonymous memory. + Always, + /// Do not touch the host THP configuration. + Leave, +} + +impl ThpMode { + /// The value to write to the sysfs files, or `None` for [`Self::Leave`]. + #[must_use] + pub fn sysfs_value(self) -> Option<&'static str> { + match self { + Self::Never => Some("never"), + Self::Madvise => Some("madvise"), + Self::Always => Some("always"), + Self::Leave => None, + } + } +} + +impl std::fmt::Display for ThpMode { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.sysfs_value().unwrap_or("leave")) + } +} + +impl std::str::FromStr for ThpMode { + type Err = ParseThpModeError; + + fn from_str(s: &str) -> Result { + match s.to_lowercase().as_str() { + "never" => Ok(Self::Never), + "madvise" => Ok(Self::Madvise), + "always" => Ok(Self::Always), + "leave" => Ok(Self::Leave), + _ => Err(ParseThpModeError(s.to_owned())), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn default_is_never() { + assert_eq!(ThpMode::default(), ThpMode::Never); + } + + #[test] + fn from_str_round_trip() { + for mode in [ + ThpMode::Never, + ThpMode::Madvise, + ThpMode::Always, + ThpMode::Leave, + ] { + assert_eq!(mode.to_string().parse::().unwrap(), mode); + } + } + + #[test] + fn from_str_case_insensitive() { + assert_eq!("NEVER".parse::().unwrap(), ThpMode::Never); + assert_eq!("Leave".parse::().unwrap(), ThpMode::Leave); + } + + #[test] + fn from_str_invalid() { + let err = "sometimes".parse::().unwrap_err(); + assert!(err.to_string().contains("sometimes")); + } + + #[test] + fn sysfs_values() { + assert_eq!(ThpMode::Never.sysfs_value(), Some("never")); + assert_eq!(ThpMode::Madvise.sysfs_value(), Some("madvise")); + assert_eq!(ThpMode::Always.sysfs_value(), Some("always")); + assert_eq!(ThpMode::Leave.sysfs_value(), None); + } +} diff --git a/services/runner/src/parser/tuning.rs b/services/runner/src/parser/tuning.rs index f9d8d6fb2c..4d887d1594 100644 --- a/services/runner/src/parser/tuning.rs +++ b/services/runner/src/parser/tuning.rs @@ -1,4 +1,4 @@ -use bencher_runner::{PerfEventParanoid, Swappiness, TuningConfig}; +use bencher_runner::{PerfEventParanoid, Swappiness, ThpMode, TuningConfig}; use clap::Args; /// Host tuning flags shared by `run` and `up` subcommands. @@ -68,6 +68,11 @@ pub struct CliTuning { /// Set `perf_event_paranoid` value (default: -1). #[arg(long, allow_hyphen_values = true)] pub perf_event_paranoid: Option, + + /// Set host transparent hugepage mode (default: never). + /// Use "leave" to preserve the host configuration. + #[arg(long)] + pub thp: Option, } impl TryFrom for TuningConfig { @@ -100,6 +105,7 @@ impl TryFrom for TuningConfig { disable_cstates: !cli.cstates, steer_kernel_work: !cli.no_irq_steering, cpuset_partition: !cli.no_cpuset_partition, + thp: cli.thp.unwrap_or_default(), }) } } @@ -125,6 +131,7 @@ mod tests { swappiness: None, governor: None, perf_event_paranoid: None, + thp: None, } } @@ -142,6 +149,7 @@ mod tests { assert!(config.disable_cstates); assert!(config.steer_kernel_work); assert!(config.cpuset_partition); + assert_eq!(config.thp, ThpMode::Never); assert_eq!(config.swappiness, Some(Swappiness::DEFAULT)); assert_eq!(config.perf_event_paranoid, Some(PerfEventParanoid::DEFAULT)); assert_eq!(config.governor.as_deref(), Some("performance")); @@ -159,10 +167,21 @@ mod tests { assert!(!config.disable_cstates); assert!(!config.steer_kernel_work); assert!(!config.cpuset_partition); + assert_eq!(config.thp, ThpMode::Leave); assert_eq!(config.swappiness, None); assert_eq!(config.governor, None); } + #[test] + fn thp_override_passed_through() { + let cli = CliTuning { + thp: Some(ThpMode::Leave), + ..default_cli() + }; + let config = TuningConfig::try_from(cli).unwrap(); + assert_eq!(config.thp, ThpMode::Leave); + } + #[test] fn individual_keep_flags_disable_single_knobs() { let cli = CliTuning { From be106d0e7606249ec02db9b0954a777e5e1f6abb Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Tue, 7 Jul 2026 04:55:42 +0000 Subject: [PATCH 05/10] Address review: partition restore gap and controller enablement Save the cpuset partition restore entry before attempting any mode write. A write can succeed at the syscall level while the kernel rejects the partition in the read-back; previously the fallback path left the partition file dirty on the persistent bencher cgroup, and a later apply could even save the dirty state as its restore value. The saved value is now normalized to the mode token, since a rejected partition reads back as ' invalid ()', which is not a valid value to write back. Make enable_controllers verification-gated: write failures are now tolerated when the required controllers are already enabled, e.g. pre-configured by an admin for an unprivileged runner. This was a behavior regression from making the call unconditional. Also fix the controller check to match whole tokens; 'cpuset' alone previously satisfied the 'cpu' controller via substring matching. --- plus/bencher_runner/src/jail/cgroup.rs | 137 +++++++++++++++++++------ 1 file changed, 105 insertions(+), 32 deletions(-) diff --git a/plus/bencher_runner/src/jail/cgroup.rs b/plus/bencher_runner/src/jail/cgroup.rs index b4e7b46c3a..650130a183 100644 --- a/plus/bencher_runner/src/jail/cgroup.rs +++ b/plus/bencher_runner/src/jail/cgroup.rs @@ -61,36 +61,33 @@ impl CgroupManager { /// Enable controllers in a cgroup. /// /// Enables cpu, memory, and pids controllers (required), and io/cpuset controllers - /// (optional, for I/O throttling and CPU pinning). Returns an error if required - /// controllers cannot be enabled. + /// (optional, for I/O throttling and CPU pinning). The verification read is the + /// real gate: write failures are tolerated when the required controllers are + /// already enabled (e.g., pre-configured by an admin for an unprivileged runner). fn enable_controllers(path: &Utf8Path) -> Result<(), RunnerError> { let subtree_control = path.join("cgroup.subtree_control"); - // Try to enable all controllers at once (most efficient) - if fs::write(&subtree_control, "+cpu +memory +pids +io +cpuset").is_err() { - // Fall back to enabling without cpuset - if fs::write(&subtree_control, "+cpu +memory +pids +io").is_err() { - // Fall back to enabling required controllers without io or cpuset - fs::write(&subtree_control, "+cpu +memory +pids").map_err(|e| { - JailError::EnableControllers { - path: subtree_control.clone(), - source: e, - } - })?; - } - } + // Try to enable all controllers at once, falling back to smaller sets + let write_result = fs::write(&subtree_control, "+cpu +memory +pids +io +cpuset") + .or_else(|_| fs::write(&subtree_control, "+cpu +memory +pids +io")) + .or_else(|_| fs::write(&subtree_control, "+cpu +memory +pids")); // Verify that required controllers are enabled let enabled = fs::read_to_string(&subtree_control).unwrap_or_default(); - for required in ["cpu", "memory", "pids"] { - if !enabled.contains(required) { - return Err(JailError::MissingController { - controller: required.to_owned(), - path: subtree_control.clone(), - enabled: enabled.clone(), + if let Some(missing) = missing_required_controller(&enabled) { + return Err(match write_result { + Err(e) => JailError::EnableControllers { + path: subtree_control, + source: e, } - .into()); - } + .into(), + Ok(()) => JailError::MissingController { + controller: missing.to_owned(), + path: subtree_control, + enabled, + } + .into(), + }); } Ok(()) @@ -376,7 +373,7 @@ impl BencherPartition { let partition_path = self.path.join("cpuset.cpus.partition"); let original = match fs::read_to_string(&partition_path) { - Ok(value) => value.trim().to_owned(), + Ok(value) => partition_mode_token(&value).to_owned(), Err(e) => { // Missing on kernels without cpuset partition support. eprintln!("Warning: cpuset partitions unavailable ({partition_path}: {e})"); @@ -384,19 +381,22 @@ impl BencherPartition { }, }; + // Save the restore entry before attempting any mode write: a write + // can succeed at the syscall level while the kernel rejects the + // partition in the read-back, and the file must still revert on + // drop. Restoring an unchanged value is a harmless no-op. + guard.save_restore( + partition_path.clone(), + original, + "bencher cpuset partition".to_owned(), + ); + for (mode, level) in [ ("isolated", PartitionLevel::Isolated), ("root", PartitionLevel::Root), ] { match try_partition_mode(&partition_path, mode) { - Ok(()) => { - guard.save_restore( - partition_path, - original, - "bencher cpuset partition".to_owned(), - ); - return level; - }, + Ok(()) => return level, Err(e) => { eprintln!("Warning: cpuset partition mode '{mode}' not achieved: {e}"); }, @@ -431,6 +431,16 @@ fn try_partition_mode(path: &Utf8Path, mode: &str) -> Result<(), JailError> { verify_partition_state(path, mode) } +/// Extract the mode token from a `cpuset.cpus.partition` read-back. +/// +/// A healthy partition reads back as just the mode (`member`, `root`, +/// `isolated`); a rejected one as ` invalid ()`. Only the +/// mode token is a valid value to write back on restore. An empty or +/// unreadable value falls back to the kernel default `member`. +fn partition_mode_token(state: &str) -> &str { + state.split_whitespace().next().unwrap_or("member") +} + /// Read back a partition file and check the kernel reports exactly `mode`. fn verify_partition_state(path: &Utf8Path, mode: &str) -> Result<(), JailError> { let state = fs::read_to_string(path).map_err(|e| JailError::WriteCgroup { @@ -486,6 +496,16 @@ fn save_and_write( true } +/// Return the first required controller missing from a +/// `cgroup.subtree_control` listing, or `None` when all are enabled. +/// +/// Matches whole tokens: `cpuset` alone must not satisfy `cpu`. +fn missing_required_controller(enabled: &str) -> Option<&'static str> { + ["cpu", "memory", "pids"] + .into_iter() + .find(|required| !enabled.split_whitespace().any(|token| token == *required)) +} + /// Check if cgroup v2 is available. #[expect(dead_code, reason = "utility for future cgroup v2 feature detection")] #[must_use] @@ -598,6 +618,59 @@ mod tests { assert_eq!(level, PartitionLevel::Member); } + #[test] + fn partition_restore_entry_saved_before_mode_writes() { + let (_dir, root) = fake_cgroup_root(); + // A previous run (or crash) left the partition file dirty: the + // kernel reports rejected partitions inline in the read-back. + fs::write( + root.join("bencher/cpuset.cpus.partition"), + "isolated invalid (Cpu list not exclusive)\n", + ) + .unwrap(); + let layout = CpuLayout::with_core_count(8); + + { + let mut guard = empty_guard(); + let level = BencherPartition::new(&root).apply(&layout, &mut guard); + assert_eq!(level, PartitionLevel::Isolated); + } + + // The restore writes back the normalized mode token, never the + // full dirty state (which is not a valid value to write). + assert_eq!( + fs::read_to_string(root.join("bencher/cpuset.cpus.partition")).unwrap(), + "isolated" + ); + } + + #[test] + fn partition_mode_token_normalizes_states() { + assert_eq!(partition_mode_token("member\n"), "member"); + assert_eq!(partition_mode_token("isolated\n"), "isolated"); + assert_eq!( + partition_mode_token("isolated invalid (Cpu list not exclusive)\n"), + "isolated" + ); + assert_eq!(partition_mode_token(""), "member"); + } + + #[test] + fn missing_required_controller_matches_whole_tokens() { + assert_eq!(missing_required_controller("cpu memory pids"), None); + assert_eq!( + missing_required_controller("cpuset cpu memory pids io"), + None + ); + assert_eq!(missing_required_controller(""), Some("cpu")); + // "cpuset" alone must not satisfy the "cpu" controller + assert_eq!( + missing_required_controller("cpuset memory pids"), + Some("cpu") + ); + assert_eq!(missing_required_controller("cpu memory"), Some("pids")); + } + #[test] fn verify_partition_state_accepts_exact_mode() { let dir = tempfile::tempdir().unwrap(); From 3d12eea67832b4e88d988dc23575609b34679ac8 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Tue, 7 Jul 2026 06:16:46 +0000 Subject: [PATCH 06/10] Document crash-restore limits, THP zero-only encoding, and pinning scope Review follow-ups from PR #923, documentation only: - tuning module docs now state that restoration happens on clean shutdown only, and that a crash makes the pre-tuning values unrecoverable for the next run (the C-state fd hold and per-run cgroups self-heal by construction) - note that /dev/cpu_dma_latency only supports the literal zero value here; a non-zero latency would need i32::to_ne_bytes - note that vCPU threads run briefly before pinning and the cgroup cpuset is the hard confinement, with pinning as a refinement --- plus/bencher_runner/src/firecracker/pin.rs | 5 +++++ plus/bencher_runner/src/tuning/dma_latency.rs | 4 +++- plus/bencher_runner/src/tuning/mod.rs | 11 +++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/plus/bencher_runner/src/firecracker/pin.rs b/plus/bencher_runner/src/firecracker/pin.rs index 4fd2f4b1fa..7ec31a5adf 100644 --- a/plus/bencher_runner/src/firecracker/pin.rs +++ b/plus/bencher_runner/src/firecracker/pin.rs @@ -26,6 +26,11 @@ const POLL_INTERVAL: Duration = Duration::from_millis(20); /// Polls `/proc//task/*/comm` until `vcpu_count` vCPU threads appear /// (they are spawned during `InstanceStart`) or the discovery timeout /// elapses, then pins whatever was found. +/// +/// vCPU threads necessarily run for a moment before being pinned (they +/// do not exist until `InstanceStart`). The cgroup cpuset is the hard +/// confinement to benchmark cores; per-thread pinning is a refinement +/// on top of it that stops migration between those cores. pub(super) fn pin_vcpu_threads(fc_pid: u32, layout: &CpuLayout, vcpu_count: u8) { let task_dir = Utf8PathBuf::from(format!("/proc/{fc_pid}/task")); let deadline = Instant::now() + DISCOVERY_TIMEOUT; diff --git a/plus/bencher_runner/src/tuning/dma_latency.rs b/plus/bencher_runner/src/tuning/dma_latency.rs index f3ce897cdb..80814867a2 100644 --- a/plus/bencher_runner/src/tuning/dma_latency.rs +++ b/plus/bencher_runner/src/tuning/dma_latency.rs @@ -14,7 +14,9 @@ use super::TuningGuard; pub(super) const CPU_DMA_LATENCY: &str = "/dev/cpu_dma_latency"; /// The kernel expects a native-endian i32 latency in microseconds. -/// The target value 0 has the same byte representation on all endiannesses. +/// Only the value 0 is supported here, whose byte representation is +/// endianness-independent; any future non-zero latency must be encoded +/// with `i32::to_ne_bytes` instead of a literal array. const ZERO_LATENCY: [u8; 4] = [0; 4]; /// Request a maximum CPU exit latency of 0 us and hold the constraint. diff --git a/plus/bencher_runner/src/tuning/mod.rs b/plus/bencher_runner/src/tuning/mod.rs index 4bbbfb4e17..72df7721de 100644 --- a/plus/bencher_runner/src/tuning/mod.rs +++ b/plus/bencher_runner/src/tuning/mod.rs @@ -5,6 +5,17 @@ //! a RAII guard. Missing sysfs/procfs files are skipped with an //! informational message — this handles ARM and other platforms //! where certain controls do not exist. +//! +//! # Crash safety +//! +//! Restoration only happens on clean shutdown (guard drop). On SIGKILL +//! or panic-abort, sysctl writes, IRQ affinities, THP mode, and the +//! cpuset partition stay applied. Worse, the next run then reads the +//! already-tuned value as "current", saves nothing, and a later clean +//! exit cannot restore the true pre-tuning value; a reboot (or manual +//! reset) recovers it. Two mechanisms self-heal by construction: the +//! `/dev/cpu_dma_latency` fd releases its PM `QoS` constraint when the +//! process dies, and per-run cgroups are removed by their own cleanup. #![cfg_attr( target_os = "linux", From 3f4020e016c8670ad28722681e8931b55d560223 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Tue, 7 Jul 2026 06:23:27 +0000 Subject: [PATCH 07/10] Clarify review nits: flag polarity, pinning scope, metrics asymmetry Comment-only follow-ups from the PR #923 review: - document the CliTuning flag polarity rationale (keep-enabled flags for hardware features, --no- flags for runner-side actions) - note that at full vCPU width the highest-index vCPU shares its core with the VMM threads in assign_core - note that CPU core pinning in runner run is deliberately independent of --no-tuning, matching runner up - note why the local spawn-failure path emits no run metrics --- plus/bencher_runner/src/firecracker/pin.rs | 5 ++++- plus/bencher_runner/src/local.rs | 2 ++ plus/bencher_runner/src/run.rs | 4 +++- services/runner/src/parser/tuning.rs | 7 +++++++ 4 files changed, 16 insertions(+), 2 deletions(-) diff --git a/plus/bencher_runner/src/firecracker/pin.rs b/plus/bencher_runner/src/firecracker/pin.rs index 7ec31a5adf..10911f6df6 100644 --- a/plus/bencher_runner/src/firecracker/pin.rs +++ b/plus/bencher_runner/src/firecracker/pin.rs @@ -106,7 +106,10 @@ fn parse_vcpu_comm(comm: &str) -> Option { /// /// vCPU `N` gets its own core (`benchmark[N % len]`); all other threads /// (VMM, API server) share the last benchmark core, keeping them off the -/// cores running the earlier vCPUs. +/// cores running the earlier vCPUs. When the vCPU count equals the +/// benchmark core count, the highest-index vCPU therefore shares its +/// core with the VMM threads and sees more scheduling noise than its +/// peers; the lower-index vCPUs keep dedicated cores. fn assign_core(comm: &str, benchmark: &[usize]) -> Option { let &last = benchmark.last()?; match parse_vcpu_comm(comm) { diff --git a/plus/bencher_runner/src/local.rs b/plus/bencher_runner/src/local.rs index 1c21410d61..0c79a94182 100644 --- a/plus/bencher_runner/src/local.rs +++ b/plus/bencher_runner/src/local.rs @@ -101,6 +101,8 @@ pub fn local_execute( isolation.configure_command(&mut cmd); let start = Instant::now(); + // No metrics are emitted on spawn failure: nothing ran, so there is + // no wall clock or cgroup usage to report. let child = cmd .spawn() .map_err(|e| crate::error::ConfigError::BinaryNotFound { diff --git a/plus/bencher_runner/src/run.rs b/plus/bencher_runner/src/run.rs index 7bff2133e2..cd3a16e039 100644 --- a/plus/bencher_runner/src/run.rs +++ b/plus/bencher_runner/src/run.rs @@ -147,7 +147,9 @@ pub fn run_with_args(args: &RunArgs) -> Result<(), RunnerError> { // Detect the CPU layout after tuning (disabling SMT changes the core // count), steer kernel work off the benchmark cores, and pin the run - // to them. Mirrors the `runner up` path. + // to them. Mirrors the `runner up` path. Core pinning is core runner + // behavior, deliberately independent of --no-tuning: the tuning config + // only gates the tuning knobs (including the partition and steering). #[cfg(target_os = "linux")] { let cpu_layout = crate::cpu::CpuLayout::detect(); diff --git a/services/runner/src/parser/tuning.rs b/services/runner/src/parser/tuning.rs index 4d887d1594..e3305f42e6 100644 --- a/services/runner/src/parser/tuning.rs +++ b/services/runner/src/parser/tuning.rs @@ -2,6 +2,13 @@ use bencher_runner::{PerfEventParanoid, Swappiness, ThpMode, TuningConfig}; use clap::Args; /// Host tuning flags shared by `run` and `up` subcommands. +/// +/// Flag polarity: hardware features that tuning disables use positive +/// keep-enabled flags naming the feature (`--smt`, `--turbo`, +/// `--cstates`, `--ksm`); actions the runner itself performs use `--no-` +/// flags naming the skipped action (`--no-irq-steering`, +/// `--no-cpuset-partition`), since a bare `--irq-steering` would read +/// as enabling something that is already on by default. #[expect( clippy::struct_excessive_bools, reason = "CLI flags map to independent tuning knobs" From e07f278df353bb55a8632b461f90c2c3a912b2c5 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Tue, 7 Jul 2026 06:31:19 +0000 Subject: [PATCH 08/10] Add nosandbox metrics scenario and concurrent-runner doc note Extend cargo test-runner with a non-sandboxed scenario asserting the local path emits the BENCHER_METRICS stderr marker with transport 'local' (it previously emitted no metrics at all). Note in the tuning module docs that tuning assumes a single runner process per host: the sysctls, IRQ affinities, THP mode, and cpuset partition are host-global, so a second concurrent runner's shutdown restores them out from under the first. --- plus/bencher_runner/src/tuning/mod.rs | 5 ++++ tasks/test_runner/src/task/scenarios.rs | 34 +++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/plus/bencher_runner/src/tuning/mod.rs b/plus/bencher_runner/src/tuning/mod.rs index 72df7721de..4b07a7eed1 100644 --- a/plus/bencher_runner/src/tuning/mod.rs +++ b/plus/bencher_runner/src/tuning/mod.rs @@ -16,6 +16,11 @@ //! reset) recovers it. Two mechanisms self-heal by construction: the //! `/dev/cpu_dma_latency` fd releases its PM `QoS` constraint when the //! process dies, and per-run cgroups are removed by their own cleanup. +//! +//! Tuning also assumes a single runner process per host: the sysctls, +//! IRQ affinities, THP mode, and cpuset partition are host-global, so a +//! second concurrent runner's shutdown restores them out from under the +//! first. #![cfg_attr( target_os = "linux", diff --git a/tasks/test_runner/src/task/scenarios.rs b/tasks/test_runner/src/task/scenarios.rs index 7de4e33885..3ee044963d 100644 --- a/tasks/test_runner/src/task/scenarios.rs +++ b/tasks/test_runner/src/task/scenarios.rs @@ -2140,6 +2140,40 @@ CMD ["sh", "-c", "echo $MY_VAR"]"#, } }, }, + Scenario { + name: "nosandbox_metrics", + description: "Non-sandboxed: run metrics on stderr with local transport", + // Verifies the local path emits ---BENCHER_METRICS:{json}--- with + // transport "local" (it previously emitted no metrics at all). + dockerfile: r#"FROM busybox:musl +CMD ["echo", "local_metrics_test"]"#, + cancel_after_secs: None, + sandboxed: false, + extra_args: &["--timeout", "60"], + validate: |output| { + let metrics_line = output + .stderr + .lines() + .find(|l| l.contains("---BENCHER_METRICS:")); + let Some(line) = metrics_line else { + bail!( + "No BENCHER_METRICS line found in stderr.\nstderr: {}", + output.stderr + ) + }; + let json_str = extract_json_substr(line); + if let Ok(json) = serde_json::from_str::(json_str) + && let Some(transport) = + json.get("transport").and_then(serde_json::Value::as_str) + { + if transport == "local" { + return Ok(()); + } + bail!("Unexpected transport type: {transport}") + } + bail!("Could not find transport in metrics: {json_str}") + }, + }, Scenario { name: "nosandbox_exit_code", description: "Non-sandboxed: non-zero exit code propagation", From 8c233ddc84337919f93792580fd504f2f7d1955d Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Tue, 7 Jul 2026 07:00:00 +0000 Subject: [PATCH 09/10] Disable swap for local run cgroups, mirroring the Firecracker path The local isolation module documents that it mirrors the Firecracker path, but only applied the cpuset. Also disable swap on the per-run cgroup so benchmark memory stays resident on the no-sandbox path; swap thrashing adds run-to-run variance. --- plus/bencher_runner/src/local_isolation.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/plus/bencher_runner/src/local_isolation.rs b/plus/bencher_runner/src/local_isolation.rs index 489773d833..6b04d13eb5 100644 --- a/plus/bencher_runner/src/local_isolation.rs +++ b/plus/bencher_runner/src/local_isolation.rs @@ -64,6 +64,10 @@ impl LocalIsolation { if let Err(e) = cgroup.apply_cpuset(layout) { eprintln!("Warning: failed to apply cpuset for local run: {e}"); } + // Keep benchmark memory resident, mirroring the Firecracker path + if let Err(e) = cgroup.disable_swap() { + eprintln!("Warning: failed to disable swap for local run: {e}"); + } Some(cgroup) }, Err(e) => { From 98696e5bb6d71d1a42342219c38d819efaa6d948 Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Tue, 7 Jul 2026 07:10:21 +0000 Subject: [PATCH 10/10] Build CPU layout from online CPU IDs, normalize tuning output CpuLayout::detect() previously counted online CPUs and assumed the IDs were contiguous 0..N. Disabling SMT (on by default) on topologies with interleaved sibling numbering, common on AMD, leaves a non-contiguous online set like 0,2,4,6; the count-based layout would then pin cpusets and affinities to offline cores. Parse the actual ID list from /sys/devices/system/cpu/online instead, keeping the count-based path as a fallback. with_core_count() now delegates to with_cpu_ids(). Also normalize the remaining emdashes in the tuning output strings and the relocated host-tuning comments to hyphens, per the no-emdash policy in CLAUDE.md; new code already used hyphens, so output was mixed. --- plus/bencher_runner/src/cpu.rs | 147 +++++++++++++++++++------- plus/bencher_runner/src/run.rs | 2 +- plus/bencher_runner/src/tuning/mod.rs | 42 ++++---- plus/bencher_runner/src/up/mod.rs | 2 +- 4 files changed, 130 insertions(+), 63 deletions(-) diff --git a/plus/bencher_runner/src/cpu.rs b/plus/bencher_runner/src/cpu.rs index ad19562639..7d06979857 100644 --- a/plus/bencher_runner/src/cpu.rs +++ b/plus/bencher_runner/src/cpu.rs @@ -33,21 +33,37 @@ impl CpuLayout { /// Detect available CPUs and create a layout. /// /// Reserves 1-2 cores for housekeeping (depending on total core count) - /// and assigns the rest to benchmarks. + /// and assigns the rest to benchmarks. On Linux this reads the actual + /// online CPU IDs rather than a count: disabling SMT on topologies with + /// interleaved sibling numbering (common on AMD) leaves a non-contiguous + /// online set like `0,2,4,6`, and a count-based layout would pin to + /// offline cores. #[must_use] pub fn detect() -> Self { - let num_cores = Self::available_cores(); - Self::with_core_count(num_cores) + #[cfg(target_os = "linux")] + if let Ok(online) = fs::read_to_string("/sys/devices/system/cpu/online") + && let Some(ids) = parse_cpu_id_list(&online) + && !ids.is_empty() + { + return Self::with_cpu_ids(ids); + } + + Self::with_core_count(Self::available_cores()) } - /// Create a layout for a given number of cores. + /// Create a layout from an explicit list of online CPU IDs. /// - /// - 1 core: housekeeping = [0], benchmark = [0] (shared, no isolation) - /// - 2-7 cores: housekeeping = [0], benchmark = [1..n] - /// - 8+ cores: housekeeping = [0, 1], benchmark = [2..n] + /// IDs need not be contiguous. The lowest 1-2 IDs become housekeeping + /// cores and the rest benchmark cores: + /// - 1 ID: housekeeping and benchmark share it (no isolation) + /// - 2-7 IDs: housekeeping = first, benchmark = rest + /// - 8+ IDs: housekeeping = first two, benchmark = rest #[must_use] - pub fn with_core_count(num_cores: usize) -> Self { - if num_cores == 0 { + pub fn with_cpu_ids(mut ids: Vec) -> Self { + ids.sort_unstable(); + ids.dedup(); + + if ids.is_empty() { // Fallback for edge cases return Self { housekeeping: vec![0], @@ -55,27 +71,33 @@ impl CpuLayout { }; } - if num_cores == 1 { + if ids.len() == 1 { // Single core - no isolation possible return Self { - housekeeping: vec![0], - benchmark: vec![0], + housekeeping: ids.clone(), + benchmark: ids, }; } // Reserve 1 core for housekeeping on small systems, 2 on larger ones - let housekeeping_count = if num_cores >= 8 { 2 } else { 1 }; - - let housekeeping: Vec = (0..housekeeping_count).collect(); - let benchmark: Vec = (housekeeping_count..num_cores).collect(); + let housekeeping_count = if ids.len() >= 8 { 2 } else { 1 }; + let benchmark = ids.split_off(housekeeping_count); Self { - housekeeping, + housekeeping: ids, benchmark, } } + /// Create a layout for a given number of contiguous cores `0..n`. + #[must_use] + pub fn with_core_count(num_cores: usize) -> Self { + Self::with_cpu_ids((0..num_cores).collect()) + } + /// Get the number of available CPU cores. + /// + /// Count-based fallback for when the online CPU ID list is unavailable. #[cfg(target_os = "linux")] #[expect( clippy::cast_possible_truncation, @@ -83,14 +105,6 @@ impl CpuLayout { reason = "sysconf returns c_long; core count always fits in usize" )] fn available_cores() -> usize { - // Try reading from /sys/devices/system/cpu/online first - if let Ok(online) = fs::read_to_string("/sys/devices/system/cpu/online") - && let Some(count) = parse_cpu_list(&online) - { - return count; - } - - // Fallback to nix sysconf match nix::unistd::sysconf(nix::unistd::SysconfVar::_NPROCESSORS_ONLN) { Ok(Some(n)) if n > 0 => n as usize, _ => 1, // Ultimate fallback @@ -128,24 +142,25 @@ impl CpuLayout { } } -/// Parse a CPU list string like "0-3" or "0,2,4" or "0-3,8-11". -/// -/// Returns the total count of CPUs in the list. +/// Parse a kernel CPU list string like "0-3" or "0,2,4" or "0-3,8-11" +/// into the expanded list of CPU IDs. #[cfg(target_os = "linux")] -fn parse_cpu_list(s: &str) -> Option { - let mut count = 0; +fn parse_cpu_id_list(s: &str) -> Option> { + let mut ids = Vec::new(); for part in s.trim().split(',') { let part = part.trim(); if let Some((start, end)) = part.split_once('-') { let start: usize = start.trim().parse().ok()?; let end: usize = end.trim().parse().ok()?; - count += end - start + 1; + if end < start { + return None; + } + ids.extend(start..=end); } else { - let _: usize = part.parse().ok()?; - count += 1; + ids.push(part.parse().ok()?); } } - Some(count) + Some(ids) } /// Format a list of CPU IDs as a cpuset string. @@ -358,6 +373,48 @@ mod tests { assert!(layout.has_isolation()); } + #[test] + fn layout_interleaved_smt_offline() { + // SMT disabled on an interleaved-sibling topology (e.g., AMD): + // only the even-numbered cores remain online. + let layout = CpuLayout::with_cpu_ids(vec![0, 2, 4, 6]); + assert_eq!(layout.housekeeping, vec![0]); + assert_eq!(layout.benchmark, vec![2, 4, 6]); + assert!(layout.has_isolation()); + assert_eq!(layout.benchmark_cpuset(), "2,4,6"); + } + + #[test] + fn layout_interleaved_eight_ids() { + let layout = CpuLayout::with_cpu_ids(vec![0, 2, 4, 6, 8, 10, 12, 14]); + assert_eq!(layout.housekeeping, vec![0, 2]); + assert_eq!(layout.benchmark, vec![4, 6, 8, 10, 12, 14]); + assert!(layout.has_isolation()); + } + + #[test] + fn layout_ids_unsorted_and_duplicated() { + let layout = CpuLayout::with_cpu_ids(vec![6, 2, 0, 4, 2]); + assert_eq!(layout.housekeeping, vec![0]); + assert_eq!(layout.benchmark, vec![2, 4, 6]); + } + + #[test] + fn layout_single_nonzero_id() { + let layout = CpuLayout::with_cpu_ids(vec![5]); + assert_eq!(layout.housekeeping, vec![5]); + assert_eq!(layout.benchmark, vec![5]); + assert!(!layout.has_isolation()); + } + + #[test] + fn layout_empty_ids() { + let layout = CpuLayout::with_cpu_ids(Vec::new()); + assert_eq!(layout.housekeeping, vec![0]); + assert_eq!(layout.benchmark, vec![0]); + assert!(!layout.has_isolation()); + } + #[test] fn format_cpuset_empty() { assert_eq!(format_cpuset(&[]), ""); @@ -437,19 +494,29 @@ mod tests { #[cfg(target_os = "linux")] #[test] - fn parse_cpu_list_simple() { - assert_eq!(parse_cpu_list("0-3"), Some(4)); + fn parse_cpu_id_list_simple() { + assert_eq!(parse_cpu_id_list("0-3"), Some(vec![0, 1, 2, 3])); + } + + #[cfg(target_os = "linux")] + #[test] + fn parse_cpu_id_list_complex() { + assert_eq!( + parse_cpu_id_list("0-3,8-11\n"), + Some(vec![0, 1, 2, 3, 8, 9, 10, 11]) + ); } #[cfg(target_os = "linux")] #[test] - fn parse_cpu_list_complex() { - assert_eq!(parse_cpu_list("0-3,8-11"), Some(8)); + fn parse_cpu_id_list_individual() { + assert_eq!(parse_cpu_id_list("0,2,4"), Some(vec![0, 2, 4])); } #[cfg(target_os = "linux")] #[test] - fn parse_cpu_list_individual() { - assert_eq!(parse_cpu_list("0,2,4"), Some(3)); + fn parse_cpu_id_list_invalid() { + assert_eq!(parse_cpu_id_list("abc"), None); + assert_eq!(parse_cpu_id_list("3-1"), None); } } diff --git a/plus/bencher_runner/src/run.rs b/plus/bencher_runner/src/run.rs index cd3a16e039..08cd223e52 100644 --- a/plus/bencher_runner/src/run.rs +++ b/plus/bencher_runner/src/run.rs @@ -140,7 +140,7 @@ pub fn run_with_args(args: &RunArgs) -> Result<(), RunnerError> { // Warn about host conditions that limit benchmark accuracy (Linux only) preflight::print_host_warnings(); - // Apply host tuning — guard restores settings on drop (no-op on non-Linux) + // Apply host tuning - guard restores settings on drop (no-op on non-Linux) let mut tuning_guard = crate::tuning::apply(&args.tuning); let mut config = build_config_from_run_args(args)?; diff --git a/plus/bencher_runner/src/tuning/mod.rs b/plus/bencher_runner/src/tuning/mod.rs index 4b07a7eed1..b7a8bc843c 100644 --- a/plus/bencher_runner/src/tuning/mod.rs +++ b/plus/bencher_runner/src/tuning/mod.rs @@ -3,7 +3,7 @@ //! Applies system-level optimizations (ASLR, CPU governor, SMT, etc.) //! to reduce benchmark noise. All settings are restored on drop via //! a RAII guard. Missing sysfs/procfs files are skipped with an -//! informational message — this handles ARM and other platforms +//! informational message - this handles ARM and other platforms //! where certain controls do not exist. //! //! # Crash safety @@ -50,7 +50,7 @@ const INTEL_NO_TURBO: &str = "/sys/devices/system/cpu/intel_pstate/no_turbo"; #[cfg(target_os = "linux")] const CPUFREQ_BOOST: &str = "/sys/devices/system/cpu/cpufreq/boost"; -/// Host tuning configuration — all defaults optimize for benchmark accuracy. +/// Host tuning configuration - all defaults optimize for benchmark accuracy. #[expect( clippy::struct_excessive_bools, reason = "each bool maps to an independent system knob" @@ -116,7 +116,7 @@ impl Default for TuningConfig { } impl TuningConfig { - /// A config with all tuning disabled — no changes will be made. + /// A config with all tuning disabled - no changes will be made. pub fn disabled() -> Self { Self { disable_aslr: false, @@ -313,29 +313,29 @@ fn write_sysctl(guard: &mut TuningGuard, path: &str, value: &str, label: &str) { let path = Utf8PathBuf::from(path); if !path.exists() { - println!(" Tuning: {label} — skipped (path not found)"); + println!(" Tuning: {label} - skipped (path not found)"); return; } let current = match std::fs::read_to_string(&path) { Ok(v) => v.trim().to_owned(), Err(e) => { - println!(" Tuning: {label} — skipped (read failed: {e})"); + println!(" Tuning: {label} - skipped (read failed: {e})"); return; }, }; if current == value { - println!(" Tuning: {label} — already {value}"); + println!(" Tuning: {label} - already {value}"); return; } if let Err(e) = std::fs::write(path.as_str(), value) { - println!(" Tuning: {label} — skipped (write failed: {e})"); + println!(" Tuning: {label} - skipped (write failed: {e})"); return; } - println!(" Tuning: {label} — set to {value} (was {current})"); + println!(" Tuning: {label} - set to {value} (was {current})"); guard.saved.push(SavedSetting { path, value: current, @@ -405,7 +405,7 @@ fn parse_bracketed_value(content: &str) -> Option<&str> { fn set_cpu_governor(guard: &mut TuningGuard, target: &str) { let base = Utf8Path::new("/sys/devices/system/cpu"); let Ok(entries) = std::fs::read_dir(base) else { - println!(" Tuning: CPU governor — skipped (cannot read {base})"); + println!(" Tuning: CPU governor - skipped (cannot read {base})"); return; }; @@ -438,11 +438,11 @@ fn set_cpu_governor(guard: &mut TuningGuard, target: &str) { } if let Err(e) = std::fs::write(&gov_path, target) { - println!(" Tuning: CPU governor ({name_str}) — skipped (write failed: {e})"); + println!(" Tuning: CPU governor ({name_str}) - skipped (write failed: {e})"); continue; } - println!(" Tuning: CPU governor ({name_str}) — set to {target} (was {current})"); + println!(" Tuning: CPU governor ({name_str}) - set to {target} (was {current})"); let gov_utf8_path = Utf8PathBuf::try_from(gov_path) .unwrap_or_else(|p| Utf8PathBuf::from(p.into_path_buf().to_string_lossy().as_ref())); guard.saved.push(SavedSetting { @@ -459,29 +459,29 @@ fn set_smt(guard: &mut TuningGuard) { let path = Utf8PathBuf::from("/sys/devices/system/cpu/smt/control"); if !path.exists() { - println!(" Tuning: SMT — skipped (not available on this platform)"); + println!(" Tuning: SMT - skipped (not available on this platform)"); return; } let current = match std::fs::read_to_string(&path) { Ok(v) => v.trim().to_owned(), Err(e) => { - println!(" Tuning: SMT — skipped (read failed: {e})"); + println!(" Tuning: SMT - skipped (read failed: {e})"); return; }, }; if current == "off" || current == "notsupported" || current == "notimplemented" { - println!(" Tuning: SMT — already {current}"); + println!(" Tuning: SMT - already {current}"); return; } if let Err(e) = std::fs::write(path.as_str(), "off") { - println!(" Tuning: SMT — skipped (write failed: {e})"); + println!(" Tuning: SMT - skipped (write failed: {e})"); return; } - println!(" Tuning: SMT — disabled (was {current})"); + println!(" Tuning: SMT - disabled (was {current})"); guard.saved.push(SavedSetting { path, value: current, @@ -497,7 +497,7 @@ fn set_turbo(guard: &mut TuningGuard) { } else if Utf8Path::new(CPUFREQ_BOOST).exists() { write_sysctl(guard, CPUFREQ_BOOST, "0", "turboboost (generic)"); } else { - println!(" Tuning: turboboost — skipped (not available on this platform)"); + println!(" Tuning: turboboost - skipped (not available on this platform)"); } } @@ -505,8 +505,8 @@ fn set_turbo(guard: &mut TuningGuard) { #[cfg(target_os = "linux")] fn restore(path: &Utf8Path, value: &str, label: &str) { match std::fs::write(path.as_str(), value) { - Ok(()) => println!(" Tuning: {label} — restored to {value}"), - Err(e) => println!(" Tuning: {label} — restore failed: {e}"), + Ok(()) => println!(" Tuning: {label} - restored to {value}"), + Err(e) => println!(" Tuning: {label} - restore failed: {e}"), } } @@ -518,7 +518,7 @@ fn restore(path: &Utf8Path, value: &str, label: &str) { #[cfg(not(target_os = "linux"))] pub struct TuningGuard; -/// No-op on non-Linux — returns a stub guard. +/// No-op on non-Linux - returns a stub guard. #[cfg(not(target_os = "linux"))] pub fn apply(_config: &TuningConfig) -> TuningGuard { TuningGuard @@ -643,7 +643,7 @@ mod tests { fs::write(&file_path, "changed").unwrap(); assert_eq!(fs::read_to_string(&file_path).unwrap(), "changed"); } - // Guard dropped — should restore + // Guard dropped - should restore assert_eq!(fs::read_to_string(&file_path).unwrap(), "original"); } diff --git a/plus/bencher_runner/src/up/mod.rs b/plus/bencher_runner/src/up/mod.rs index e8abafb718..35b1994308 100644 --- a/plus/bencher_runner/src/up/mod.rs +++ b/plus/bencher_runner/src/up/mod.rs @@ -102,7 +102,7 @@ impl Up { // Warn about host conditions that limit benchmark accuracy (Linux only) preflight::print_host_warnings(); - // Apply host tuning — guard restores settings on drop (no-op on non-Linux). + // Apply host tuning - guard restores settings on drop (no-op on non-Linux). // This must happen before CPU layout detection so that SMT changes // are reflected in the core count. let mut tuning_guard = crate::tuning::apply(&self.config.tuning);