undervolt: init at 0.2.8 #44919

Vodurden · 2018-08-12T02:14:06Z

Motivation for this change

Undervolt is a tool that lets you configure the voltage supplied to Intel CPUs on Linux. This is particularly useful for reducing the thermal footprint of a laptop.

This change almost fits CONTRIBUTING.md but I'm not sure who the maintainer should be. Is that something I should add myself to?

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

samueldr · 2018-08-12T02:19:37Z

@GrahamcOfBorg build undervolt

Unless I'm missing something, the code itself in a whole looks fine. I made some notes as a review.

GrahamcOfBorg · 2018-08-12T02:27:27Z

Success on aarch64-linux (full log)

Attempted: undervolt

Partial log (click to expand)

unconvert_rounded_offset (undervolt)
Doctest: undervolt.unconvert_rounded_offset ... ok
unpack_offset (undervolt)
Doctest: undervolt.unpack_offset ... ok

----------------------------------------------------------------------
Ran 5 tests in 0.045s

OK
/nix/store/mcihi7gdj6srcr05hbdsm96gahfdhy3p-undervolt-0.2.7

GrahamcOfBorg · 2018-08-12T02:27:28Z

No attempt on x86_64-darwin (full log)

The following builds were skipped because they don't evaluate on x86_64-darwin: undervolt

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.

GrahamcOfBorg · 2018-08-12T02:28:12Z

Success on x86_64-linux (full log)

Attempted: undervolt

Partial log (click to expand)

unconvert_rounded_offset (undervolt)
Doctest: undervolt.unconvert_rounded_offset ... ok
unpack_offset (undervolt)
Doctest: undervolt.unpack_offset ... ok

----------------------------------------------------------------------
Ran 5 tests in 0.054s

OK
/nix/store/agh9is4wvrqffgw4r7cc5dd7hvks1mzs-undervolt-0.2.7

samueldr

As I said, looks fine. Not approving because of my nitpicks, but not outright requesting changes in case another reviewer feels I'm overly zealous.

samueldr · 2018-08-12T02:30:35Z

pkgs/os-specific/linux/undervolt/default.nix

+      target (CPU will throttle when this temperature is reached).
+    '';
+    license = licenses.gpl2;
+    platforms = platforms.linux;


While it builds on ARM, I'm pretty sure it won't work as it's for intel hardware. Furthermore, I would be surprised if it did anything for 32-bit processors. Might be better to only target x86_64

platforms = [ "x86_64-linux" ];

samueldr · 2018-08-12T02:34:37Z

nixos/modules/services/hardware/undervolt.nix

+        undervolt derivation to use.
+      '';
+    };
+


~~(In the following options) it might be better to drop the Offset suffix from the option names, pretty redundant and they're all offsets, except for enable and package.~~ Disregard that as there are more than voltage offsets.

It may be worthwhile to look into supporting other options like --temp

Good idea! I've added temp support and tested that it works on my NixOS machine

eadwu · 2018-08-17T22:20:13Z

Would be better to bump the version to 0.2.8. At least for my dear xps (cough georgewhewell/undervolt#25).

sha256 = "0crkqc5zq0gpyg031hfwdxymfc2gc1h8b6m0axzlh7gvnxlf5hra";

Vodurden · 2018-08-18T02:35:44Z

Done! I tested 0.2.8 on my machine and it seems to be working fine. Let me know if there are any other issues :)

Edit: I've also rebased this branch against latest master again so hopefully there shouldn't be any conflicts if we feel it's ready to merge

Vodurden · 2018-08-19T05:35:01Z

@samueldr, @eadwu: Just checking in to see if there's anything else I need to do on this PR to get it merged. Please let me know if there are any other blockers and I'll try to resolve them as soon as possible.

Thanks in advance :)

samueldr

Looks good, can't actually test it, but seeing there are two users already is good.

Just re-reading the code and saw some stuff, do comment/resolve as needed and I think then it'll be good to merge.

samueldr · 2018-08-19T17:18:21Z

nixos/modules/services/hardware/undervolt.nix

+        ExecStart = ''
+          ${pkgs.undervolt}/bin/undervolt -v \
+            ${optionalString (cfg.coreOffset != null) "--core ${cfg.coreOffset}"} \
+            ${optionalString (cfg.coreOffset != null) "--cache ${cfg.coreOffset}"} \


Is it wanted that --cache uses cfg.coreOffset?

Yes it is:

Core or Cache offsets have no effect. It is not possible to set different offsets for CPU Core and Cache. The CPU will take the smaller of the two offsets, and apply that to both CPU and Cache. A warning message will be displayed if you attempt to set different offsets.

Please add a comment explaining so there's nobody trying to helpfully "fix" what's not to be fixed.

samueldr · 2018-08-19T17:20:55Z

pkgs/os-specific/linux/undervolt/default.nix

@@ -0,0 +1,29 @@
+{ stdenv, fetchFromGitHub, python3Packages }:
+
+with python3Packages;


it's generally better to use with only with flat lists or sets; makes it easier to understand what the identifiers refer to.

In your particular case, removing the with statement, then changing buildPythonApplication to python3Packages.buildPythonApplication works fine.

(Using with stdenv.lib in meta is fine)

Vodurden · 2018-08-20T10:36:02Z

@samueldr: Thanks for the review! I've implemented your recommendations.

I also noticed that the logging in journalctl -u undervolt was very verbose as I was always setting the -v flag so I've moved that behind an option. I've tested this new setting and everything seems fine.

Finally I've rebased this against master so hopefully there shouldn't be any conflicts if we feel it's ready to merge.

Thanks again and please let me know if there's anything else preventing this from being merged :)

We want to be able to configure persistent undervolting in the NixOS configuration

Vodurden · 2018-08-22T08:00:30Z

@samueldr: Thanks for approving the changes!

I'm just wondering if there's anything else that needs to be done to merge this branch? I don't have permission to merge so I'm not sure if I should just wait for someone else to click the merge button or if I need to make any changes/reach out to anyone to get it sorted out.

Atemu · 2020-02-19T15:10:20Z

Hey @Vodurden, is there a specific reason the undervolt needs to be applied every 2min?

The service activation is spamming my journal.

Vodurden · 2020-02-20T11:20:50Z

Hi @Atemu, I don't think there is any reason it needs to be 2 minutes. I copied the duration from the README of undervolt: https://github.com/georgewhewell/undervolt

Re-reading the README it looks like they actually use a 30 second interval. I'm not sure why it needs to be so short but at the time I wrote this PR I didn't dig into the reason why. I wouldn't be surprised if just setting the value on boot works fine but you'd need to test it to be sure :)

Atemu · 2020-02-20T11:32:26Z

Thanks!

I found at least one while hacking around on it, the voltages are reset after resume.

eadwu · 2020-02-20T14:09:15Z

Yeah that's the reason why there's a timer, I use an alternate version.

    systemd.services.undervolt = {
      after = [ "suspend.target" "hibernate.target" "hybrid-sleep.target" ];
      wantedBy = [ "suspend.target" "hibernate.target" "hybrid-sleep.target" "multi-user.target" ];

      description = "Intel Undervolt";
      serviceConfig.ExecStart = undervoltCmd;
    };

    services.udev.extraRules = ''
      # Apply an undervolt for Intel CPUs
      ACTION=="change", SUBSYSTEM=="power_supply", ATTR{type}=="Mains", ATTR{online}=="0", RUN+="${undervoltCmd}"
      ACTION=="change", SUBSYSTEM=="power_supply", ATTR{type}=="Mains", ATTR{online}=="1", RUN+="${undervoltCmd}"
    '';

Atemu · 2020-02-21T10:23:56Z

Thanks! I experimented around a bit with this and found that the first declaration can be condensed down into

  systemd.services.undervolt.wantedBy = [ "post-resume.target" "multi-user.target" ];
  systemd.services.undervolt.after = [ "post-resume.target" ];

What exactly does the second one do? I'm not too familiar with udev but it looks like it's supposed to deal with power supply changes?
I couldn't get the undervolt to go away when (dis-)connecting the charger though.

eadwu · 2020-02-21T14:08:02Z

It may be a problem with certain CPUs or computers, anyway if you need more help create a new issue instead of commenting here.

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) labels Aug 12, 2018

Vodurden force-pushed the init-undervolt branch from 9d6113e to 4e8a2ad Compare August 12, 2018 02:19

