From 942850cd85590508bc73ef56ec264b117438db86 Mon Sep 17 00:00:00 2001 From: mmc Date: Mon, 24 Aug 2026 20:43:41 -0500 Subject: [PATCH 1/2] chcpu: read cpu lists from sysfs through one helper `enabled_cpu_list` inlined the open, the read and the error path for one attribute name. A second cpu-list attribute is about to be read the same way, and duplicating eleven lines to change one string is the wrong shape. The helper takes `impl AsRef`, like every other accessor on `SysFSCpu`. `enabled_cpu_list` is its only caller for now, so behavior is unchanged. --- src/uu/chcpu/src/sysfs.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/uu/chcpu/src/sysfs.rs b/src/uu/chcpu/src/sysfs.rs index c499f086..f55f2122 100644 --- a/src/uu/chcpu/src/sysfs.rs +++ b/src/uu/chcpu/src/sysfs.rs @@ -104,18 +104,21 @@ impl SysFSCpu { .map_err(|err| ChCpuError::io1("failed to write file", Self::inner_path(name), err)) } - pub(crate) fn enabled_cpu_list(&self) -> Result { + fn cpu_list(&self, name: impl AsRef) -> Result { + let name = name.as_ref(); let mut buffer = Vec::default(); - self.open_inner("online", libc::O_RDONLY | libc::O_CLOEXEC)? + self.open_inner(name, libc::O_RDONLY | libc::O_CLOEXEC)? .read_to_end(&mut buffer) - .map_err(|err| { - ChCpuError::io1("failed to read file", Self::inner_path("online"), err) - })?; + .map_err(|err| ChCpuError::io1("failed to read file", Self::inner_path(name), err))?; CpuList::try_from(buffer.as_slice()) } + pub(crate) fn enabled_cpu_list(&self) -> Result { + self.cpu_list("online") + } + pub(crate) fn cpu_dir_path(&self, cpu_index: usize) -> Result { let dir_name = PathBuf::from(format!("cpu{cpu_index}")); From 1192deb3470593af32c8394419c34351b5abe944 Mon Sep 17 00:00:00 2001 From: mmc Date: Mon, 24 Aug 2026 20:43:41 -0500 Subject: [PATCH 2/2] chcpu: bound the cpu list walk at the highest possible CPU A cpu-list range was bounded only by the integer type, so `CpuList::run` stepped through every index it named, one faccessat each, and printed one stderr line per index that did not exist. Measured at roughly 241,000 indices per second, which extrapolates to about five hours for `--enable 0-4294967295`; `--disable` took the same path and, as root, offlined the CPUs that do exist before grinding through the ones that do not. The walk now stops at the highest index in /sys/devices/system/cpu/possible. That mask is fixed during boot discovery, so no index above it can be brought online for the life of the boot, not even by hot-add, and `possible_cpus=` already pre-allocates slots for CPUs that are hot-added later: - https://docs.kernel.org/core-api/cpu_hotplug.html - https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-devices-system-cpu Bounding there therefore cannot refuse an operation that could have succeeded. Only the walk is bounded, not the parse, so which argv are accepted is unchanged. Indices above the bound are reported one range at a time instead of one index at a time; a single index keeps its existing wording, so the common `chcpu -e 24` diagnostic is untouched. Exit status and stdout are unchanged for every argv: the collapsed remainder still counts as a failure, and an index above the bound never produced stdout to begin with. The bound is taken inside `walk_cpu_list` rather than passed to `run`, so no operation can be added that omits it: an omitted bound is the multi-hour walk the bound exists to prevent, with no compile error to catch it. Where the attribute cannot be read the walk stays unbounded, as before. Refusing the operation instead would let one missing optional attribute stop a CPU that does exist from being enabled. Three tests cover the bound; all walk only indices that are absent or already online, so they need no privileges and change no CPU state, and each skips itself where the bound cannot be read rather than walking unbounded. The two absent indices in the existing multi-element test are no longer adjacent, because a cpu-list coalesces touching ranges and each range is now reported once. --- src/uu/chcpu/src/chcpu.rs | 62 ++++++++++++++--- src/uu/chcpu/src/errors.rs | 14 ++++ src/uu/chcpu/src/sysfs.rs | 13 ++++ tests/by-util/test_chcpu.rs | 130 +++++++++++++++++++++++++++++++++--- 4 files changed, 197 insertions(+), 22 deletions(-) diff --git a/src/uu/chcpu/src/chcpu.rs b/src/uu/chcpu/src/chcpu.rs index 4e9c9425..21dc0dfc 100644 --- a/src/uu/chcpu/src/chcpu.rs +++ b/src/uu/chcpu/src/chcpu.rs @@ -204,23 +204,50 @@ impl fmt::Display for DispatchMode { pub(crate) struct CpuList(RangeInclusiveSet); impl CpuList { + /// The highest index in the list. `RangeInclusiveSet` keeps its ranges + /// coalesced and ordered, so the last one holds it. + pub(crate) fn max_index(&self) -> Option { + self.0.last().map(|range| *range.end()) + } + /// A failure on one CPU must not stop the remaining ones, so failures are /// reported here and reflected in the exit code instead of being returned: /// returning one would let `uucore` print it a second time. - fn run(&self, f: &mut dyn FnMut(usize) -> Result<(), ChCpuError>) { - use std::ops::RangeInclusive; - + /// + /// `max_cpu_index` bounds the walk. A cpu-list range is only as wide as the + /// integer type, so without it `--enable 0-4294967295` spends hours calling + /// `f` on indices no kernel can have. Indices above the bound cannot exist, so + /// they are reported one range at a time rather than one index at a time. + /// `None` walks everything, as it did before the bound existed; call through + /// `walk_cpu_list` rather than passing it, so the bound cannot be dropped. + fn run( + &self, + max_cpu_index: Option, + f: &mut dyn FnMut(usize) -> Result<(), ChCpuError>, + ) { let mut success_occurred = false; let mut failure_occurred = false; - for cpu_index in self.0.iter().flat_map(RangeInclusive::to_owned) { - match f(cpu_index) { - Ok(()) => success_occurred = true, - Err(err) => { - uucore::show!(err); - failure_occurred = true; + for range in self.0.iter() { + let (first, last) = (*range.start(), *range.end()); + let walked_last = max_cpu_index.map_or(last, |max| max.min(last)); + + // Empty when the whole range sits above the bound. + for cpu_index in first..=walked_last { + match f(cpu_index) { + Ok(()) => success_occurred = true, + Err(err) => { + uucore::show!(err); + failure_occurred = true; + } } } + + if walked_last < last { + // The comparison guarantees the increment stays in range. + uucore::show!(ChCpuError::absent_cpus(first.max(walked_last + 1), last)); + failure_occurred = true; + } } if success_occurred && failure_occurred { @@ -282,13 +309,26 @@ impl FromStr for CpuList { } } +/// Walks `cpu_list`, bounded by what the machine can have. The bound is taken here +/// rather than passed in so that no operation can be added that omits it: a walk +/// given no bound steps through every index the integer type allows, which is the +/// hours-long walk the bound exists to prevent. +#[cfg(unix)] +fn walk_cpu_list( + sysfs_cpu: &sysfs::SysFSCpu, + cpu_list: &CpuList, + f: &mut dyn FnMut(usize) -> Result<(), ChCpuError>, +) { + cpu_list.run(sysfs_cpu.max_possible_cpu_index(), f); +} + #[cfg(unix)] fn enable_cpu(cpu_list: &CpuList, enable: bool) -> Result<(), ChCpuError> { let sysfs_cpu = sysfs::SysFSCpu::open()?; let mut enabled_cpu_list = sysfs_cpu.enabled_cpu_list().ok(); - cpu_list.run(&mut move |cpu_index| { + walk_cpu_list(&sysfs_cpu, cpu_list, &mut |cpu_index| { sysfs_cpu.enable_cpu(enabled_cpu_list.as_mut(), cpu_index, enable) }); @@ -306,7 +346,7 @@ fn configure_cpu(cpu_list: &CpuList, configure: bool) -> Result<(), ChCpuError> let enabled_cpu_list = sysfs_cpu.enabled_cpu_list().ok(); - cpu_list.run(&mut move |cpu_index| { + walk_cpu_list(&sysfs_cpu, cpu_list, &mut |cpu_index| { sysfs_cpu.configure_cpu(enabled_cpu_list.as_ref(), cpu_index, configure) }); diff --git a/src/uu/chcpu/src/errors.rs b/src/uu/chcpu/src/errors.rs index d26e7537..f2e2a31e 100644 --- a/src/uu/chcpu/src/errors.rs +++ b/src/uu/chcpu/src/errors.rs @@ -31,6 +31,9 @@ pub enum ChCpuError { #[error("CPU {0} does not exist")] InvalidCpuIndex(usize), + #[error("CPUs {0}-{1} do not exist")] + InvalidCpuIndexRange(usize, usize), + #[error("{0}: {1}")] IO0(String, std::io::Error), @@ -48,6 +51,16 @@ pub enum ChCpuError { } impl ChCpuError { + /// A run of nonexistent CPU indices, reported once instead of once per index. + /// A single index keeps the wording it has always had. + pub(crate) fn absent_cpus(first: usize, last: usize) -> Self { + if first == last { + Self::InvalidCpuIndex(first) + } else { + Self::InvalidCpuIndexRange(first, last) + } + } + pub(crate) fn io0(message: impl Into, error: std::io::Error) -> Self { Self::IO0(message.into(), error) } @@ -74,6 +87,7 @@ impl ChCpuError { | Self::CpuSpecNotPositiveInteger | Self::EmptyCpuList | Self::InvalidCpuIndex(_) + | Self::InvalidCpuIndexRange(..) | Self::OneCpuIsEnabled | Self::NotInteger(_) | Self::SetCpuDispatchUnsupported => self, diff --git a/src/uu/chcpu/src/sysfs.rs b/src/uu/chcpu/src/sysfs.rs index f55f2122..4b93f7ed 100644 --- a/src/uu/chcpu/src/sysfs.rs +++ b/src/uu/chcpu/src/sysfs.rs @@ -119,6 +119,19 @@ impl SysFSCpu { self.cpu_list("online") } + /// The highest CPU index the kernel can ever bring online. `cpu_possible_mask` + /// is fixed during boot discovery, so nothing above it can appear later, not + /// even by hot-add: . + /// + /// `None` where the attribute cannot be read, which leaves the walk unbounded + /// rather than refusing the operation: one missing optional attribute must not + /// stop a CPU that does exist from being enabled. + pub(crate) fn max_possible_cpu_index(&self) -> Option { + self.cpu_list("possible") + .ok() + .and_then(|list| list.max_index()) + } + pub(crate) fn cpu_dir_path(&self, cpu_index: usize) -> Result { let dir_name = PathBuf::from(format!("cpu{cpu_index}")); diff --git a/tests/by-util/test_chcpu.rs b/tests/by-util/test_chcpu.rs index 4c5913ae..e596898f 100644 --- a/tests/by-util/test_chcpu.rs +++ b/tests/by-util/test_chcpu.rs @@ -72,26 +72,66 @@ mod linux { use uutests::new_ucmd; /// CPU indices no kernel can have: `CONFIG_NR_CPUS` is orders of magnitude below - /// these, so `/sys/devices/system/cpu/cpu9999[89]` never exists and `chcpu` - /// rejects them before it would write anything. - const ABSENT_CPU: &str = "99999"; - const ABSENT_CPU_2: &str = "99998"; + /// these, so `/sys/devices/system/cpu/cpu9999[789]` never exists and `chcpu` + /// rejects them before it would write anything. The two named here are + /// deliberately not adjacent: a cpu-list coalesces touching ranges and reports + /// each resulting range once, so an adjacent pair yields one diagnostic rather + /// than two. `ABSENT_CPU - 1` supplies that adjacent case where it is wanted. + const ABSENT_CPU: usize = 99999; + const ABSENT_CPU_2: usize = 99997; + + /// Whether `cpuN` exposes an `online` attribute that reads `1`. `cpu0` commonly + /// has no such attribute, so a CPU index cannot simply be assumed. + fn cpu_is_online(index: usize) -> bool { + std::fs::read_to_string(format!("/sys/devices/system/cpu/cpu{index}/online")) + .is_ok_and(|state| state.trim() == "1") + } /// First CPU exposing an `online` attribute that reads `1`, or `None` where no - /// CPU is hot-pluggable. `cpu0` commonly has no such attribute, so a CPU index - /// cannot simply be assumed. + /// CPU is hot-pluggable. Not simply the first online CPU: `cpu0` is online on + /// every running system yet commonly has no such attribute, so a CPU index + /// cannot be assumed. fn first_online_cpu() -> Option { - (0..1024).find(|index| { - std::fs::read_to_string(format!("/sys/devices/system/cpu/cpu{index}/online")) - .is_ok_and(|state| state.trim() == "1") - }) + (0..1024).find(|index| cpu_is_online(*index)) + } + + /// Highest index in `/sys/devices/system/cpu/possible`, which is where the walk + /// stops. `None` unless every element parses, because the binary parses that + /// file all-or-nothing: a helper that salvaged a bound from a list the binary + /// rejects would report a stop the binary does not have, and the tests guarded + /// on it would then walk an unbounded range. + fn max_possible_cpu() -> Option { + let list = std::fs::read_to_string("/sys/devices/system/cpu/possible").ok()?; + let mut max: Option = None; + + for element in list.trim().split(',') { + let (first, last) = element.split_once('-').unwrap_or((element, element)); + let (first, last): (usize, usize) = + (first.trim().parse().ok()?, last.trim().parse().ok()?); + + if first > last { + return None; + } + + max = Some(max.map_or(last, |max| max.max(last))); + } + + max + } + + /// Whether the walk stops below `index`. Indices above the stop cannot exist and + /// are collapsed into one diagnostic instead of probed one at a time; where the + /// stop is unknown the walk is unbounded, and neither the collapse nor the + /// constant running time it buys holds. + fn walk_stops_below(index: usize) -> bool { + max_possible_cpu().is_some_and(|max| max < index) } #[test] fn test_absent_cpu_is_reported_once() { new_ucmd!() .arg("--enable") - .arg(ABSENT_CPU) + .arg(ABSENT_CPU.to_string()) .fails_with_code(1) .stderr_only(format!("chcpu: CPU {ABSENT_CPU} does not exist\n")); } @@ -125,4 +165,72 @@ mod linux { .stdout_is(format!("CPU {cpu} is already enabled\n")) .stderr_is(format!("chcpu: CPU {ABSENT_CPU} does not exist\n")); } + + /// A cpu-list range is bounded only by the integer type, so walking it one index + /// at a time took about five hours for this argv. Indices above the highest + /// possible CPU cannot exist and are reported as one range, which makes it + /// constant time. No index is walked, so no CPU state can change even as root. + #[test] + fn test_absent_cpu_range_is_reported_once() { + if !walk_stops_below(ABSENT_CPU) { + eprintln!( + "skipping test_absent_cpu_range_is_reported_once: the walk is unbounded here, \ + so this argv would run for hours" + ); + return; + } + + new_ucmd!() + .arg("--enable") + .arg(format!("{ABSENT_CPU}-4294967295")) + .fails_with_code(1) + .stderr_only(format!( + "chcpu: CPUs {ABSENT_CPU}-4294967295 do not exist\n" + )); + } + + /// Adjacent elements coalesce into one range before the walk, so a pair above the + /// bound takes the plural wording and names both, where the non-adjacent pair in + /// [`test_every_absent_cpu_is_reported_once`] still yields two lines. Two indices + /// is the narrowest range that is not reported as a single CPU. + #[test] + fn test_adjacent_absent_cpus_are_reported_as_one_range() { + let first = ABSENT_CPU - 1; + + if !walk_stops_below(first) { + eprintln!( + "skipping test_adjacent_absent_cpus_are_reported_as_one_range: the walk is \ + unbounded here, so each index is probed and reported separately" + ); + return; + } + + new_ucmd!() + .arg("--enable") + .arg(format!("{first},{ABSENT_CPU}")) + .fails_with_code(1) + .stderr_only(format!("chcpu: CPUs {first}-{ABSENT_CPU} do not exist\n")); + } + + /// A range straddling the bound walks the part at or below it and collapses the + /// rest, so the first index named is the one just past the bound. Runs only + /// where the boundary CPU is already online, so `enable_cpu` returns before + /// writing and no CPU state changes even as root. + #[test] + fn test_range_spanning_the_bound_reports_the_remainder_once() { + let Some(max) = max_possible_cpu().filter(|index| cpu_is_online(*index)) else { + eprintln!( + "skipping test_range_spanning_the_bound_reports_the_remainder_once: \ + the highest possible CPU is unknown or not online" + ); + return; + }; + + new_ucmd!() + .arg("--enable") + .arg(format!("{max}-4294967295")) + .fails_with_code(64) + .stdout_is(format!("CPU {max} is already enabled\n")) + .stderr_is(format!("chcpu: CPUs {}-4294967295 do not exist\n", max + 1)); + } }