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.

Sign up for GitHub

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

Jump to bottom

Add nixos cpufreq max min options #53041

Merged
merged 1 commit into from Jan 2, 2019
Merged

Add nixos cpufreq max min options #53041

merged 1 commit into from Jan 2, 2019

Conversation

cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Dec 29, 2018 •
edited

Motivation for this change

This adds a NixOS option for setting the CPU max and min frequencies with cpufreq. The two options that have been added are:

  • powerManagement.cpufreq.max
  • powerManagement.cpufreq.min

It also renames the powerManagement.cpuFreqGovernor option to powerManagement.cpufreq.governor. I could update this to be a module alias instead of a module rename if that would be preferred.

This updates the installer to use the new option name. It also updates the manual for with a note about the new name.

I think it makes the most sense to have powerManagement.cpufreq.{governor, max, min} instead of the options powerManagement.{cpuFreqGovernor, cpuFreqMax, cpuFreqMin}, but I'd be okay with either one.

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@cdepillabout cdepillabout force-pushed the add-nixos-cpufreq-max-min-options branch from 7b0028b to fe86dbb Compare December 29, 2018 15:43
infinisil
infinisil requested changes Dec 29, 2018
View reviewed changes
nixos/modules/tasks/cpu-freq.nix Outdated Show resolved Hide resolved
@infinisil
Copy link
Member

Nice, now the only thing left to do is squash the changes into 1 or 2 commits (one for the option rename and another for the rest), whatever you prefer

@cdepillabout cdepillabout force-pushed the add-nixos-cpufreq-max-min-options branch from 383fbfb to 475b945 Compare December 31, 2018 04:31
@cdepillabout
Copy link
Member Author

@infinisil I've squashed this into one commit: 475b945.

I thought this would be easier than trying to separate it into two commits.

This should be ready to be merged.

@infinisil
Copy link
Member

Oh actually, I haven't thought of this but this change will actually issue a warning for most NixOS users, and there's been some push against that, see #44107

So I think maybe an alias option is better.

@cdepillabout cdepillabout force-pushed the add-nixos-cpufreq-max-min-options branch from 475b945 to 49102f3 Compare December 31, 2018 16:32
@cdepillabout
Copy link
Member Author

cdepillabout commented Dec 31, 2018 •
edited

@infinisil I've changed the option from a rename to an alias in commit f6873e24132.

This PR should now be ready for a final review and merge.

@cdepillabout cdepillabout force-pushed the add-nixos-cpufreq-max-min-options branch 2 times, most recently from 4fde351 to f6873e2 Compare December 31, 2018 16:48
infinisil
infinisil reviewed Jan 1, 2019
View reviewed changes
nixos/doc/manual/release-notes/rl-1903.xml Outdated
The <option>powerManagement.cpuFreqGovernor</option> option has been
aliased to <option>powerManagement.cpufreq.governor</option>. On laptops,
<option>powerManagement.cpuFreqGovernor</option> is sometimes set in
<literal>/etc/nixos/hardware.nix</literal>, so you can rename it to the new
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/hardware.nix/hardware-configuration.nix

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I thought I had fixed that in a previous rebase.

Fixed in commit b0f10d2.

@cdepillabout
cpufreq: add option for setting the cpu max and min frequencies
b0f10d2 
This adds a NixOS option for setting the CPU max and min frequencies
with `cpufreq`.  The two options that have been added are:

- `powerManagement.cpufreq.max`
- `powerManagement.cpufreq.min`

It also adds an alias to the `powerManagement.cpuFreqGovernor` option as
`powerManagement.cpufreq.governor`.  This updates the installer to use
the new option name.  It also updates the manual with a note about
the new name.
@cdepillabout cdepillabout force-pushed the add-nixos-cpufreq-max-min-options branch from f6873e2 to b0f10d2 Compare January 1, 2019 10:18
infinisil
infinisil approved these changes Jan 1, 2019
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cdepillabout
Copy link
Member Author

@infinisil Thanks for working with me on this and giving your approval.

Could this be merged into master? I'd like to backport it to 18.09 after it is merged.

@infinisil infinisil merged commit 4731545 into NixOS:master Jan 2, 2019
@cdepillabout cdepillabout deleted the add-nixos-cpufreq-max-min-options branch January 2, 2019 08:18
@cdepillabout cdepillabout mentioned this pull request Jan 2, 2019
Merged
10 tasks
@infinisil
Copy link
Member

@cdepillabout Ah darn. Well I think the normal mkAliasOptionModule should propagate priorities as well, so this is like 1. But this might take a while to test properly with all existing aliases (does it break other things?), so I suggest going with 2 instead for now. We can add the alias back in later.

@cdepillabout
Copy link
Member Author

@infinisil Okay, sounds good. I'll go ahead and send two pull requests:

  1. A PR to temporarily remove the powerManagement.cpufreq.governor alias.
  2. A PR to change mkAliasOptionModule to propagate priorities.

If we are able to successfully merge 2, then I'll send a third PR adding back the powerManagement.cpufreq.governor alias.

@cdepillabout cdepillabout mentioned this pull request Jan 3, 2019
Merged
10 tasks
infinisil pushed a commit that referenced this pull request Jan 3, 2019
@cdepillabout
nixos/cpufreq: Remove the alias to set the cpu frequency governor
46ecec8 
This PR temporarily fixes the issue with PR 53041 as explained
here:

#53041 (comment)

The alias `powerManagement.cpufreq.governor` to
`powerManagement.cpuFreqGovernor` has been removed.
cdepillabout added a commit to cdepillabout/nixpkgs that referenced this pull request Jan 4, 2019
@cdepillabout
lib/modules: Add a function to create an option alias that respects t…
b81b3ad 
…he priority

This commit adds a function `mkAliasOptionModuleWithPriority`.  This
function will make an alias to an existing option and copy over the
priority.

This functionality is needed for PRs like NixOS#53041.  In that case
`nixos-generate-config` added an option to `hardware-configuration.nix`
with `mkDefault`.  That option was then changed and an alias created for
the old name.

The end user should be able to set the non-alias option in their
`configuration.nix` and have everything work correctly.  Without this
function, the priority for the option won't be copied over correctly
and the end-user will get a message saying they have the same option
set to two different values.
@cdepillabout cdepillabout mentioned this pull request Jan 4, 2019
Merged
10 tasks
@src-of-truth src-of-truth bot mentioned this pull request Oct 6, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@infinisil infinisil infinisil approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cdepillabout @infinisil @eadwu @GrahamcOfBorg