eadwu mentioned this pull request Aug 12, 2018

undervolt: init at 0.2.5 #43597

Closed

9 tasks

GrahamcOfBorg added 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Aug 12, 2018

samueldr reviewed Aug 12, 2018

View reviewed changes

Vodurden force-pushed the init-undervolt branch 2 times, most recently from 8f5dede to ab8c757 Compare August 12, 2018 04:13

Vodurden force-pushed the init-undervolt branch from ab8c757 to 86a183f Compare August 18, 2018 02:35

Vodurden force-pushed the init-undervolt branch from 86a183f to 3affc59 Compare August 18, 2018 02:36

Vodurden changed the title ~~undervolt: init at 0.2.7~~ undervolt: init at 0.2.8 Aug 19, 2018

samueldr requested changes Aug 19, 2018

View reviewed changes

Vodurden force-pushed the init-undervolt branch from 3affc59 to 8f30703 Compare August 20, 2018 10:33

Vodurden added 2 commits August 20, 2018 21:01

undervolt: init at 0.2.8

e3cbeef

nixos/undervolt: adding undervolt module

4142020

We want to be able to configure persistent undervolting in the NixOS configuration

Vodurden force-pushed the init-undervolt branch from 8f30703 to 4142020 Compare August 20, 2018 11:01

samueldr approved these changes Aug 20, 2018

View reviewed changes

samueldr merged commit 05310e3 into NixOS:master Aug 22, 2018

Janik-Haag added the 12. first-time contribution label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

undervolt: init at 0.2.8 #44919

undervolt: init at 0.2.8 #44919

Vodurden commented Aug 12, 2018 •

edited

samueldr commented Aug 12, 2018 •

edited

GrahamcOfBorg commented Aug 12, 2018

GrahamcOfBorg commented Aug 12, 2018

GrahamcOfBorg commented Aug 12, 2018

samueldr left a comment

samueldr Aug 12, 2018

samueldr Aug 12, 2018

Vodurden Aug 12, 2018

eadwu commented Aug 17, 2018 •

edited

Vodurden commented Aug 18, 2018 •

edited

Vodurden commented Aug 19, 2018

samueldr left a comment

samueldr Aug 19, 2018

samueldr Aug 19, 2018

Vodurden commented Aug 20, 2018

Vodurden commented Aug 22, 2018

Atemu commented Feb 19, 2020

Vodurden commented Feb 20, 2020

Atemu commented Feb 20, 2020

eadwu commented Feb 20, 2020

Atemu commented Feb 21, 2020

eadwu commented Feb 21, 2020

		@@ -0,0 +1,29 @@
		{ stdenv, fetchFromGitHub, python3Packages }:

		with python3Packages;

undervolt: init at 0.2.8 #44919

undervolt: init at 0.2.8 #44919

Conversation

Vodurden commented Aug 12, 2018 • edited

Motivation for this change

Things done

samueldr commented Aug 12, 2018 • edited

GrahamcOfBorg commented Aug 12, 2018

GrahamcOfBorg commented Aug 12, 2018

GrahamcOfBorg commented Aug 12, 2018

samueldr left a comment

Choose a reason for hiding this comment

samueldr Aug 12, 2018

Choose a reason for hiding this comment

samueldr Aug 12, 2018

Choose a reason for hiding this comment

Vodurden Aug 12, 2018

Choose a reason for hiding this comment

eadwu commented Aug 17, 2018 • edited

Vodurden commented Aug 18, 2018 • edited

Vodurden commented Aug 19, 2018

samueldr left a comment

Choose a reason for hiding this comment

samueldr Aug 19, 2018

Choose a reason for hiding this comment

samueldr Aug 19, 2018

Choose a reason for hiding this comment

Vodurden commented Aug 20, 2018

Vodurden commented Aug 22, 2018

Atemu commented Feb 19, 2020

Vodurden commented Feb 20, 2020

Atemu commented Feb 20, 2020

eadwu commented Feb 20, 2020

Atemu commented Feb 21, 2020

eadwu commented Feb 21, 2020

Vodurden commented Aug 12, 2018 •

edited

samueldr commented Aug 12, 2018 •

edited

eadwu commented Aug 17, 2018 •

edited

Vodurden commented Aug 18, 2018 •

edited