nvme: Implement Enhanced Command Retry

PATCH New Keith Busch 2 participants 4 messages Nov 27, 2018 NVMe

Share & tools

Keith Busch

author

2018-11-27 15:37 UTC #0 lore raw

PATCH 1/1 nvme: Implement Enhanced Command Retry

A controller may have an internal state that is not able to successfully process commands for a short duration. In such states, an immediate command requeue is expected to fail. The driver may exceed its max retry count, which permanently ends the command in failure when the same command would succeed after waiting for the controller to be ready.

NVMe ratified TP 4033 provides a delay hint in the completion status code for failed commands. Implement the retry delay based on the command completion status and the controller's requested delay.

Note that requeued commands are handled per request_queue, not per individual request. If multiple commands fail, the controller should consistently report the desired delay time for retryable commands in all CQEs, otherwise the requeue list may be kicked too soon.

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

---
 drivers/nvme/host/core.c | 37 ++++++++++++++++++++++++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 include/linux/nvme.h     | 17 ++++++++++++++++-
 3 files changed, 53 insertions(+), 2 deletions(-)

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

index c418b7a347e0..7397cb48db70 100644
@@ -261,8 +261,16 @@ void nvme_complete_rq(struct request *req)
 		}
 
 		if (!blk_queue_dying(req->q)) {
+			unsigned long delay = 0;
+			struct nvme_ns *ns = req->q->queuedata;
+			u16 crd = (nvme_req(req)->status & NVME_SC_CRD) >> 11;
+
+			if (crd && ns)
+				delay = ns->ctrl->crdt[crd - 1] * 100;
+
 			nvme_req(req)->retries++;
-			blk_mq_requeue_request(req, true);
+			blk_mq_requeue_request(req, false);
+			blk_mq_delay_kick_requeue_list(req->q, delay);
 			return;
 		}
 	}

@@ -1883,6 +1891,25 @@ static int nvme_configure_timestamp(struct nvme_ctrl *ctrl)
 	return ret;
 }
 
