From 00805d2808e529377b47f4cfb3017d37838e34ff Mon Sep 17 00:00:00 2001 From: NotAShelf Date: Thu, 15 May 2025 00:05:06 +0300 Subject: [PATCH] cpu: streamline governor setting; consolidate sharaed logic --- src/cli/debug.rs | 2 +- src/cpu.rs | 45 ++++++++++--- src/engine.rs | 167 +++++++++++++++++----------------------------- src/monitor.rs | 17 ++--- src/util/error.rs | 4 ++ 5 files changed, 108 insertions(+), 127 deletions(-) diff --git a/src/cli/debug.rs b/src/cli/debug.rs index c34565d..86371cf 100644 --- a/src/cli/debug.rs +++ b/src/cli/debug.rs @@ -246,7 +246,7 @@ fn check_and_print_sysfs_path(path: &str, description: &str) { fn is_systemd_service_active(service_name: &str) -> Result> { let output = Command::new("systemctl") .arg("is-active") - .arg(format!("{}.service", service_name)) + .arg(format!("{service_name}.service")) .stdout(Stdio::piped()) // capture stdout instead of letting it print .stderr(Stdio::null()) // redirect stderr to null .output()?; diff --git a/src/cpu.rs b/src/cpu.rs index e8d4bb6..f8dd3f4 100644 --- a/src/cpu.rs +++ b/src/cpu.rs @@ -1,6 +1,7 @@ use crate::core::{GovernorOverrideMode, TurboSetting}; use crate::util::error::ControlError; use core::str; +use std::path::PathBuf; use std::{fs, io, path::Path, string::ToString}; pub type Result = std::result::Result; @@ -8,12 +9,15 @@ pub type Result = std::result::Result; // Write a value to a sysfs file fn write_sysfs_value(path: impl AsRef, value: &str) -> Result<()> { let p = path.as_ref(); + fs::write(p, value).map_err(|e| { let error_msg = format!("Path: {:?}, Value: '{}', Error: {}", p.display(), value, e); - if e.kind() == io::ErrorKind::PermissionDenied { - ControlError::PermissionDenied(error_msg) - } else { - ControlError::WriteError(error_msg) + match e.kind() { + io::ErrorKind::PermissionDenied => ControlError::PermissionDenied(error_msg), + io::ErrorKind::NotFound => { + ControlError::PathMissing(format!("Path '{}' does not exist", p.display())) + } + _ => ControlError::WriteError(error_msg), } }) } @@ -82,7 +86,10 @@ pub fn set_governor(governor: &str, core_id: Option) -> Result<()> { // First, check if the requested governor is available on the system let available_governors = get_available_governors()?; - if !available_governors.contains(&governor.to_string()) { + if !available_governors + .iter() + .any(|g| g.eq_ignore_ascii_case(governor)) + { return Err(ControlError::InvalidGovernor(format!( "Governor '{}' is not available. Available governors: {}", governor, @@ -106,19 +113,35 @@ pub fn set_governor(governor: &str, core_id: Option) -> Result<()> { /// Retrieves the list of available CPU governors on the system pub fn get_available_governors() -> Result> { - // Check cpu0 for available governors - let path = "/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors"; - if !Path::new(path).exists() { + // Prefer cpu0, fall back to first cpu with cpufreq + let mut governor_path = + PathBuf::from("/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors"); + if !governor_path.exists() { + let core_count = get_logical_core_count()?; + let candidate = (0..core_count) + .map(|i| format!("/sys/devices/system/cpu/cpu{i}/cpufreq/scaling_available_governors")) + .find(|path| Path::new(path).exists()); + if let Some(path) = candidate { + governor_path = path.into(); + } + } + if !governor_path.exists() { return Err(ControlError::NotSupported( "Could not determine available governors".to_string(), )); } - let content = fs::read_to_string(path).map_err(|e| { + let content = fs::read_to_string(&governor_path).map_err(|e| { if e.kind() == io::ErrorKind::PermissionDenied { - ControlError::PermissionDenied(format!("Permission denied reading from {path}")) + ControlError::PermissionDenied(format!( + "Permission denied reading from {}", + governor_path.display() + )) } else { - ControlError::ReadError(format!("Failed to read from {path}: {e}")) + ControlError::ReadError(format!( + "Failed to read from {}: {e}", + governor_path.display() + )) } })?; diff --git a/src/engine.rs b/src/engine.rs index 4f94ff6..be49b66 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -4,6 +4,38 @@ use crate::cpu::{self}; use crate::util::error::{ControlError, EngineError}; use log::{debug, info, warn}; +/// Try applying a CPU feature and handle common error cases. Centralizes the where we +/// previously did: +/// 1. Try to apply a feature setting +/// 2. If not supported, log a warning and continue +/// 3. If other error, propagate the error +fn try_apply_feature( + feature_name: &str, + value_description: &str, + apply_fn: F, +) -> Result<(), EngineError> +where + F: FnOnce() -> Result, +{ + info!("Setting {feature_name} to '{value_description}'"); + + match apply_fn() { + Ok(_) => Ok(()), + Err(e) => { + if matches!(e, ControlError::NotSupported(_)) + || matches!(e, ControlError::PathMissing(_)) + { + warn!( + "{feature_name} setting is not supported on this system. Skipping {feature_name} configuration." + ); + Ok(()) + } else { + Err(EngineError::ControlError(e)) + } + } + } +} + /// Determines the appropriate CPU profile based on power status or forced mode, /// and applies the settings using functions from the `cpu` module. pub fn determine_and_apply_settings( @@ -18,17 +50,7 @@ pub fn determine_and_apply_settings( override_governor.trim() ); - // Check if the governor is available before applying - let available_governors = report.cpu_global.available_governors.clone(); - if !available_governors.contains(&override_governor.trim().to_string()) { - return Err(EngineError::ConfigurationError(format!( - "Governor '{}' from override file is not available on this system. Available governors: {}", - override_governor.trim(), - available_governors.join(", ") - ))); - } - - // Apply the override governor setting + // Apply the override governor setting - validation is handled by set_governor cpu::set_governor(override_governor.trim(), None)?; } @@ -64,15 +86,19 @@ pub fn determine_and_apply_settings( // Apply settings from selected_profile_config if let Some(governor) = &selected_profile_config.governor { - // Check if the governor is available before trying to set it - if report.cpu_global.available_governors.contains(governor) { - info!("Setting governor to '{governor}'"); - cpu::set_governor(governor, None)?; - } else { - let available = report.cpu_global.available_governors.join(", "); - warn!( - "Configured governor '{governor}' is not available on this system. Available governors: {available}. Skipping." - ); + info!("Setting governor to '{governor}'"); + // Let set_governor handle the validation + if let Err(e) = cpu::set_governor(governor, None) { + // If the governor is not available, log a warning + if matches!(e, ControlError::InvalidGovernor(_)) + || matches!(e, ControlError::NotSupported(_)) + { + warn!( + "Configured governor '{governor}' is not available on this system. Skipping." + ); + } else { + return Err(e.into()); + } } } @@ -84,110 +110,37 @@ pub fn determine_and_apply_settings( manage_auto_turbo(report, selected_profile_config)?; } _ => { - // Try to set turbo, but handle the error gracefully - if let Err(e) = cpu::set_turbo(turbo_setting) { - // If the feature is not supported, just log a warning instead of failing - if matches!(e, ControlError::NotSupported(_)) { - warn!( - "Turbo boost control is not supported on this system. Skipping turbo setting." - ); - } else { - // For other errors, propagate them - return Err(EngineError::ControlError(e)); - } - } + try_apply_feature("Turbo boost", &format!("{turbo_setting:?}"), || { + cpu::set_turbo(turbo_setting) + })?; } } } if let Some(epp) = &selected_profile_config.epp { - info!("Setting EPP to '{epp}'"); - // Try to set EPP, but handle the error gracefully - if let Err(e) = cpu::set_epp(epp, None) { - // If the feature is not supported, just log a warning instead of failing - if matches!(e, ControlError::NotSupported(_)) - || e.to_string().contains("No such file or directory") - { - warn!("EPP setting is not supported on this system. Skipping EPP configuration."); - } else { - return Err(EngineError::ControlError(e)); - } - } + try_apply_feature("EPP", epp, || cpu::set_epp(epp, None))?; } if let Some(epb) = &selected_profile_config.epb { - info!("Setting EPB to '{epb}'"); - // Try to set EPB, but handle the error gracefully - if let Err(e) = cpu::set_epb(epb, None) { - // If the feature is not supported, just log a warning instead of failing - if matches!(e, ControlError::NotSupported(_)) - || e.to_string().contains("No such file or directory") - { - warn!("EPB setting is not supported on this system. Skipping EPB configuration."); - } else { - return Err(EngineError::ControlError(e)); - } - } + try_apply_feature("EPB", epb, || cpu::set_epb(epb, None))?; } if let Some(min_freq) = selected_profile_config.min_freq_mhz { - info!("Setting min frequency to '{min_freq} MHz'"); - if let Err(e) = cpu::set_min_frequency(min_freq, None) { - // If the feature is not supported, just log a warning instead of failing - if matches!(e, ControlError::NotSupported(_)) - || e.to_string().contains("No such file or directory") - { - warn!( - "CPU frequency control is not supported on this system. Skipping min frequency setting." - ); - } else { - return Err(EngineError::ControlError(e)); - } - } + try_apply_feature("min frequency", &format!("{min_freq} MHz"), || { + cpu::set_min_frequency(min_freq, None) + })?; } if let Some(max_freq) = selected_profile_config.max_freq_mhz { - info!("Setting max frequency to '{max_freq} MHz'"); - if let Err(e) = cpu::set_max_frequency(max_freq, None) { - // If the feature is not supported, just log a warning instead of failing - if matches!(e, ControlError::NotSupported(_)) - || e.to_string().contains("No such file or directory") - { - warn!( - "CPU frequency control is not supported on this system. Skipping max frequency setting." - ); - } else { - return Err(EngineError::ControlError(e)); - } - } + try_apply_feature("max frequency", &format!("{max_freq} MHz"), || { + cpu::set_max_frequency(max_freq, None) + })?; } if let Some(profile) = &selected_profile_config.platform_profile { - info!("Setting platform profile to '{profile}'"); - // Try to get available platform profiles first to validate - match cpu::get_platform_profiles() { - Ok(available_profiles) => { - if available_profiles.contains(profile) { - cpu::set_platform_profile(profile)?; - } else { - warn!( - "Platform profile '{}' is not available. Available profiles: {}. Skipping.", - profile, - available_profiles.join(", ") - ); - } - } - Err(e) => { - // If platform profiles are not supported, log a warning and continue - if matches!(e, ControlError::NotSupported(_)) { - warn!( - "Platform profile control is not supported on this system. Skipping profile setting." - ); - } else { - return Err(EngineError::ControlError(e)); - } - } - } + try_apply_feature("platform profile", profile, || { + cpu::set_platform_profile(profile) + })?; } debug!("Profile settings applied successfully."); diff --git a/src/monitor.rs b/src/monitor.rs index 91997a1..a3b6b83 100644 --- a/src/monitor.rs +++ b/src/monitor.rs @@ -408,14 +408,15 @@ pub fn get_cpu_global_info(cpu_cores: &[CpuCoreInfo]) -> CpuGlobalInfo { let mut cpufreq_base_path_buf = PathBuf::from("/sys/devices/system/cpu/cpu0/cpufreq/"); if !cpufreq_base_path_buf.exists() { - // Fallback: find first available CPU with cpufreq - for i in 1..=get_logical_core_count().unwrap_or(1) { - let test_path = format!("/sys/devices/system/cpu/cpu{}/cpufreq/", i - 1); - let test_path_buf = PathBuf::from(&test_path); - if test_path_buf.exists() { - cpufreq_base_path_buf = test_path_buf; - break; - } + let core_count = get_logical_core_count().unwrap_or_else(|e| { + eprintln!("Warning: {e}"); + 0 + }); + let path = (0..core_count) + .map(|i| PathBuf::from(format!("/sys/devices/system/cpu/cpu{i}/cpufreq/"))) + .find(|path| path.exists()); + if let Some(test_path_buf) = path { + cpufreq_base_path_buf = test_path_buf; } } diff --git a/src/util/error.rs b/src/util/error.rs index 7b92b8f..4f9391f 100644 --- a/src/util/error.rs +++ b/src/util/error.rs @@ -11,6 +11,7 @@ pub enum ControlError { InvalidGovernor(String), ParseError(String), ReadError(String), + PathMissing(String), } impl From for ControlError { @@ -47,6 +48,9 @@ impl std::fmt::Display for ControlError { Self::ReadError(s) => { write!(f, "Failed to read sysfs path: {s}") } + Self::PathMissing(s) => { + write!(f, "Path missing: {s}") + } } } }