nvme power saving
v3 cover nvme power saving
Hi all-
Here's v3 of the APST patch set. The biggest bikesheddable thing (I think) is the scaling factor. I currently have it hardcoded so that we wait 50x the total latency before entering a power saving state. On my Samsung 950, this means we enter state 3 (70mW, 0.5ms entry latency, 5ms exit latency) after 275ms and state 4 (5mW, 2ms entry latency, 22ms exit latency) after 1200ms. I have the default max latency set to 25ms.
FWIW, in practice, the latency this introduces seems to be well under 22ms, but my benchmark is a bit silly and I might have measured it wrong. I certainly haven't observed a slowdown just using my laptop.
This time around, I changed the names of parameters after Jay Frayensee got confused by the first try. Now they are:
- ps_max_latency_us in sysfs: actually controls it.
- nvme_core.default_ps_max_latency_us: sets the default.
Yeah, they're mouthfuls, but they should be clearer now.
Changes from v2:
- Rename the parameters.
Changes from v1:
- Get rid of feature buffer alignment warnings.
- Change the error message if NPSS is bogus.
- Rename apst_max_latency_ns to apst_max_latency_us because module params don't like u64 or unsigned long long and I wanted to make it fit more comfortably in a ulong module param. (And the nanoseconds were useless.)
- Add a module parameter for the default max latency.
Andy Lutomirski (3):
nvme/scsi: Remove power management support
nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
nvme: Enable autonomous power state transitions
drivers/nvme/host/core.c | 184 ++++++++++++++++++++++++++++++++++++++++++++--- drivers/nvme/host/nvme.h | 10 ++- drivers/nvme/host/scsi.c | 80 ++------------------- include/linux/nvme.h | 6 ++ 4 files changed, 196 insertions(+), 84 deletions(-)
Signature · 1 line
2.7.4
PATCH v3 1/3 nvme/scsi: Remove power management support
As far as I can tell, there is basically nothing correct about this code. It misinterprets npss (off-by-one). It hardcodes a bunch of power states, which is nonsense, because they're all just indices into a table that software needs to parse. It completely ignores the distinction between operational and non-operational states. And, until 4.8, if all of the above magically succeeded, it would dereference a NULL pointer and OOPS.
Since this code appears to be useless, just delete it.
--- drivers/nvme/host/scsi.c | 74 ++---------------------------------------------- 1 file changed, 3 insertions(+), 71 deletions(-)
drivers/nvme/host/scsi.c +3 -71
@@ -72,15 +72,6 @@ static int sg_version_num = 30534; /* 2 digits for each component */#define ALL_LUNS_RETURNED 0x02#define ALL_WELL_KNOWN_LUNS_RETURNED 0x01#define RESTRICTED_LUNS_RETURNED 0x00 -#define NVME_POWER_STATE_START_VALID 0x00 -#define NVME_POWER_STATE_ACTIVE 0x01 -#define NVME_POWER_STATE_IDLE 0x02 -#define NVME_POWER_STATE_STANDBY 0x03 -#define NVME_POWER_STATE_LU_CONTROL 0x07 -#define POWER_STATE_0 0 -#define POWER_STATE_1 1 -#define POWER_STATE_2 2 -#define POWER_STATE_3 3#define DOWNLOAD_SAVE_ACTIVATE 0x05#define DOWNLOAD_SAVE_DEFER_ACTIVATE 0x0E#define ACTIVATE_DEFERRED_MICROCODE 0x0F
@@ -1229,64 +1220,6 @@ static void nvme_trans_fill_read_cap(u8 *response, struct nvme_id_ns *id_ns,/* Start Stop Unit Helper Functions */ -static int nvme_trans_power_state (struct nvme_ns *ns,struct sg_io_hdr *hdr, -u8 pc,u8 pcmod,u8 start) -{ -int res; -int nvme_sc; -struct nvme_id_ctrl *id_ctrl; -int lowest_pow_st;/* max npss = lowest power consumption */ -unsigned ps_desired =0 ; - - nvme_sc =nvme_identify_ctrl (ns->ctrl, &id_ctrl); - res =nvme_trans_status_code (hdr, nvme_sc); -if (res) -return res; - - lowest_pow_st =max (POWER_STATE_0 , (int )(id_ctrl->npss -1 )); -kfree (id_ctrl); - -switch (pc) { -case NVME_POWER_STATE_START_VALID : -/* Action unspecified if POWER CONDITION MODIFIER != 0 */ -if (pcmod ==0 && start ==0x1 ) - ps_desired =POWER_STATE_0 ; -if (pcmod ==0 && start ==0x0 ) - ps_desired = lowest_pow_st; -break ; -case NVME_POWER_STATE_ACTIVE : -/* Action unspecified if POWER CONDITION MODIFIER != 0 */ -if (pcmod ==0 ) - ps_desired =POWER_STATE_0 ; -break ; -case NVME_POWER_STATE_IDLE : -/* Action unspecified if POWER CONDITION MODIFIER != [0,1,2] */ -if (pcmod ==0x0 ) - ps_desired =POWER_STATE_1 ; -else if (pcmod ==0x1 ) - ps_desired =POWER_STATE_2 ; -else if (pcmod ==0x2 ) - ps_desired =POWER_STATE_3 ; -break ; -case NVME_POWER_STATE_STANDBY : -/* Action unspecified if POWER CONDITION MODIFIER != [0,1] */ -if (pcmod ==0x0 ) - ps_desired =max (POWER_STATE_0 , (lowest_pow_st -2 )); -else if (pcmod ==0x1 ) - ps_desired =max (POWER_STATE_0 , (lowest_pow_st -1 )); -break ; -case NVME_POWER_STATE_LU_CONTROL : -default : - res =nvme_trans_completion (hdr,SAM_STAT_CHECK_CONDITION , -ILLEGAL_REQUEST ,SCSI_ASC_INVALID_CDB , -SCSI_ASCQ_CAUSE_NOT_REPORTABLE ); -break ; - } - nvme_sc =nvme_set_features (ns->ctrl,NVME_FEAT_POWER_MGMT , ps_desired,0 , -NULL ); -return nvme_trans_status_code (hdr, nvme_sc); -} -static int nvme_trans_send_activate_fw_cmd (struct nvme_ns *ns,struct sg_io_hdr *hdr,u8 buffer_id) {
@@ -2235,11 +2168,10 @@ static int nvme_trans_synchronize_cache(struct nvme_ns *ns,static int nvme_trans_start_stop (struct nvme_ns *ns,struct sg_io_hdr *hdr,u8 *cmd) { -u8 immed, pcmod, pc, no_flush, start; +u8 immed, pcmod, no_flush, start; immed = cmd[1 ] &0x01 ; pcmod = cmd[3 ] &0x0f ; - pc = (cmd[4 ] &0xf0 ) >>4 ; no_flush = cmd[4 ] &0x04 ; start = cmd[4 ] &0x01 ;
@@ -2254,8 +2186,8 @@ static int nvme_trans_start_stop(struct nvme_ns *ns, struct sg_io_hdr *hdr,if (res)return res; } -/* Setup the expected power state transition */ -return nvme_trans_power_state (ns, hdr, pc, pcmod, start); + +return 0 ; } }
Signature · 1 line
2.7.4
Re: [PATCH v3 1/3] nvme/scsi: Remove power management support
Quoted 9 lines · Andy Lutomirski
As far as I can tell, there is basically nothing correct about this code. It misinterprets npss (off-by-one). It hardcodes a bunch of power states, which is nonsense, because they're all just indices into a table that software needs to parse. It completely ignores the distinction between operational and non-operational states. And, until 4.8, if all of the above magically succeeded, it would dereference a NULL pointer and OOPS.
Since this code appears to be useless, just delete it.
Yes, please!
Re: [PATCH v3 1/3] nvme/scsi: Remove power management support
Quoted 4 lines · Christoph Hellwig
Quoted 1 line · Andy Lutomirski
Since this code appears to be useless, just delete it.
Yes, please!
For a future patch, any objection to deleting the whole file? :)
Christoph Hellwig
maintainerRe: [PATCH v3 1/3] nvme/scsi: Remove power management support
Quoted 7 lines · Keith Busch
Quoted 4 lines · Christoph Hellwig
Quoted 1 line · Andy Lutomirski
Since this code appears to be useless, just delete it.
Yes, please!
For a future patch, any objection to deleting the whole file? :)
Not from me for sure, but I fear some users might be using it after all.
Re: [PATCH v3 1/3] nvme/scsi: Remove power management support
Quoted 10 lines · Christoph Hellwig
Quoted 7 lines · Keith Busch
Quoted 4 lines · Christoph Hellwig
Quoted 1 line · Andy Lutomirski
Since this code appears to be useless, just delete it.
Yes, please!
For a future patch, any objection to deleting the whole file? :)
Not from me for sure, but I fear some users might be using it after all.
IIRC there was some issues with openSUSE and pre NVMe 1.2 Adapters without the scsi translation.
Byte,
Johannes
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
J Freyensee
Acked-byRe: [PATCH v3 1/3] nvme/scsi: Remove power management support
In reply to Christoph HellwigQuoted 10 lines · Christoph Hellwig
Quoted 9 lines · Andy Lutomirski
As far as I can tell, there is basically nothing correct about this code.??It misinterprets npss (off-by-one).??It hardcodes a bunch of power states, which is nonsense, because they're all just indices into a table that software needs to parse.??It completely ignores the distinction between operational and non-operational states. And, until 4.8, if all of the above magically succeeded, it would dereference a NULL pointer and OOPS.
Quoted 5 lines
Quoted 1 line
Since this code appears to be useless, just delete it.
Yes, please!
Acked-by Christoph Hellwig <hch at lst.de>
PATCH v3 2/3 nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
In reply to Andy LutomirskiAny user I can imagine that needs a buffer at all will want to pass a pointer directly. There are no currently callers that use buffers, so this change is painless, and it will make it much easier to start using features that use buffers (e.g. APST).
--- drivers/nvme/host/core.c | 18 ++++++++++-------- drivers/nvme/host/nvme.h | 4 ++-- drivers/nvme/host/scsi.c | 6 +++--- 3 files changed, 15 insertions(+), 13 deletions(-)
drivers/nvme/host/core.c +10 -8
@@ -597,7 +597,7 @@ int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid, }int nvme_get_features (struct nvme_ctrl *dev,unsigned fid,unsigned nsid, -dma_addr_t dma_addr,u32 *result) +void *buffer,size_t buflen,u32 *result) {struct nvme_command c;struct nvme_completion cqe;
@@ -606,10 +606,9 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,memset (&c,0 ,sizeof (c)); c.features.opcode = nvme_admin_get_features; c.features.nsid =cpu_to_le32 (nsid); - c.features.dptr.prp1 =cpu_to_le64 (dma_addr); c.features.fid =cpu_to_le32 (fid); - ret =__nvme_submit_sync_cmd (dev->admin_q, &c, &cqe,NULL ,0 ,0 , + ret =__nvme_submit_sync_cmd (dev->admin_q, &c, &cqe, buffer, buflen,0 ,NVME_QID_ANY ,0 ,0 );if (ret >=0 && result) *result =le32_to_cpu (cqe.result);
@@ -617,7 +616,7 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid, }int nvme_set_features (struct nvme_ctrl *dev,unsigned fid,unsigned dword11, -dma_addr_t dma_addr,u32 *result) +const void *buffer,size_t buflen,u32 *result) {struct nvme_command c;struct nvme_completion cqe;
@@ -625,12 +624,15 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,memset (&c,0 ,sizeof (c)); c.features.opcode = nvme_admin_set_features; - c.features.dptr.prp1 =cpu_to_le64 (dma_addr); c.features.fid =cpu_to_le32 (fid); c.features.dword11 =cpu_to_le32 (dword11); - ret =__nvme_submit_sync_cmd (dev->admin_q, &c, &cqe,NULL ,0 ,0 , -NVME_QID_ANY ,0 ,0 ); +/* +* Casting buffer to void* is safe here: __nvme_submit_sync_cmd knows +* that we're writing because it decodes the opcode. +*/ + ret =__nvme_submit_sync_cmd (dev->admin_q, &c, &cqe, + (void *)buffer, buflen,0 ,NVME_QID_ANY ,0 ,0 );if (ret >=0 && result) *result =le32_to_cpu (cqe.result);return ret;
@@ -664,7 +666,7 @@ int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)u32 result;int status, nr_io_queues; - status =nvme_set_features (ctrl,NVME_FEAT_NUM_QUEUES , q_count,0 , + status =nvme_set_features (ctrl,NVME_FEAT_NUM_QUEUES , q_count,NULL ,0 , &result);if (status <0 )return status;
drivers/nvme/host/nvme.h +2 -2
@@ -292,9 +292,9 @@ int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid, struct nvme_id_ns **id);int nvme_get_log_page (struct nvme_ctrl *dev,struct nvme_smart_log **log);int nvme_get_features (struct nvme_ctrl *dev,unsigned fid,unsigned nsid, -dma_addr_t dma_addr,u32 *result); +void *buffer,size_t buflen,u32 *result);int nvme_set_features (struct nvme_ctrl *dev,unsigned fid,unsigned dword11, -dma_addr_t dma_addr,u32 *result); +const void *buffer,size_t buflen,u32 *result);int nvme_set_queue_count (struct nvme_ctrl *ctrl,int *count);void nvme_start_keep_alive (struct nvme_ctrl *ctrl);void nvme_stop_keep_alive (struct nvme_ctrl *ctrl);
drivers/nvme/host/scsi.c +3 -3
@@ -906,7 +906,7 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,kfree (smart_log);/* Get Features for Temp Threshold */ - res =nvme_get_features (ns->ctrl,NVME_FEAT_TEMP_THRESH ,0 ,0 , + res =nvme_get_features (ns->ctrl,NVME_FEAT_TEMP_THRESH ,0 ,NULL ,0 , &feature_resp);if (res !=NVME_SC_SUCCESS ) temp_c_thresh =LOG_TEMP_UNKNOWN ;
@@ -1039,7 +1039,7 @@ static int nvme_trans_fill_caching_page(struct nvme_ns *ns,if (len <MODE_PAGE_CACHING_LEN )return -EINVAL ; - nvme_sc =nvme_get_features (ns->ctrl,NVME_FEAT_VOLATILE_WC ,0 ,0 , + nvme_sc =nvme_get_features (ns->ctrl,NVME_FEAT_VOLATILE_WC ,0 ,NULL ,0 , &feature_resp); res =nvme_trans_status_code (hdr, nvme_sc);if (res)
@@ -1328,7 +1328,7 @@ static int nvme_trans_modesel_get_mp(struct nvme_ns *ns, struct sg_io_hdr *hdr,case MODE_PAGE_CACHING : dword11 = ((mode_page[2 ] &CACHING_MODE_PAGE_WCE_MASK ) ?1 :0 ); nvme_sc =nvme_set_features (ns->ctrl,NVME_FEAT_VOLATILE_WC , - dword11,0 ,NULL ); + dword11,NULL ,0 ,NULL ); res =nvme_trans_status_code (hdr, nvme_sc);break ;case MODE_PAGE_CONTROL :
Signature · 1 line
2.7.4
Christoph Hellwig
maintainerRe: [PATCH v3 2/3] nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
Quoted 7 lines · Andy Lutomirski
Any user I can imagine that needs a buffer at all will want to pass a pointer directly. There are no currently callers that use buffers, so this change is painless, and it will make it much easier to start using features that use buffers (e.g. APST).
Signed-off-by Andy Lutomirski <luto at kernel.org>---
Looks good mostly good, but a nitpick below:
drivers/nvme/host/core.c int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11, · line 635
Quoted 6 lines
drivers/nvme/host/core.c +6
-0+/* +* Casting buffer to void* is safe here: __nvme_submit_sync_cmd knows +* that we're writing because it decodes the opcode. +*/ + ret =__nvme_submit_sync_cmd (dev->admin_q, &c, &cqe, + (void *)buffer, buflen,0 ,NVME_QID_ANY ,0 ,0 );
Cant we just drop the const annotation to avoid these casts?
Re: [PATCH v3 2/3] nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
Quoted 12 lines · Christoph Hellwig
Quoted 7 lines · Andy Lutomirski
Any user I can imagine that needs a buffer at all will want to pass a pointer directly. There are no currently callers that use buffers, so this change is painless, and it will make it much easier to start using features that use buffers (e.g. APST).
Signed-off-by Andy Lutomirski <luto at kernel.org>---
Looks good mostly good, but a nitpick below:
Quoted 9 lines
drivers/nvme/host/core.c int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11, · line 635
Quoted 6 lines
drivers/nvme/host/core.c +6
-0+/* +* Casting buffer to void* is safe here: __nvme_submit_sync_cmd knows +* that we're writing because it decodes the opcode. +*/ + ret =__nvme_submit_sync_cmd (dev->admin_q, &c, &cqe, + (void *)buffer, buflen,0 ,NVME_QID_ANY ,0 ,0 );Cant we just drop the const annotation to avoid these casts?
Then we'd have nvme_set_feature() taking a non-const pointer, which would seem a little bit silly to me and might require the same cast somewhere else down the road.
Re: [PATCH v3 2/3] nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
Quoted 20 lines · Andy Lutomirski
Quoted 19 lines · Christoph Hellwig
Quoted 7 lines · Andy Lutomirski
Any user I can imagine that needs a buffer at all will want to pass a pointer directly. There are no currently callers that use buffers, so this change is painless, and it will make it much easier to start using features that use buffers (e.g. APST).
Signed-off-by Andy Lutomirski <luto at kernel.org>---
Looks good mostly good, but a nitpick below:
Quoted 6 lines
drivers/nvme/host/core.c +6
-0+/* +* Casting buffer to void* is safe here: __nvme_submit_sync_cmd knows +* that we're writing because it decodes the opcode. +*/ + ret =__nvme_submit_sync_cmd (dev->admin_q, &c, &cqe, + (void *)buffer, buflen,0 ,NVME_QID_ANY ,0 ,0 );
Quoted 6 lines
Quoted 2 lines
Cant we just drop the const annotation to avoid these casts?
Then we'd have
nvme_set_feature()taking a non-const pointer, which would seem a little bit silly to me and might require the same cast somewhere else down the road.
That'd still be a lot cleaner than the cast, which needs a comment to explain why it's supposedly safe. Just drop the const, imho.
Signature · 1 line
Jens Axboe
PATCH v3 3/3 nvme: Enable autonomous power state transitions
In reply to Andy LutomirskiNVMe devices can advertise multiple power states. These states can be either "operational" (the device is fully functional but possibly slow) or "non-operational" (the device is asleep until woken up). Some devices can automatically enter a non-operational state when idle for a specified amount of time and then automatically wake back up when needed.
The hardware configuration is a table. For each state, an entry in the table indicates the next deeper non-operational state, if any, to autonomously transition to and the idle time required before transitioning.
This patch teaches the driver to program APST so that each successive non-operational state will be entered after an idle time equal to 100% of the total latency (entry plus exit) associated with that state. A sysfs attribute 'ps_max_latency_us' gives the maximum acceptable latency in ns; non-operational states with total latency greater than this value will not be used. As a special case, ps_max_latency_us=0 will disable APST entirely. On hardware without APST support, ps_max_latency_us will not be exposed in sysfs.
The ps_max_latency_us parameter for newly-probed devices is set by the module parameter nvme_core.default_ps_max_latency_us.
In theory, the device can expose "default" APST table, but this doesn't seem to function correctly on my device (Samsung 950), nor does it seem particularly useful. There is also an optional mechanism by which a configuration can be "saved" so it will be automatically loaded on reset. This can be configured from userspace, but it doesn't seem useful to support in the driver.
On my laptop, enabling APST seems to save nearly 1W.
The hardware tables can be decoded in userspace with nvme-cli. 'nvme id-ctrl /dev/nvmeN' will show the power state table and 'nvme get-feature -f 0x0c -H /dev/nvme0' will show the current APST configuration.
I called the parameters ps_max_latency_us instead of apst_max_latency_us because we might support other power saving modes (e.g. non-automonous power state transitions or even runtime D3) and the same parameter could control the maximum allowable latency for these states as well.
--- drivers/nvme/host/core.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++ drivers/nvme/host/nvme.h | 6 ++ include/linux/nvme.h | 6 ++ 3 files changed, 178 insertions(+)
drivers/nvme/host/core.c +166 -0
@@ -56,6 +56,11 @@ EXPORT_SYMBOL_GPL(nvme_max_retries);static int nvme_char_major;module_param (nvme_char_major, int,0 ); +static unsigned long default_ps_max_latency_us =25000 ; +module_param (default_ps_max_latency_us, ulong,0644 ); +MODULE_PARM_DESC (ps_max_latency_us, +"default max power saving latency; overridden per device in sysfs" ); +static LIST_HEAD (nvme_ctrl_list);static DEFINE_SPINLOCK (dev_list_lock);
@@ -1209,6 +1214,98 @@ static void nvme_set_queue_limits(struct nvme_ctrl *ctrl,blk_queue_write_cache (q, vwc, vwc); } +static void nvme_configure_apst (struct nvme_ctrl *ctrl) +{ +/* +* APST (Autonomous Power State Transition) lets us program a +* table of power state transitions that the controller will +* perform automatically. We configure it with a simple +* heuristic: we are willing to spend at most 2% of the time +* transitioning between power states. Therefore, when running +* in any given state, we will enter the next lower-power +* non-operational state after waiting 100 * (enlat + exlat) +* microseconds, as long as that state's total latency is under +* the requested maximum latency. +* +* We will not autonomously enter any non-operational state for +* which the total latency exceeds ps_max_latency_us. Users +* can set ps_max_latency_us to zero to turn off APST. +*/ + +unsigned apste; +struct nvme_feat_auto_pst *table; +int ret; + +if (!ctrl->apsta) +return ;/* APST isn't supported. */ + +if (ctrl->npss >31 ) { +dev_warn (ctrl->device,"NPSS is invalid; not using APST\n" ); +return ; + } + + table =kzalloc (sizeof (*table),GFP_KERNEL ); +if (!table) +return ; + +if (ctrl->ps_max_latency_us ==0 ) { +/* Turn off APST. */ + apste =0 ; + }else { +__le64 target =cpu_to_le64 (0 ); +int state; + +/* +* Walk through all states from lowest- to highest-power. +* According to the spec, lower-numbered states use more +* power. NPSS, despite the name, is the index of the +* lowest-power state, not the number of states. +*/ +for (state = (int )ctrl->npss; state >=0 ; state--) { +u64 total_latency_us, transition_ms; + +if (target) + table->entries[state] = target; + +/* +* Is this state a useful non-operational state for +* higher-power states to autonomously transition to? +*/ +if (!(ctrl->psd[state].flags &2 )) +continue ;/* It's an operational state. */ + + total_latency_us = + (u64 )cpu_to_le32 (ctrl->psd[state].entry_lat) + + +cpu_to_le32 (ctrl->psd[state].exit_lat); +if (total_latency_us > ctrl->ps_max_latency_us) +continue ; + +/* +* This state is good. Use it as the APST idle +* target for higher power states. +*/ + transition_ms = total_latency_us +19 ; +do_div (transition_ms,20 ); +if (transition_ms >= (1 <<24 )) + transition_ms = (1 <<24 ); + + target =cpu_to_le64 ((state <<3 ) | + (transition_ms <<8 )); + } + + apste =1 ; + } + + ret =nvme_set_features (ctrl,NVME_FEAT_AUTO_PST , apste, + table,sizeof (*table),NULL ); +if (ret) +dev_err (ctrl->device,"failed to set APST feature (%d)\n" , ret); + +kfree (table); +} + +static struct attribute_group nvme_dev_dynamic_attrs_group; +/* * Initialize the cached copies of the Identify data and various controller * register in our nvme_ctrl structure. This should be called as soon as
@@ -1275,6 +1372,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) ctrl->sgls =le32_to_cpu (id->sgls); ctrl->kas =le16_to_cpu (id->kas); + ctrl->npss = id->npss; + ctrl->apsta = id->apsta; +memcpy (ctrl->psd, id->psd,sizeof (ctrl->psd)); +if (ctrl->ops->is_fabrics) { ctrl->icdoff =le16_to_cpu (id->icdoff); ctrl->ioccsz =le32_to_cpu (id->ioccsz);
@@ -1298,6 +1399,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) }kfree (id); + +nvme_configure_apst (ctrl); + +sysfs_update_group (&ctrl->device->kobj, &nvme_dev_dynamic_attrs_group);return ret; }EXPORT_SYMBOL_GPL (nvme_init_identify);
@@ -1568,6 +1673,41 @@ static ssize_t nvme_sysfs_show_address(struct device *dev, } static DEVICE_ATTR(address, S_IRUGO, nvme_sysfs_show_address, NULL); +static ssize_t nvme_sysfs_show_ps_max_latency_us ( +struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct nvme_ctrl *ctrl =dev_get_drvdata (dev); + +return snprintf (buf,PAGE_SIZE ,"%llu\n" , ctrl->ps_max_latency_us); +} + +static ssize_t nvme_sysfs_store_ps_max_latency_us ( +struct device *dev, +struct device_attribute *attr, +const char *buf,size_t size) +{ +struct nvme_ctrl *ctrl =dev_get_drvdata (dev); +int ret; +u64 val; + + ret =kstrtoull (buf,10 , &val); +if (ret) +return ret; + +if (ctrl->ps_max_latency_us != val) { + ctrl->ps_max_latency_us = val; +nvme_configure_apst (ctrl); + } + +return size; +} + +static DEVICE_ATTR(ps_max_latency_us, 0644, + nvme_sysfs_show_ps_max_latency_us, + nvme_sysfs_store_ps_max_latency_us); +static struct attribute *nvme_dev_attrs[] = { &dev_attr_reset_controller.attr, &dev_attr_rescan_controller.attr,
@@ -1609,8 +1749,33 @@ static struct attribute_group nvme_dev_attrs_group = { .is_visible = nvme_dev_attrs_are_visible, }; +static struct attribute *nvme_dev_dynamic_attrs[] = { + &dev_attr_ps_max_latency_us.attr, +NULL +}; + +static umode_t nvme_dev_dynamic_attrs_are_visible (struct kobject *kobj, +struct attribute *a,int n) +{ + struct device *dev = container_of(kobj, struct device, kobj); +struct nvme_ctrl *ctrl =dev_get_drvdata (dev); + +if (a == &dev_attr_ps_max_latency_us.attr) { +if (!ctrl->apsta) +return 0 ; + } + +return a->mode; +} + +static struct attribute_group nvme_dev_dynamic_attrs_group = { + .attrs = nvme_dev_dynamic_attrs, + .is_visible = nvme_dev_dynamic_attrs_are_visible, +}; +static const struct attribute_group *nvme_dev_attr_groups[] = { &nvme_dev_attrs_group, + &nvme_dev_dynamic_attrs_group,NULL , };
@@ -1995,6 +2160,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev, ctrl->quirks = quirks;INIT_WORK (&ctrl->scan_work, nvme_scan_work);INIT_WORK (&ctrl->async_event_work, nvme_async_event_work); + ctrl->ps_max_latency_us = default_ps_max_latency_us; ret =nvme_set_instance (ctrl);if (ret)
drivers/nvme/host/nvme.h +6 -0
@@ -129,13 +129,19 @@ struct nvme_ctrl {u32 vs;u32 sgls;u16 kas; +u8 npss; +u8 apsta;unsigned int kato;bool subsystem;unsigned long quirks; +struct nvme_id_power_state psd[32 ];struct work_struct scan_work;struct work_struct async_event_work;struct delayed_work ka_work; +/* Power saving configuration */ +u64 ps_max_latency_us; +/* Fabrics only */ u16 sqsize;u32 ioccsz;
include/linux/nvme.h +6 -0
@@ -543,6 +543,12 @@ struct nvme_dsm_range {__le64 slba; }; +/* Features */ + +struct nvme_feat_auto_pst { +__le64 entries[32 ]; +}; +/* Admin commands */ enum nvme_admin_opcode {
Signature · 1 line
2.7.4
Re: [PATCH v3 3/3] nvme: Enable autonomous power state transitions
Looks good to me:
Did you sort out with the PM people if this should have any framework integration or not?
Re: [PATCH v3 3/3] nvme: Enable autonomous power state transitions
Quoted 7 lines · Christoph Hellwig
Looks good to me:
Reviewed-by Christoph Hellwig <hch at lst.de>Did you sort out with the PM people if this should have any framework integration or not?
No. I'll email now. I'm not holding my breath for a good answer very quickly.
Christoph Hellwig
maintainerRe: [PATCH v3 3/3] nvme: Enable autonomous power state transitions
Quoted 1 line · Andy Lutomirski
No. I'll email now. I'm not holding my breath for a good answer very quickly.
Just to make it clear: I'm perfectly fine with merging your patch as-is for now as it is really useful. But at the same time I'd really love to kick off that discussion, as an integration will be useful in the long run.
Re: [PATCH v3 3/3] nvme: Enable autonomous power state transitions
In reply to Andy LutomirskiQuoted 38 lines · Andy Lutomirski
NVMe devices can advertise multiple power states. These states can be either "operational" (the device is fully functional but possibly slow) or "non-operational" (the device is asleep until woken up). Some devices can automatically enter a non-operational state when idle for a specified amount of time and then automatically wake back up when needed.
The hardware configuration is a table. For each state, an entry in the table indicates the next deeper non-operational state, if any, to autonomously transition to and the idle time required before transitioning.
This patch teaches the driver to program APST so that each successive non-operational state will be entered after an idle time equal to 100% of the total latency (entry plus exit) associated with that state. A sysfs attribute '
ps_max_latency_us' gives the maximum acceptable latency in ns; non-operational states with total latency greater than this value will not be used. As a special case, ps_max_latency_us=0 will disable APST entirely. On hardware without APST support,ps_max_latency_uswill not be exposed in sysfs.The
ps_max_latency_usparameter for newly-probed devices is set by the module parameter nvme_core.default_ps_max_latency_us.In theory, the device can expose "default" APST table, but this doesn't seem to function correctly on my device (Samsung 950), nor does it seem particularly useful. There is also an optional mechanism by which a configuration can be "saved" so it will be automatically loaded on reset. This can be configured from userspace, but it doesn't seem useful to support in the driver.
On my laptop, enabling APST seems to save nearly 1W.
The hardware tables can be decoded in userspace with nvme-cli. 'nvme id-ctrl
/dev/nvmeN' will show the power state table and 'nvme get-feature -f0x0c-H/dev/nvme0' will show the current APST configuration.
Quoted 7 lines
I called the parameters
ps_max_latency_usinstead ofapst_max_latency_usbecause we might support other power saving modes (e.g. non-automonous power state transitions or even runtime D3) and the same parameter could control the maximum allowable latency for these states as well.Signed-off-by Andy Lutomirski <[email protected]>
Thanks, looks good to me.