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

nvme: Add support for Autonomous Power State Transition #1444

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eseipi
Copy link

@eseipi eseipi commented Oct 1, 2024

APST is an optional NVMe power-saving feature that allows devices to autonomously enter higher non-operational power states after a certain amount of idle time, reducing the controller's overall power consumption.

On my laptop with a Toshiba XG5, the ability to adjust APST settings allowed me to nearly double the battery life (from about 6 to 11 hours) and reduce the temperature of both the CPU and the controller by about 20°C.

At the moment, the code is very simple: the driver checks two new kenv variables (hw.nvme.apst_enable and hw.nvme.apst_data) at initialization and submit the Set Features command with those values, if any (for more info see 5.1.25.1.6 of NVM Express Base Specification rev. 2.1). If only apst_enable is set, the transition data is retrieved from the controller itself, but AFAIK the spec doesn't say anything about its presence being mandatory. Right now I only have two devices, and both have sane defaults.

Linux is doing the whole thing differently: each time it generates the data based on available power states, some set of user-provided parameters such as latency and tolerance, and a bit of heuristic. I used similar approach at first, but after afedorov@ feedback and a little more thought, moved all that kind of stuff to userspace (D47381). In either case, any further comments/ideas on this matter would be greatly appreciated.

@eseipi eseipi requested a review from bsdimp as a code owner October 1, 2024 10:03
@bsdimp
Copy link
Member

bsdimp commented Oct 1, 2024

Generally, I like this. I'm unsure of what we should do by default...

@wigneddoom
Copy link

This is a great feature. But for some reason, I think that we can implement it in user space - nvmecontrol(8)

@eseipi

This comment was marked as outdated.

@bsdimp bsdimp self-assigned this Oct 4, 2024
@eseipi eseipi marked this pull request as draft October 6, 2024 08:08
@eseipi
Copy link
Author

eseipi commented Oct 7, 2024

Though my approach to configuring APST closely resembles the Linux implementation, I agree that its design could be improved. In particular, I totally agree with @wigneddoom on moving as much functionality as possible out of kernel, though I'm still considering the best way to achieve this. I've marked this PR as a draft, so I can take some time to give it more thought. Please remove the needs-review label for now.

@eseipi eseipi marked this pull request as ready for review October 9, 2024 16:51
@eseipi eseipi force-pushed the main branch 4 times, most recently from 45d3802 to 913a3f4 Compare October 11, 2024 19:57
@eseipi
Copy link
Author

eseipi commented Oct 31, 2024

I've created a revision (D47381) for nvmecontrol(8), which leaves just two things here: toggling (hw.nvme.apst_enable) and data overriding (hw.nvme.apst_data). This saves us from having to hook userspace when (re)initializing the controller, so I think it's reasonable to have this in the driver.

Signed-off-by: Alexey Sukhoguzov <[email protected]>
APST is an optional NVMe power-saving feature that allows devices
to autonomously enter higher non-operational power states after a
certain amount of idle time, reducing the controller's overall power
consumption.

Signed-off-by: Alexey Sukhoguzov <[email protected]>
The apst_data tunable allows APST configuration to be adjusted
during controller initialization.  It accepts an array of encoded
integers, each defining specific transition parameters.

Signed-off-by: Alexey Sukhoguzov <[email protected]>
@eseipi
Copy link
Author

eseipi commented Dec 14, 2024

Fixes:

  • merge conflict with f08746a;
  • poor memory management on nvme_ctrlr_cmd_get_feature failure;
  • couple of style issues.

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

Successfully merging this pull request may close these issues.

3 participants