nvme: Honor RTD3 Entry Latency for shutdowns

PATCH New Martin K. Petersen 5 participants 17 messages Aug 16, 2017 – Aug 28, 2017 NVMe

Share & tools

Martin K. Petersen

author

2017-08-16 19:55 UTC #0 lore raw

PATCH 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

If an NVMe controller reports RTD3 Entry Latency bigger than shutdown_timeout, use that value to set the shutdown timer. Otherwise fall back to the module parameter which defaults to 5 seconds.

Signed-off-by Martin K. Petersen <martin.petersen at oracle.com>

---
 drivers/nvme/host/core.c | 18 +++++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 18 insertions(+), 1 deletion(-)

drivers/nvme/host/core.c +17 -1

index b7dfbbeccb47..ed8f286f24bd 100644
@@ -1465,7 +1465,7 @@ EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 {
-	unsigned long timeout = jiffies + (shutdown_timeout * HZ);
+	unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
 	u32 csts;
 	int ret;

@@ -1833,6 +1833,22 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->sgls = le32_to_cpu(id->sgls);
 	ctrl->kas = le16_to_cpu(id->kas);
 
+	if (ctrl->vs >= NVME_VS(1, 2, 0)) {
+		u32 transition_time = le32_to_cpu(id->rtd3e);
+
+		do_div(transition_time, 1000000); /* us -> s */
+
+		if (transition_time > shutdown_timeout) {
+			ctrl->shutdown_timeout = transition_time;
+			dev_warn(ctrl->device,
+				 "Shutdown timeout set to %u seconds\n",
+				 ctrl->shutdown_timeout);
+		}
+	}
+
+	if (!ctrl->shutdown_timeout)
+		ctrl->shutdown_timeout = shutdown_timeout;
+
 	ctrl->npss = id->npss;
 	ctrl->apsta = id->apsta;
 	prev_apst_enabled = ctrl->apst_enabled;

drivers/nvme/host/nvme.h +1 -0

index 2c8a02be46fd..9b959ee18cb6 100644
@@ -162,6 +162,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
 	unsigned long quirks;

Signature · 1 line

2.13.0

Scott Bauer

2017-08-16 20:25 UTC #1 lore raw

PATCH 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

Quoted 3 lines · Martin K. Petersen

If an NVMe controller reports RTD3 Entry Latency bigger than shutdown_timeout, use that value to set the shutdown timer. Otherwise fall back to the module parameter which defaults to 5 seconds.

My only concern is if this value extends past the DPM_WATCHDOG timeout value. If it does we're going to end up panic()ing the kernel during suspends. If others agree I think we should set it to the minimum value of DPM_WATCHDOG_TIMEOUT and shutdown_timeout.

Martin K. Petersen

author

2017-08-16 20:49 UTC #2 lore raw

PATCH 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

Scott,

Quoted 4 lines

My only concern is if this value extends past the DPM_WATCHDOG timeout value. If it does we're going to end up panic()ing the kernel during suspends. If others agree I think we should set it to the minimum value of DPM_WATCHDOG_TIMEOUT and shutdown_timeout.

I don't have a problem with that. Keith?

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Keith Busch

2017-08-17 15:01 UTC #3 lore raw

PATCH 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

Quoted 9 lines · Martin K. Petersen

Scott,
Quoted 4 lines
My only concern is if this value extends past the DPM_WATCHDOG timeout value. If it does we're going to end up panic()ing the kernel during suspends. If others agree I think we should set it to the minimum value of DPM_WATCHDOG_TIMEOUT and shutdown_timeout.
I don't have a problem with that. Keith?

Sounds good to me as well.

Martin K. Petersen

author

2017-08-17 17:06 UTC #4 lore raw

PATCH 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

Keith,

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
My only concern is if this value extends past the DPM_WATCHDOG timeout value. If it does we're going to end up panic()ing the kernel during suspends. If others agree I think we should set it to the minimum value of DPM_WATCHDOG_TIMEOUT and shutdown_timeout.
I don't have a problem with that. Keith?
Sounds good to me as well.

Hrm, this gets pretty messy.

DPM_WATCHDOG_TIMEOUT is buried deep in the config options (hidden behind PM_DEBUG and EXPERT). And as a result doesn't appear to be enabled in most common kernel configs. It also isn't exported from the PM subsystem in a generic way.

In addition, I'm not sure what we'd do in case a device demands a 10 second shutdown time but the user has the kernel configured with a 2 second DPM watchdog. Then what? The device is still going to take 10 seconds to complete the shutdown request.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Scott Bauer

2017-08-17 17:42 UTC #5 lore raw

PATCH 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

Quoted 19 lines · Martin K. Petersen

Keith,
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
My only concern is if this value extends past the DPM_WATCHDOG timeout value. If it does we're going to end up panic()ing the kernel during suspends. If others agree I think we should set it to the minimum value of DPM_WATCHDOG_TIMEOUT and shutdown_timeout.
I don't have a problem with that. Keith?
Sounds good to me as well.
Hrm, this gets pretty messy.
DPM_WATCHDOG_TIMEOUT is buried deep in the config options (hidden behind PM_DEBUG and EXPERT). And as a result doesn't appear to be enabled in most common kernel configs. It also isn't exported from the PM subsystem in a generic way.

Quoted 4 lines

In addition, I'm not sure what we'd do in case a device demands a 10 second shutdown time but the user has the kernel configured with a 2 second DPM watchdog. Then what? The device is still going to take 10 seconds to complete the shutdown request.

ugh, I thought it would be as easy as if (IS_ENABLED(CONFIG_DPM_WATCHDOG)) but I guess since that's a runtime check GCC cant remove the scope below, since it's not known at compilation time.

We wont be able to fix this without some gross ifdefs/ifndefs.

I guess we can do a dev_warn() or something if the shutdown time is > than some threshold instead? That way if people are getting sporatic panics on suspends we can see that warning and fix it from there?

Martin K. Petersen

author

2017-08-23 22:32 UTC #6 lore raw

PATCH v2 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

If an NVMe v1.2+ compliant controller reports RTD3 Entry Latency larger than shutdown_timeout, up to a maximum of 60 seconds, use that value to set the shutdown timer. Otherwise fall back to the module parameter which defaults to 5 seconds.

Signed-off-by Martin K. Petersen <martin.petersen at oracle.com>

---
 drivers/nvme/host/core.c | 19 ++++++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 19 insertions(+), 1 deletion(-)

drivers/nvme/host/core.c +18 -1

index b7dfbbeccb47..13cc601012b0 100644
@@ -1465,7 +1465,7 @@ EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 {
-	unsigned long timeout = jiffies + (shutdown_timeout * HZ);
+	unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
 	u32 csts;
 	int ret;

@@ -1833,6 +1833,23 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->sgls = le32_to_cpu(id->sgls);
 	ctrl->kas = le16_to_cpu(id->kas);
 
+	if (ctrl->vs >= NVME_VS(1, 2, 0)) {
+		u32 transition_time = le32_to_cpu(id->rtd3e);
+
+		do_div(transition_time, 1000000); /* us -> s */
+		clamp(transition_time, shutdown_timeout, 60); /* max 60s */
+
+		if (transition_time != shutdown_timeout) {
+			ctrl->shutdown_timeout = transition_time;
+			dev_warn(ctrl->device,
+				 "Shutdown timeout set to %u seconds\n",
+				 ctrl->shutdown_timeout);
+		}
+	}
+
+	if (!ctrl->shutdown_timeout)
+		ctrl->shutdown_timeout = shutdown_timeout;
+
 	ctrl->npss = id->npss;
 	ctrl->apsta = id->apsta;
 	prev_apst_enabled = ctrl->apst_enabled;

drivers/nvme/host/nvme.h +1 -0

index 2c8a02be46fd..9b959ee18cb6 100644
@@ -162,6 +162,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
 	unsigned long quirks;

Signature · 1 line

2.14.1

Christoph Hellwig

2017-08-24 09:02 UTC #7 lore raw

PATCH v2 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

Quoted 4 lines · Martin K. Petersen

If an NVMe v1.2+ compliant controller reports RTD3 Entry Latency larger than shutdown_timeout, up to a maximum of 60 seconds, use that value to set the shutdown timer. Otherwise fall back to the module parameter which defaults to 5 seconds.

This generally looks fine, but NVMe allows TPs and thus new fields to be implemented even against older spec versions, so we should avoid version tests wherever possible.

drivers/nvme/host/core.c int nvme_init_identify(struct nvme_ctrl *ctrl) · line 1837

Quoted 2 lines

drivers/nvme/host/core.c +2 -0

+	if (ctrl->vs >= NVME_VS(1, 2, 0)) {
+		u32 transition_time = le32_to_cpu(id->rtd3e);

So instead of the version check here I'd check for a non-zero RTD3E field instead.

Martin K. Petersen

author

2017-08-25 02:26 UTC #8 lore raw

PATCH v2 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

Christoph,

Quoted 3 lines

This generally looks fine, but NVMe allows TPs and thus new fields to be implemented even against older spec versions, so we should avoid version tests wherever possible.

Fine with me. I was just mirroring the other version conditionals.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Martin K. Petersen

author

2017-08-25 02:26 UTC #9 lore raw

PATCH v3 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

In reply to Christoph Hellwig

If an NVMe controller reports RTD3 Entry Latency larger than shutdown_timeout, up to a maximum of 60 seconds, use that value to set the shutdown timer. Otherwise fall back to the module parameter which defaults to 5 seconds.

Signed-off-by Martin K. Petersen <martin.petersen at oracle.com>

---
 drivers/nvme/host/core.c | 16 +++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

drivers/nvme/host/core.c +15 -1

index b7dfbbeccb47..775b702f9c91 100644
@@ -1465,7 +1465,7 @@ EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 {
-	unsigned long timeout = jiffies + (shutdown_timeout * HZ);
+	unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
 	u32 csts;
 	int ret;

@@ -1765,6 +1765,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	int ret, page_shift;
 	u32 max_hw_sectors;
 	bool prev_apst_enabled;
+	u32 transition_time;
 
 	ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs);
 	if (ret) {

@@ -1833,6 +1834,19 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->sgls = le32_to_cpu(id->sgls);
 	ctrl->kas = le16_to_cpu(id->kas);
 
+	transition_time = le32_to_cpu(id->rtd3e);
+	if (transition_time) {
+		do_div(transition_time, 1000000); /* us -> s */
+		clamp(transition_time, shutdown_timeout, 60); /* max 60s */
+		ctrl->shutdown_timeout = transition_time;
+
+		if (transition_time != shutdown_timeout)
+			dev_warn(ctrl->device,
+				 "Shutdown timeout set to %u seconds\n",
+				 ctrl->shutdown_timeout);
+	} else
+		ctrl->shutdown_timeout = shutdown_timeout;
+
 	ctrl->npss = id->npss;
 	ctrl->apsta = id->apsta;
 	prev_apst_enabled = ctrl->apst_enabled;

drivers/nvme/host/nvme.h +1 -0

index 2c8a02be46fd..9b959ee18cb6 100644
@@ -162,6 +162,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
 	unsigned long quirks;

Signature · 1 line

2.14.1

Christoph Hellwig

2017-08-25 07:41 UTC #10 lore raw

PATCH v3 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

Quoted 6 lines · Martin K. Petersen

If an NVMe controller reports RTD3 Entry Latency larger than shutdown_timeout, up to a maximum of 60 seconds, use that value to set the shutdown timer. Otherwise fall back to the module parameter which defaults to 5 seconds.
Signed-off-by Martin K. Petersen <martin.petersen at oracle.com>

Do you think this is urgent enough for 4.13?

Martin K. Petersen

author

2017-08-25 14:15 UTC #11 lore raw

PATCH v3 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

Christoph,

Quoted 9 lines · Martin K. Petersen

Quoted 6 lines
If an NVMe controller reports RTD3 Entry Latency larger than shutdown_timeout, up to a maximum of 60 seconds, use that value to set the shutdown timer. Otherwise fall back to the module parameter which defaults to 5 seconds.
Signed-off-by Martin K. Petersen <martin.petersen at oracle.com>
Do you think this is urgent enough for 4.13?

4.14 should be fine.

Thanks!

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Keith Busch

2017-08-25 19:56 UTC #12 lore raw

PATCH v3 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

In reply to Martin K. Petersen

drivers/nvme/host/core.c int nvme_init_identify(struct nvme_ctrl *ctrl) · line 1841

Quoted 5 lines · Martin K. Petersen

drivers/nvme/host/core.c +5 -0

+	transition_time = le32_to_cpu(id->rtd3e);
+	if (transition_time) {
+		do_div(transition_time, 1000000); /* us -> s */
+		clamp(transition_time, shutdown_timeout, 60); /* max 60s */
+		ctrl->shutdown_timeout = transition_time;

My first experience with the kernel's 'clamp', so I had to look up that macro. The above does not appear to fit the usage since 'clamp' returns a value rather than directly clamp the parameter; I think it should be:

	transition_time = clamp(transition_time, shutdown_timeout, 60);

Martin K. Petersen

author

2017-08-25 23:14 UTC #13 lore raw

PATCH v4 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

Signed-off-by Martin K. Petersen <martin.petersen at oracle.com>

---
 drivers/nvme/host/core.c | 16 +++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

drivers/nvme/host/core.c +15 -1

index b7dfbbeccb47..ea7bf12b563c 100644
@@ -1465,7 +1465,7 @@ EXPORT_SYMBOL_GPL(nvme_enable_ctrl);
 
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl)
 {
-	unsigned long timeout = jiffies + (shutdown_timeout * HZ);
+	unsigned long timeout = jiffies + (ctrl->shutdown_timeout * HZ);
 	u32 csts;
 	int ret;

@@ -1765,6 +1765,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	int ret, page_shift;
 	u32 max_hw_sectors;
 	bool prev_apst_enabled;
+	u32 transition_time;
 
 	ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs);
 	if (ret) {

@@ -1833,6 +1834,19 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->sgls = le32_to_cpu(id->sgls);
 	ctrl->kas = le16_to_cpu(id->kas);
 
+	transition_time = le32_to_cpu(id->rtd3e);
+	if (transition_time) {
+		do_div(transition_time, 1000000); /* us -> s */
+		ctrl->shutdown_timeout = clamp_t(unsigned int, transition_time,
+						 shutdown_timeout, 60);
+
+		if (ctrl->shutdown_timeout != shutdown_timeout)
+			dev_warn(ctrl->device,
+				 "Shutdown timeout set to %u seconds\n",
+				 ctrl->shutdown_timeout);
+	} else
+		ctrl->shutdown_timeout = shutdown_timeout;
+
 	ctrl->npss = id->npss;
 	ctrl->apsta = id->apsta;
 	prev_apst_enabled = ctrl->apst_enabled;

drivers/nvme/host/nvme.h +1 -0

index 2c8a02be46fd..9b959ee18cb6 100644
@@ -162,6 +162,7 @@ struct nvme_ctrl {
 	u16 kas;
 	u8 npss;
 	u8 apsta;
+	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
 	unsigned long quirks;

Signature · 1 line

2.14.1

Sagi Grimberg

maintainer

2017-08-28 06:05 UTC #14 lore raw

PATCH v4 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

Keith Busch

2017-08-28 14:22 UTC #15 lore raw

PATCH v4 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

In reply to Martin K. Petersen

Quoted 6 lines · Martin K. Petersen

If an NVMe controller reports RTD3 Entry Latency larger than shutdown_timeout, up to a maximum of 60 seconds, use that value to set the shutdown timer. Otherwise fall back to the module parameter which defaults to 5 seconds.
Signed-off-by Martin K. Petersen <martin.petersen at oracle.com>

Looks good,

Reviewed-by Keith Busch <keith.busch at intel.com>

Martin K. Petersen

author

2017-08-25 23:16 UTC #16 lore raw

PATCH v3 1/1 nvme: Honor RTD3 Entry Latency for shutdowns

In reply to Keith Busch

Keith,

Quoted 6 lines

My first experience with the kernel's 'clamp', so I had to look up that macro. The above does not appear to fit the usage since 'clamp' returns a value rather than directly clamp the parameter; I think it should be:
	transition_time = clamp(transition_time, shutdown_timeout, 60);

Ah, my brain was seduced by the do_div() above.

Fixed and tested in qemu.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering