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 c499f086..4b93f7ed 100644 --- a/src/uu/chcpu/src/sysfs.rs +++ b/src/uu/chcpu/src/sysfs.rs @@ -104,18 +104,34 @@ 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") + } + + /// 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)); + } }