+static int nvme_configure_acre(struct nvme_ctrl *ctrl)
+{
+	struct nvme_feat_host_behavior *host;
+	int ret;
+
+	if (!ctrl->crdt[0])
+		return 0;
+
+	host = kzalloc(sizeof(*host), GFP_KERNEL);
+	if (!host)
+		return 0;
+
+	host->acre = NVME_ENABLE_ACRE;
+	ret = nvme_set_features(ctrl, NVME_FEAT_HOST_BEHAVIOR, 0,
+				host, sizeof(*host), NULL);
+	kfree(host);
+	return ret;
+}
+
 static int nvme_configure_apst(struct nvme_ctrl *ctrl)
 {
 	/*

@@ -2404,6 +2431,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 		ctrl->quirks &= ~NVME_QUIRK_NO_DEEPEST_PS;
 	}
 
+	ctrl->crdt[0] = le16_to_cpu(id->crdt1);
+	ctrl->crdt[1] = le16_to_cpu(id->crdt2);
+	ctrl->crdt[2] = le16_to_cpu(id->crdt3);
+
 	ctrl->oacs = le16_to_cpu(id->oacs);
 	ctrl->oncs = le16_to_cpup(&id->oncs);
 	ctrl->oaes = le32_to_cpu(id->oaes);

@@ -2504,6 +2535,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	if (ret < 0)
 		return ret;
 
+	ret = nvme_configure_acre(ctrl);
+	if (ret < 0)
+		return ret;
+
 	ctrl->identified = true;
 
 	return 0;

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

index 27663ce3044e..51063dbf08bf 100644
@@ -181,6 +181,7 @@ struct nvme_ctrl {
 	u32 page_size;
 	u32 max_hw_sectors;
 	u32 max_segments;
+	u16 crdt[3];
 	u16 oncs;
 	u16 oacs;
 	u16 nssa;

include/linux/nvme.h +16 -1

index c03973c215ad..88812cb15be0 100644
@@ -223,7 +223,11 @@ struct nvme_id_ctrl {
 	__le32			rtd3e;
 	__le32			oaes;
 	__le32			ctratt;
-	__u8			rsvd100[156];
+	__u8			rsvd100[28];
+	__le16			crdt1;
+	__le16			crdt2;
+	__le16			crdt3;
+	__u8			rsvd134[122];
 	__le16			oacs;
 	__u8			acl;
 	__u8			aerl;

@@ -756,6 +760,15 @@ enum {
 	NVME_HOST_MEM_RETURN	= (1 << 1),
 };
 
+struct nvme_feat_host_behavior {
+	__u8 acre;
+	__u8 resv1[511];
+};
+
+enum {
+	NVME_ENABLE_ACRE	= 1,
+};
+
 /* Admin commands */
 
 enum nvme_admin_opcode {

@@ -810,6 +823,7 @@ enum {
 	NVME_FEAT_RRL		= 0x12,
 	NVME_FEAT_PLM_CONFIG	= 0x13,
 	NVME_FEAT_PLM_WINDOW	= 0x14,
+	NVME_FEAT_HOST_BEHAVIOR	= 0x16,
 	NVME_FEAT_SW_PROGRESS	= 0x80,
 	NVME_FEAT_HOST_ID	= 0x81,
 	NVME_FEAT_RESV_MASK	= 0x82,

@@ -1265,6 +1279,7 @@ enum {
 	NVME_SC_ANA_TRANSITION		= 0x303,
 	NVME_SC_HOST_PATH_ERROR		= 0x370,
 
+	NVME_SC_CRD			= 0x1800,
 	NVME_SC_DNR			= 0x4000,
 };

Signature · 1 line

2.14.4

Christoph Hellwig

maintainer

2018-11-27 15:46 UTC #1 lore raw

PATCH 1/1 nvme: Implement Enhanced Command Retry

drivers/nvme/host/core.c void nvme_complete_rq(struct request *req) · line 269

Quoted 7 lines · Keith Busch

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

 		if (!blk_queue_dying(req->q)) {
+			unsigned long delay = 0;
+			struct nvme_ns *ns = req->q->queuedata;
+			u16 crd = (nvme_req(req)->status & NVME_SC_CRD) >> 11;
+
+			if (crd && ns)
+				delay = ns->ctrl->crdt[crd - 1] * 100;

Do we need bounce checking for crd here?

drivers/nvme/host/core.c void nvme_complete_rq(struct request *req) · line 274

Quoted 6 lines

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

+
 			nvme_req(req)->retries++;
-			blk_mq_requeue_request(req, true);
+			blk_mq_requeue_request(req, false);
+			blk_mq_delay_kick_requeue_list(req->q, delay);
 			return;

I also think it would be nice to split out this code block into a little nvme_retry_req ala nvme_failover_req.

Otherwise this looks sensible to me.

Keith Busch

author

2018-11-27 15:50 UTC #2 lore raw

PATCH 1/1 nvme: Implement Enhanced Command Retry

Quoted 10 lines · Christoph Hellwig

drivers/nvme/host/core.c void nvme_complete_rq(struct request *req) · line 269
Quoted 7 lines · Keith Busch
drivers/nvme/host/core.c +6 -0
 		if (!blk_queue_dying(req->q)) {
+			unsigned long delay = 0;
+			struct nvme_ns *ns = req->q->queuedata;
+			u16 crd = (nvme_req(req)->status & NVME_SC_CRD) >> 11;
+
+			if (crd && ns)
+				delay = ns->ctrl->crdt[crd - 1] * 100;
Do we need bounce checking for crd here?

I assume you mean bounds, in which case it should not be necessary since the result of the mask and shift can't be > 3, which is the size of the crdt array. I can add the check if not having it is visually alarming.

Quoted 9 lines

drivers/nvme/host/core.c void nvme_complete_rq(struct request *req) · line 274
Quoted 6 lines
drivers/nvme/host/core.c +3 -1
+
 			nvme_req(req)->retries++;
-			blk_mq_requeue_request(req, true);
+			blk_mq_requeue_request(req, false);
+			blk_mq_delay_kick_requeue_list(req->q, delay);
 			return;
I also think it would be nice to split out this code block into a little nvme_retry_req ala nvme_failover_req.

Sounds good, will do.

Quoted 1 line

Otherwise this looks sensible to me.

Christoph Hellwig

maintainer

2018-11-27 16:14 UTC #3 lore raw

PATCH 1/1 nvme: Implement Enhanced Command Retry

Quoted 12 lines · Keith Busch

Quoted 10 lines · Christoph Hellwig
drivers/nvme/host/core.c void nvme_complete_rq(struct request *req) · line 269
Quoted 7 lines · Keith Busch
drivers/nvme/host/core.c +6 -0
 		if (!blk_queue_dying(req->q)) {
+			unsigned long delay = 0;
+			struct nvme_ns *ns = req->q->queuedata;
+			u16 crd = (nvme_req(req)->status & NVME_SC_CRD) >> 11;
+
+			if (crd && ns)
+				delay = ns->ctrl->crdt[crd - 1] * 100;
Do we need bounce checking for crd here?

Quoted 3 lines

I assume you mean bounds, in which case it should not be necessary since the result of the mask and shift can't be > 3, which is the size of the crdt array. I can add the check if not having it is visually alarming.

True, I don't think we'll need a check. Maybe at best a comment.