Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support running without tweakable scheduler if cfs profiles are disabled #91

Closed
ananace opened this issue Apr 30, 2023 · 2 comments · Fixed by #92
Closed

Support running without tweakable scheduler if cfs profiles are disabled #91

ananace opened this issue Apr 30, 2023 · 2 comments · Fixed by #92

Comments

@ananace
Copy link

ananace commented Apr 30, 2023

Currently, if the kernel is built without SCHED_DEBUG, then the entire functionality of the scheduler is prevented - even if the CFS tweaking functionality is never used, especially seeing as cfs-profiles are set as disabled in the default configuration.

I ended up writing a small - and mediocre - patch to be able to get a binary that I can deploy on systems both with and without CFS tuning paths; (Not a rust developer, and not going to submit it as a PR as it should really handle the error properly on startup if cfs-profiles are enabled)

diff --git a/daemon/src/main.rs b/daemon/src/main.rs
index 9823dcb..d9d7bb1 100644
--- a/daemon/src/main.rs
+++ b/daemon/src/main.rs
@@ -158,7 +158,7 @@ async fn daemon(
         return reload(connection).await;
     }
 
-    let service = &mut service::Service::new(owner, SchedPaths::new()?);
+    let service = &mut service::Service::new(owner, SchedPaths::new().ok());
     service.reload_configuration();
 
     let (tx, mut rx) = tokio::sync::mpsc::channel(4);
diff --git a/daemon/src/service.rs b/daemon/src/service.rs
index a252f68..8f0df58 100644
--- a/daemon/src/service.rs
+++ b/daemon/src/service.rs
@@ -12,7 +12,7 @@ use system76_scheduler_config::scheduler::Condition;
 
 pub struct Service<'owner> {
     pub config: crate::config::Config,
-    cfs_paths: SchedPaths,
+    cfs_paths: Option<SchedPaths>,
     foreground_processes: Vec<u32>,
     foreground: Option<u32>,
     pipewire_processes: Vec<u32>,
@@ -21,7 +21,7 @@ pub struct Service<'owner> {
 }
 
 impl<'owner> Service<'owner> {
-    pub fn new(owner: LCellOwner<'owner>, cfs_paths: SchedPaths) -> Self {
+    pub fn new(owner: LCellOwner<'owner>, cfs_paths: Option<SchedPaths>) -> Self {
         Self {
             config: crate::config::Config::default(),
             cfs_paths,
@@ -273,7 +273,7 @@ impl<'owner> Service<'owner> {
             return;
         }
 
-        crate::cfs::tweak(&self.cfs_paths, config);
+        crate::cfs::tweak(&self.cfs_paths.as_ref().unwrap(), config);
     }
 
     pub fn cfs_on_battery(&self, on_battery: bool) {
@mmstick
Copy link
Member

mmstick commented Apr 30, 2023

The general idea is correct but the unwrap will cause a panic if the debug paths are missing

mmstick added a commit that referenced this issue Apr 30, 2023
Fixes an early return of the service SchedPaths::new returns an error.

Closes #91
@mmstick mmstick mentioned this issue Apr 30, 2023
@ananace
Copy link
Author

ananace commented Apr 30, 2023

I was originally considering just doing an if self.cfs_paths.is_some() around the unwrap, but I wanted to keep the patch short and also actually in some way act on the inability to modify the scheduler.
A more correct patch would probably instead only attempt to unwrap the scheduler paths if the configuration file has cfs-profiles enabled - so that you get the correct error out of it. Instead of converting it to an optional and throwing the error out completely.

mmstick added a commit that referenced this issue May 1, 2023
Fixes an early return of the service SchedPaths::new returns an error.

Closes #91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants