Skip to contentSkip to conversation

refactor nvme-pci cq processing

PATCH New Sagi Grimberg 3 participants 14 messages Jun 18, 2017 – Jun 19, 2017 NVMe

lore
Share & tools
lore

Sagi Grimberg

author maintainer
#0 lore raw

v2 cover refactor nvme-pci cq processing

Some preps for my non-selective polling patches that got left out. I should revisit those soon.

Changes from v1:
- separated patch #1 into:
- introduce nvme_ring_cq_doorbell
- factor out cqe handling
- factor out the cqe read mechanics
- dropped the budget patch
- got rid of __nvme_process_cq

Sagi Grimberg (4):
nvme-pci: Introduce nvme_ring_cq_doorbell
nvme-pci: factor out cqe handling into a dedicated routine
nvme-pci: factor out the cqe reading mechanics from __nvme_process_cq
nvme-pci: open-code polling logic in nvme_poll

 drivers/nvme/host/pci.c | 130 ++++++++++++++++++++++++++++--------------------
 1 file changed, 76 insertions(+), 54 deletions(-)
Signature · 1 line
2.7.4
author maintainer
#1 lore raw

PATCH v2 1/4 nvme-pci: Introduce nvme_ring_cq_doorbell

Nice abstraction of the actual mechanics of how to do it. Note the change that we call it after we assign nvmeq->cq_head to avoid passing it.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/pci.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/nvme/host/pci.c +13 -4
index 0f09a2d5cf7a..042cfe5ef8e9 100644
@@ -730,6 +730,17 @@ static inline bool nvme_cqe_valid(struct nvme_queue *nvmeq, u16 head,
 	return (le16_to_cpu(nvmeq->cqes[head].status) & 1) == phase;
 }
 
+static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq)
+{
+	u16 head = nvmeq->cq_head;
+
+	if (likely(nvmeq->cq_vector >= 0)) {
+		if (nvme_dbbuf_update_and_check_event(head, nvmeq->dbbuf_cq_db,
+						      nvmeq->dbbuf_cq_ei))
+			writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
+	}
+}
+
 static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
 {
 	u16 head, phase;

@@ -776,13 +787,11 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
 	if (head == nvmeq->cq_head && phase == nvmeq->cq_phase)
 		return;
 
-	if (likely(nvmeq->cq_vector >= 0))
-		if (nvme_dbbuf_update_and_check_event(head, nvmeq->dbbuf_cq_db,
-						      nvmeq->dbbuf_cq_ei))
-			writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
 	nvmeq->cq_head = head;
 	nvmeq->cq_phase = phase;
 
+	nvme_ring_cq_doorbell(nvmeq);
+
 	nvmeq->cqe_seen = 1;
 }
Signature · 1 line
2.7.4
maintainer
#2 lore raw

PATCH v2 1/4 nvme-pci: Introduce nvme_ring_cq_doorbell

Quoted 3 lines · Sagi Grimberg

Nice abstraction of the actual mechanics of how to do it. Note the change that we call it after we assign nvmeq->cq_head to avoid passing it.

Looks fine,

Reviewed-by Christoph Hellwig <hch at lst.de>
#3 lore raw

PATCH v2 1/4 nvme-pci: Introduce nvme_ring_cq_doorbell

In reply to Sagi Grimberg
Quoted 5 lines · Sagi Grimberg

Nice abstraction of the actual mechanics of how to do it. Note the change that we call it after we assign nvmeq->cq_head to avoid passing it.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

Looks good.

Reviewed-by Keith Busch <keith.busch at intel.com>
author maintainer
#4 lore raw

PATCH v2 2/4 nvme-pci: factor out cqe handling into a dedicated routine

In reply to Sagi Grimberg

Makes the code slightly more readable.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/pci.c | 53 ++++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 23 deletions(-)
drivers/nvme/host/pci.c +30 -23
index 042cfe5ef8e9..26eb1743f8bc 100644
@@ -741,6 +741,35 @@ static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq)
 	}
 }
 
+static inline void nvme_handle_cqe(struct nvme_queue *nvmeq,
+		struct nvme_completion *cqe)
+{
+	struct request *req;
+
+	if (unlikely(cqe->command_id >= nvmeq->q_depth)) {
+		dev_warn(nvmeq->dev->ctrl.device,
+			"invalid id %d completed on queue %d\n",
+			cqe->command_id, le16_to_cpu(cqe->sq_id));
+		return;
+	}
+
+	/*
+	 * AEN requests are special as they don't time out and can
+	 * survive any kind of queue freeze and often don't respond to
+	 * aborts.  We don't even bother to allocate a struct request
+	 * for them but rather special case them here.
+	 */
+	if (unlikely(nvmeq->qid == 0 &&
+			cqe->command_id >= NVME_AQ_BLKMQ_DEPTH)) {
+		nvme_complete_async_event(&nvmeq->dev->ctrl,
+				cqe->status, &cqe->result);
+		return;
+	}
+
+	req = blk_mq_tag_to_rq(*nvmeq->tags, cqe->command_id);
+	nvme_end_request(req, cqe->status, cqe->result);
+}
+
 static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
 {
 	u16 head, phase;

@@ -750,7 +779,6 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
 
 	while (nvme_cqe_valid(nvmeq, head, phase)) {
 		struct nvme_completion cqe = nvmeq->cqes[head];
-		struct request *req;
 
 		if (++head == nvmeq->q_depth) {
 			head = 0;

@@ -760,28 +788,7 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
 		if (tag && *tag == cqe.command_id)
 			*tag = -1;
 
-		if (unlikely(cqe.command_id >= nvmeq->q_depth)) {
-			dev_warn(nvmeq->dev->ctrl.device,
-				"invalid id %d completed on queue %d\n",
-				cqe.command_id, le16_to_cpu(cqe.sq_id));
-			continue;
-		}
-
-		/*
-		 * AEN requests are special as they don't time out and can
-		 * survive any kind of queue freeze and often don't respond to
-		 * aborts.  We don't even bother to allocate a struct request
-		 * for them but rather special case them here.
-		 */
-		if (unlikely(nvmeq->qid == 0 &&
-				cqe.command_id >= NVME_AQ_BLKMQ_DEPTH)) {
-			nvme_complete_async_event(&nvmeq->dev->ctrl,
-					cqe.status, &cqe.result);
-			continue;
-		}
-
-		req = blk_mq_tag_to_rq(*nvmeq->tags, cqe.command_id);
-		nvme_end_request(req, cqe.status, cqe.result);
+		nvme_handle_cqe(nvmeq, &cqe);
 	}
 
 	if (head == nvmeq->cq_head && phase == nvmeq->cq_phase)
Signature · 1 line
2.7.4
maintainer
#5 lore raw

PATCH v2 2/4 nvme-pci: factor out cqe handling into a dedicated routine

Quoted 23 lines · Sagi Grimberg

Makes the code slightly more readable.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/pci.c | 53 ++++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 23 deletions(-)
drivers/nvme/host/pci.c +8 -0
index 042cfe5ef8e9..26eb1743f8bc 100644
@@ -741,6 +741,35 @@ static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq)
 	}
 }
 
+static inline void nvme_handle_cqe(struct nvme_queue *nvmeq,
+		struct nvme_completion *cqe)
+{
+	struct request *req;
+
+	if (unlikely(cqe->command_id >= nvmeq->q_depth)) {
+		dev_warn(nvmeq->dev->ctrl.device,
+			"invalid id %d completed on queue %d\n",

drivers/nvme/host/pci.c static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq) · line 761

Quoted 10 lines
drivers/nvme/host/pci.c +10 -0
+			cqe->command_id, le16_to_cpu(cqe->sq_id));
+		return;
+	}
+
+	/*
+	 * AEN requests are special as they don't time out and can
+	 * survive any kind of queue freeze and often don't respond to
+	 * aborts.  We don't even bother to allocate a struct request
+	 * for them but rather special case them here.
+	 */

Lots more space for the comments now, they could be reflowed. Nothing worth resending for, though.

Reviewed-by Christoph Hellwig <hch at lst.de>
#6 lore raw

