chcpu: bound the cpu list walk at the highest possible CPU

src/uu/chcpu/src/chcpu.rs

@@ -204,23 +204,50 @@ impl fmt::Display for DispatchMode {
 pub(crate) struct CpuList(RangeInclusiveSet<usize>);
 
 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<usize> {
+        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<usize>,
+        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)
     });
 

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<String>, 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,

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<CpuList, ChCpuError> {
+    fn cpu_list(&self, name: impl AsRef<Path>) -> Result<CpuList, ChCpuError> {
+        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<CpuList, ChCpuError> {
+        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: <https://docs.kernel.org/core-api/cpu_hotplug.html>.
+    ///
+    /// `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<usize> {
+        self.cpu_list("possible")
+            .ok()
+            .and_then(|list| list.max_index())
+    }
+
     pub(crate) fn cpu_dir_path(&self, cpu_index: usize) -> Result<PathBuf, ChCpuError> {
         let dir_name = PathBuf::from(format!("cpu{cpu_index}"));
 

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<usize> {
-        (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<usize> {
+        let list = std::fs::read_to_string("/sys/devices/system/cpu/possible").ok()?;
+        let mut max: Option<usize> = 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));
+    }
 }