Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1485226 > unrolled thread

[PATCH v4 0/3] nvme power saving

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/3] nvme power saving Andy Lutomirski <luto@kernel.org> - 2016-09-16 20:20 +0200
    [PATCH v4 2/3] nvme: Pass pointers, not dma addresses, to nvme_get/set_features() Andy Lutomirski <luto@kernel.org> - 2016-09-16 20:20 +0200
    Re: [PATCH v4 0/3] nvme power saving J Freyensee <james_p_freyensee@linux.intel.com> - 2016-09-17 03:00 +0200
    Re: [PATCH v4 0/3] nvme power saving Andy Lutomirski <luto@amacapital.net> - 2016-09-22 02:20 +0200
      Re: [PATCH v4 0/3] nvme power saving Christoph Hellwig <hch@lst.de> - 2016-09-22 15:30 +0200
    Re: [PATCH v4 0/3] nvme power saving Andy Lutomirski <luto@amacapital.net> - 2016-09-22 22:20 +0200
      Re: [PATCH v4 0/3] nvme power saving J Freyensee <james_p_freyensee@linux.intel.com> - 2016-09-22 23:40 +0200
        Re: [PATCH v4 0/3] nvme power saving Keith Busch <keith.busch@intel.com> - 2016-09-23 00:10 +0200
        Re: [PATCH v4 0/3] nvme power saving Andy Lutomirski <luto@amacapital.net> - 2016-09-23 00:20 +0200
    Re: [PATCH v4 0/3] nvme power saving Christoph Hellwig <hch@lst.de> - 2016-09-24 01:50 +0200

#1485226 — [PATCH v4 0/3] nvme power saving

Hi all-

Here's v4 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 v3:
 - Remove const from nvme_set_feature()'s parameter.  My inner C++
   programmer cringes a little...

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 | 180 ++++++++++++++++++++++++++++++++++++++++++++---
 drivers/nvme/host/nvme.h |  10 ++-
 drivers/nvme/host/scsi.c |  80 ++-------------------
 include/linux/nvme.h     |   6 ++
 4 files changed, 192 insertions(+), 84 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1485227 — [PATCH v4 2/3] nvme: Pass pointers, not dma addresses, to nvme_get/set_features()

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@kernel.org>
---
 drivers/nvme/host/core.c | 14 ++++++--------
 drivers/nvme/host/nvme.h |  4 ++--
 drivers/nvme/host/scsi.c |  6 +++---
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 2feacc70bf61..b92a7f767bfc 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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)
+		      void *buffer, size_t buflen, u32 *result)
 {
 	struct nvme_command c;
 	struct nvme_completion cqe;
@@ -625,12 +624,11 @@ 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);
+	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);
 	return ret;
@@ -664,7 +662,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;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index ab18b78102bf..c2151761ec5f 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -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);
+		      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);
diff --git a/drivers/nvme/host/scsi.c b/drivers/nvme/host/scsi.c
index 44009105f8c8..c2a0a1c7d05d 100644
--- a/drivers/nvme/host/scsi.c
+++ b/drivers/nvme/host/scsi.c
@@ -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:
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1485446

On Fri, 2016-09-16 at 11:16 -0700, Andy Lutomirski wrote:
> Hi all-
> 
> Here's v4 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.
> 

I took the patches and applied them to one of my NVMe fabric hosts on
my NVMe-over-Fabrics setup.  Basically, it doesn't test much other than
Andy's explanation that "ps_max_latency_us" does not appear in any of
/sys/block/nvmeXnY sysfs nodes (I have 7) so seems good to me on this
front.

Tested-by: Jay Freyensee <james_p_freyensee@linux.intel.com>
[jpf: defaults benign to NVMe-over-Fabrics]

[toc] | [prev] | [next] | [standalone]

#1488475

On Fri, Sep 16, 2016 at 11:16 AM, Andy Lutomirski <luto@kernel.org> wrote:
> Hi all-
>
> Here's v4 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.

Anything I can/should do to help this make it for 4.9? :)

--Andy

[toc] | [prev] | [next] | [standalone]

#1488873

On Wed, Sep 21, 2016 at 05:11:03PM -0700, Andy Lutomirski wrote:
> Anything I can/should do to help this make it for 4.9? :)

Maybe you should have added the Reviewed-by: tags you already got to
this repost? :)

Here we go again:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [next] | [standalone]

#1489519

On Thu, Sep 22, 2016 at 7:23 AM, Jens Axboe <axboe@fb.com> wrote:
>
> On 09/16/2016 12:16 PM, Andy Lutomirski wrote:
>>
>> Hi all-
>>
>> Here's v4 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.
>
>
> The only thing I don't like about this is the fact that's it's a driver private thing. Similar to ALPM on SATA, it's yet another knob that needs to be set. It we put it somewhere generic, then at least we could potentially use it in a generic fashion.

Agreed.  I'm hoping to hear back from Rafael soon about the dev_pm_qos thing.

>
> Additionally, it should not be on by default.

I think I disagree with this.  Since we don't have anything like
laptop-mode AFAIK, I think we do want it on by default.  For the
server workloads that want to consume more idle power for faster
response when idle, I think the servers should be willing to make this
change, just like they need to disable overly deep C states, etc.
(Admittedly, unifying the configuration would be nice.)

[toc] | [prev] | [next] | [standalone]

#1489576

On Thu, 2016-09-22 at 14:43 -0600, Jens Axboe wrote:
> On 09/22/2016 02:11 PM, Andy Lutomirski wrote:
> > 
> > On Thu, Sep 22, 2016 at 7:23 AM, Jens Axboe <axboe@fb.com> wrote:
> > > 
> > > 
> > > On 09/16/2016 12:16 PM, Andy Lutomirski wrote:
> > > > 
> > > > 
> > > > Hi all-
> > > > 
> > > > Here's v4 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.
> > > 
> > > 
> > > The only thing I don't like about this is the fact that's it's a
> > > driver private thing. Similar to ALPM on SATA, it's yet another
> > > knob that needs to be set. It we put it somewhere generic, then
> > > at least we could potentially use it in a generic fashion.
> > 
> > Agreed.  I'm hoping to hear back from Rafael soon about the
> > dev_pm_qos
> > thing.
> > 
> > > 
> > > 
> > > Additionally, it should not be on by default.
> > 
> > I think I disagree with this.  Since we don't have anything like
> > laptop-mode AFAIK, I think we do want it on by default.  For the
> > server workloads that want to consume more idle power for faster
> > response when idle, I think the servers should be willing to make
> > this
> > change, just like they need to disable overly deep C states, etc.
> > (Admittedly, unifying the configuration would be nice.)
> 
> I can see two reasons why we don't want it the default:
> 
> 1) Changes like this has a tendency to cause issues on various types
> of
> hardware. How many NVMe devices have you tested this on? ALPM on SATA
> had a lot of initial problems, where slowed down some SSDs unberably.

...and some SSDs don't even support this feature yet, so the number of
different NVMe devices available to test initially will most likely be
small (like the Fultondales I have, all I could check is to see if the
code broke anything if the device did not have this power-save
feature).

I agree with Jens, makes a lot of sense to start with this feature
'off'.

To 'advertise' the feature, maybe make the feature a new selection in
Kconfig?  Example, initially make it "EXPERIMENTAL", and later when
more devices implement this feature it can be integrated more tightly
into the NVMe solution and default to on.

[toc] | [prev] | [next] | [standalone]

#1489610

On Thu, Sep 22, 2016 at 02:33:36PM -0700, J Freyensee wrote:
> ...and some SSDs don't even support this feature yet, so the number of
> different NVMe devices available to test initially will most likely be
> small (like the Fultondales I have, all I could check is to see if the
> code broke anything if the device did not have this power-save
> feature).
> 
> I agree with Jens, makes a lot of sense to start with this feature
> 'off'.
> 
> To 'advertise' the feature, maybe make the feature a new selection in
> Kconfig?  Example, initially make it "EXPERIMENTAL", and later when
> more devices implement this feature it can be integrated more tightly
> into the NVMe solution and default to on.

Should we just leave the kernel out of this then? I bet we could script
this feature in user space.

[toc] | [prev] | [next] | [standalone]

#1489613

On Thu, Sep 22, 2016 at 2:33 PM, J Freyensee
<james_p_freyensee@linux.intel.com> wrote:
> On Thu, 2016-09-22 at 14:43 -0600, Jens Axboe wrote:
>> On 09/22/2016 02:11 PM, Andy Lutomirski wrote:
>> >
>> > On Thu, Sep 22, 2016 at 7:23 AM, Jens Axboe <axboe@fb.com> wrote:
>> > >
>> > >
>> > > On 09/16/2016 12:16 PM, Andy Lutomirski wrote:
>> > > >
>> > > >
>> > > > Hi all-
>> > > >
>> > > > Here's v4 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.
>> > >
>> > >
>> > > The only thing I don't like about this is the fact that's it's a
>> > > driver private thing. Similar to ALPM on SATA, it's yet another
>> > > knob that needs to be set. It we put it somewhere generic, then
>> > > at least we could potentially use it in a generic fashion.
>> >
>> > Agreed.  I'm hoping to hear back from Rafael soon about the
>> > dev_pm_qos
>> > thing.
>> >
>> > >
>> > >
>> > > Additionally, it should not be on by default.
>> >
>> > I think I disagree with this.  Since we don't have anything like
>> > laptop-mode AFAIK, I think we do want it on by default.  For the
>> > server workloads that want to consume more idle power for faster
>> > response when idle, I think the servers should be willing to make
>> > this
>> > change, just like they need to disable overly deep C states, etc.
>> > (Admittedly, unifying the configuration would be nice.)
>>
>> I can see two reasons why we don't want it the default:
>>
>> 1) Changes like this has a tendency to cause issues on various types
>> of
>> hardware. How many NVMe devices have you tested this on? ALPM on SATA
>> had a lot of initial problems, where slowed down some SSDs unberably.

I'm reasonably optimistic that the NVMe situation will be a lot better
for a couple of reasons:

1. There's only one player involved.  With ALPM, the controller and
the drive need to cooperate on entering and leaving various idle
states.  With NVMe, the controller *is* the drive, so there's no issue
where a drive manufacturer might not have tested with the relevant
controller or vice versa.

2. Windows appears to use it.  I haven't tested directly, but the
Internet seems to think that Windows uses APST and maybe even manual
state transitions, and that NVMe power states are even mandatory for
Connected Standby logo compliance.

3. The feature is new.  NVMe 1.0 didn't support APST at all, so the
driver is unlikely to cause problems with older drivers.

>
> ...and some SSDs don't even support this feature yet, so the number of
> different NVMe devices available to test initially will most likely be
> small (like the Fultondales I have, all I could check is to see if the
> code broke anything if the device did not have this power-save
> feature).
>
> I agree with Jens, makes a lot of sense to start with this feature
> 'off'.
>
> To 'advertise' the feature, maybe make the feature a new selection in
> Kconfig?  Example, initially make it "EXPERIMENTAL", and later when
> more devices implement this feature it can be integrated more tightly
> into the NVMe solution and default to on.
>

How about having a config option that's "default n" that changes the
default?  I could also add a log message when APST is first enabled on
a device to make it easier to notice a change.

--Andy

[toc] | [prev] | [next] | [standalone]

#1490505

Jens,

can we at least get patches 1 and 2 in while pondering the fate
of the right interface for patch 3?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web