PATCH v2 2/4 nvme-pci: factor out cqe handling into a dedicated routine

In reply to Sagi Grimberg
Quoted 3 lines · Sagi Grimberg

Makes the code slightly more readable.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

Looks good.

Reviewed-by Keith Busch <keith.busch at intel.com>
author maintainer
#7 lore raw

PATCH v2 3/4 nvme-pci: factor out the cqe reading mechanics from __nvme_process_cq

In reply to Sagi Grimberg

Also, maintain a consumed counter to rely on for doorbell and cqe_seen update instead of directly relying on the cq head and phase.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/pci.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)
drivers/nvme/host/pci.c +26 -22
index 26eb1743f8bc..d309b6c90511 100644
@@ -770,36 +770,40 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq,
 	nvme_end_request(req, cqe->status, cqe->result);
 }
 
-static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
+static inline bool nvme_read_cqe(struct nvme_queue *nvmeq,
+		struct nvme_completion *cqe)
 {
-	u16 head, phase;
-
-	head = nvmeq->cq_head;
-	phase = nvmeq->cq_phase;
-
-	while (nvme_cqe_valid(nvmeq, head, phase)) {
-		struct nvme_completion cqe = nvmeq->cqes[head];
+	if (nvme_cqe_valid(nvmeq, nvmeq->cq_head, nvmeq->cq_phase)) {
+		*cqe = nvmeq->cqes[nvmeq->cq_head];
 
-		if (++head == nvmeq->q_depth) {
-			head = 0;
-			phase = !phase;
+		if (++nvmeq->cq_head == nvmeq->q_depth) {
+			nvmeq->cq_head = 0;
+			nvmeq->cq_phase = !nvmeq->cq_phase;
 		}
-
-		if (tag && *tag == cqe.command_id)
-			*tag = -1;
-
-		nvme_handle_cqe(nvmeq, &cqe);
+		return true;
 	}
+	return false;
+}
 
-	if (head == nvmeq->cq_head && phase == nvmeq->cq_phase)
-		return;
+static void __nvme_process_cq(struct nvme_queue *nvmeq, int *tag)
+{
+	struct nvme_completion cqe;
+	int consumed = 0;
 
-	nvmeq->cq_head = head;
-	nvmeq->cq_phase = phase;
+	while (nvme_read_cqe(nvmeq, &cqe)) {
+		nvme_handle_cqe(nvmeq, &cqe);
+		consumed++;
 
-	nvme_ring_cq_doorbell(nvmeq);
+		if (tag && *tag == cqe.command_id) {
+			*tag = -1;
+			break;
+		}
+	}
 
-	nvmeq->cqe_seen = 1;
+	if (consumed) {
+		nvme_ring_cq_doorbell(nvmeq);
+		nvmeq->cqe_seen = 1;
+	}
 }
 
 static void nvme_process_cq(struct nvme_queue *nvmeq)
Signature · 1 line
2.7.4
maintainer
#8 lore raw

PATCH v2 3/4 nvme-pci: factor out the cqe reading mechanics from __nvme_process_cq

Looks good,

Reviewed-by Christoph Hellwig <hch at lst.de>
#9 lore raw

PATCH v2 3/4 nvme-pci: factor out the cqe reading mechanics from __nvme_process_cq

In reply to Sagi Grimberg
Quoted 4 lines · Sagi Grimberg

Also, maintain a consumed counter to rely on for doorbell and cqe_seen update instead of directly relying on the cq head and phase.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

Looks like this particular change failed to produce the best diff. :)

drivers/nvme/host/pci.c static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, · line 799

Quoted 4 lines
drivers/nvme/host/pci.c +3 -1
-	nvme_ring_cq_doorbell(nvmeq);
+		if (tag && *tag == cqe.command_id) {
+			*tag = -1;
+			break;

This 'break' is new. We're already holding the lock, why wouldn't we want to reap all the completions?

author maintainer
#10 lore raw

PATCH v2 3/4 nvme-pci: factor out the cqe reading mechanics from __nvme_process_cq

Quoted 7 lines
Quoted 5 lines

Also, maintain a consumed counter to rely on for doorbell and cqe_seen update instead of directly relying on the cq head and phase.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

Looks like this particular change failed to produce the best diff. :)

:(

Quoted 7 lines

drivers/nvme/host/pci.c static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, · line 799

Quoted 4 lines
drivers/nvme/host/pci.c +3 -1
-	nvme_ring_cq_doorbell(nvmeq);
+		if (tag && *tag == cqe.command_id) {
+			*tag = -1;
+			break;

This 'break' is new. We're already holding the lock, why wouldn't we want to reap all the completions?

Oddly enough I (re)noticed it now too. I guess that my thought was that if we were invoked from ->poll, it means that the user wants the completion asap and willing to put some extra efforts in order to get it faster... So if you think about it, any action you take post finding the completion is not what the user wants...

This absolutely needs to be documented, or actually come as a separate patch... I can also get rid of it if its a dumb idea...

author maintainer
#11 lore raw

PATCH v2 4/4 nvme-pci: open-code polling logic in nvme_poll

In reply to Sagi Grimberg

Given that the code is simple enough it seems better then passing a tag by reference for each call site, also we can now get rid of __nvme_process_cq.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/pci.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)
drivers/nvme/host/pci.c +21 -19
index d309b6c90511..2a9ee769ce9e 100644
@@ -785,7 +785,7 @@ static inline bool nvme_read_cqe(struct nvme_queue *nvmeq,
 	return false;
 }
 
-static void __nvme_process_cq(struct nvme_queue *nvmeq, int *tag)
+static void nvme_process_cq(struct nvme_queue *nvmeq)
 {
 	struct nvme_completion cqe;
 	int consumed = 0;

@@ -793,11 +793,6 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, int *tag)
 	while (nvme_read_cqe(nvmeq, &cqe)) {
 		nvme_handle_cqe(nvmeq, &cqe);
 		consumed++;
-
-		if (tag && *tag == cqe.command_id) {
-			*tag = -1;
-			break;
-		}
 	}
 
 	if (consumed) {

@@ -806,11 +801,6 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, int *tag)
 	}
 }
 
-static void nvme_process_cq(struct nvme_queue *nvmeq)
-{
-	__nvme_process_cq(nvmeq, NULL);
-}
-
 static irqreturn_t nvme_irq(int irq, void *data)
 {
 	irqreturn_t result;

@@ -833,16 +823,28 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
 
 static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag)
 {
-	if (nvme_cqe_valid(nvmeq, nvmeq->cq_head, nvmeq->cq_phase)) {
-		spin_lock_irq(&nvmeq->q_lock);
-		__nvme_process_cq(nvmeq, &tag);
-		spin_unlock_irq(&nvmeq->q_lock);
+	struct nvme_completion cqe;
+	int found = 0, consumed = 0;
 
-		if (tag == -1)
-			return 1;
-	}
+	if (!nvme_cqe_valid(nvmeq, nvmeq->cq_head, nvmeq->cq_phase))
+		return 0;
 
-	return 0;
+	spin_lock_irq(&nvmeq->q_lock);
+	while (nvme_read_cqe(nvmeq, &cqe)) {
+		nvme_handle_cqe(nvmeq, &cqe);
+		consumed++;
+
+		if (tag == cqe.command_id) {
+			found = 1;
+			break;
+		}
+       }
+
+	if (consumed)
+		nvme_ring_cq_doorbell(nvmeq);
+	spin_unlock_irq(&nvmeq->q_lock);
+
+	return found;
 }
 
 static int nvme_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)
Signature · 1 line
2.7.4
maintainer
#12 lore raw

PATCH v2 4/4 nvme-pci: open-code polling logic in nvme_poll

Looks good,

Reviewed-by Christoph Hellwig <hch at lst.de>
#13 lore raw

PATCH v2 4/4 nvme-pci: open-code polling logic in nvme_poll

In reply to Sagi Grimberg
Quoted 5 lines · Sagi Grimberg

Given that the code is simple enough it seems better then passing a tag by reference for each call site, also we can now get rid of __nvme_process_cq.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

Looks good, and the previous patch makes more sense along with this.

Reviewed-by Keith Busch <keith.busch at intelcom>