Skip to contentSkip to conversation

refactor nvme-pci cq processing

PATCH Superseded Sagi Grimberg 2 participants 10 messages Jun 15, 2017 – Jun 18, 2017 NVMe

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Sagi Grimberg

author maintainer
#0 lore raw

cover refactor nvme-pci cq processing

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

Sagi Grimberg (3):
nvme-pci: Split __nvme_process_cq to poll and handle
nvme-pci: Add budget to __nvme_process_cq
nvme-pci: open-code polling logic in nvme_poll

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

PATCH 1/3 nvme-pci: Split __nvme_process_cq to poll and handle

Just some rework to split the logic and make it slightly more readable. This will help us to easily add the irq-poll logic.

Also, introduce nvme_ring_cq_doorbell helper to mask out the cq_vector validity check.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/pci.c | 114 +++++++++++++++++++++++++++++-------------------
 1 file changed, 69 insertions(+), 45 deletions(-)
drivers/nvme/host/pci.c +69 -45
index cf091705b0a9..a99a483a8bb5 100644
@@ -60,7 +60,7 @@ MODULE_PARM_DESC(max_host_mem_size_mb,
 struct nvme_dev;
 struct nvme_queue;
 
-static void nvme_process_cq(struct nvme_queue *nvmeq);
+static int nvme_process_cq(struct nvme_queue *nvmeq);
 static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown);
 
 /*

@@ -729,76 +729,100 @@ static inline bool nvme_cqe_valid(struct nvme_queue *nvmeq, u16 head,
 	return (le16_to_cpu(nvmeq->cqes[head].status) & 1) == phase;
 }
 
-static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
+static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq)
 {
-	u16 head, phase;
+	u16 head = nvmeq->cq_head;
 
-	head = nvmeq->cq_head;
-	phase = nvmeq->cq_phase;
+	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);
+	}
+}
 
-	while (nvme_cqe_valid(nvmeq, head, phase)) {
-		struct nvme_completion cqe = nvmeq->cqes[head];
-		struct request *req;
+static inline void nvme_handle_cqe(struct nvme_queue *nvmeq,
+		struct nvme_completion *cqe)
+{
+	struct request *req;
 
-		if (++head == nvmeq->q_depth) {
-			head = 0;
-			phase = !phase;
-		}
+	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;
+	}
 
-		if (tag && *tag == cqe.command_id)
-			*tag = -1;
+	/*
+	 * 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;
+	}
 
-		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;
-		}
+	req = blk_mq_tag_to_rq(*nvmeq->tags, cqe->command_id);
+	nvme_end_request(req, cqe->status, cqe->result);
+}
 
-		/*
-		 * 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;
-		}
+static inline bool nvme_read_cqe(struct nvme_queue *nvmeq,
+		struct nvme_completion *cqe)
+{
+	if (nvme_cqe_valid(nvmeq, nvmeq->cq_head, nvmeq->cq_phase)) {
+		*cqe = nvmeq->cqes[nvmeq->cq_head];
 
-		req = blk_mq_tag_to_rq(*nvmeq->tags, cqe.command_id);
-		nvme_end_request(req, cqe.status, cqe.result);
+		if (++nvmeq->cq_head == nvmeq->q_depth) {
+			nvmeq->cq_head = 0;
+			nvmeq->cq_phase = !nvmeq->cq_phase;
+		}
+		return true;
 	}
+	return false;
+}
 
-	if (head == nvmeq->cq_head && phase == nvmeq->cq_phase)
-		return;
+static int __nvme_process_cq(struct nvme_queue *nvmeq, int *tag)
+{
+	struct nvme_completion cqe;
+	int consumed = 0;
 
-	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;
+	while (nvme_read_cqe(nvmeq, &cqe)) {
+		nvme_handle_cqe(nvmeq, &cqe);
+		consumed++;
+
+		if (tag && *tag == cqe.command_id) {
+			*tag = -1;
+			break;
+		}
+	}
+
+	if (consumed) {
+		nvme_ring_cq_doorbell(nvmeq);
+		nvmeq->cqe_seen = 1;
+	}
 
-	nvmeq->cqe_seen = 1;
+	return consumed;
 }
 
-static void nvme_process_cq(struct nvme_queue *nvmeq)
+static int nvme_process_cq(struct nvme_queue *nvmeq)
 {
-	__nvme_process_cq(nvmeq, NULL);
+	return __nvme_process_cq(nvmeq, NULL);
 }
 
 static irqreturn_t nvme_irq(int irq, void *data)
 {
 	irqreturn_t result;
 	struct nvme_queue *nvmeq = data;
+
 	spin_lock(&nvmeq->q_lock);
 	nvme_process_cq(nvmeq);
 	result = nvmeq->cqe_seen ? IRQ_HANDLED : IRQ_NONE;
 	nvmeq->cqe_seen = 0;
 	spin_unlock(&nvmeq->q_lock);
+
 	return result;
 }
Signature · 1 line
2.7.4
maintainer
#2 lore raw

PATCH 1/3 nvme-pci: Split __nvme_process_cq to poll and handle

I like this split, but I think the patch as-is is not easily verifyable. Can you break it out by splitting out one helper at a time, e.g.

patch 1: introduce nvme_ring_cq_doorbell
patch 2: introduce nvme_handle_cqe
patch 3: introduce nvme_read_cqe

Also there seems ot be no reason in the this patch to change the __nvme_process_cq and nvme_process_cq return values.

author maintainer
#3 lore raw

PATCH 2/3 nvme-pci: Add budget to __nvme_process_cq

In reply to Sagi Grimberg

Prepare for irq-poll logic

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/pci.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/nvme/host/pci.c +5 -4
index a99a483a8bb5..6733446c1535 100644
@@ -784,14 +784,15 @@ static inline bool nvme_read_cqe(struct nvme_queue *nvmeq,
 	return false;
 }
 
-static int __nvme_process_cq(struct nvme_queue *nvmeq, int *tag)
+static int __nvme_process_cq(struct nvme_queue *nvmeq, int budget, int *tag)
 {
 	struct nvme_completion cqe;
 	int consumed = 0;
 
 	while (nvme_read_cqe(nvmeq, &cqe)) {
 		nvme_handle_cqe(nvmeq, &cqe);
-		consumed++;
+		if (++consumed == budget)
+			break;
 
 		if (tag && *tag == cqe.command_id) {
 			*tag = -1;

@@ -809,7 +810,7 @@ static int __nvme_process_cq(struct nvme_queue *nvmeq, int *tag)
 
 static int nvme_process_cq(struct nvme_queue *nvmeq)
 {
-	return __nvme_process_cq(nvmeq, NULL);
+	return __nvme_process_cq(nvmeq, INT_MAX, NULL);
 }
 
 static irqreturn_t nvme_irq(int irq, void *data)

@@ -838,7 +839,7 @@ 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);
+		__nvme_process_cq(nvmeq, INT_MAX, &tag);
 		spin_unlock_irq(&nvmeq->q_lock);
 
 		if (tag == -1)
Signature · 1 line
2.7.4
maintainer
#4 lore raw

PATCH 2/3 nvme-pci: Add budget to __nvme_process_cq

Quoted 1 line · Sagi Grimberg

Prepare for irq-poll logic

I don't think we should add this until we actually get the irq-poll code back on the plate.

author maintainer
#5 lore raw

PATCH 2/3 nvme-pci: Add budget to __nvme_process_cq

Quoted 4 lines
Quoted 1 line

Prepare for irq-poll logic

I don't think we should add this until we actually get the irq-poll code back on the plate.

Yea, good point...

author maintainer
#6 lore raw

PATCH 3/3 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.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/pci.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)
drivers/nvme/host/pci.c +21 -14
index 6733446c1535..f3503c45be57 100644
@@ -784,7 +784,7 @@ static inline bool nvme_read_cqe(struct nvme_queue *nvmeq,
 	return false;
 }
 
-static int __nvme_process_cq(struct nvme_queue *nvmeq, int budget, int *tag)
+static int __nvme_process_cq(struct nvme_queue *nvmeq, int budget)
 {
 	struct nvme_completion cqe;
 	int consumed = 0;

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

@@ -810,7 +805,7 @@ static int __nvme_process_cq(struct nvme_queue *nvmeq, int budget, int *tag)
 
 static int nvme_process_cq(struct nvme_queue *nvmeq)
 {
-	return __nvme_process_cq(nvmeq, INT_MAX, NULL);
+	return __nvme_process_cq(nvmeq, INT_MAX);
 }
 
 static irqreturn_t nvme_irq(int irq, void *data)

@@ -837,16 +832,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, INT_MAX, &tag);
-		spin_unlock_irq(&nvmeq->q_lock);
+	struct nvme_completion cqe;
+	int found = 0, consumed = 0;
+
+	if (!nvme_cqe_valid(nvmeq, nvmeq->cq_head, nvmeq->cq_phase))
+		return 0;
+
+	spin_lock_irq(&nvmeq->q_lock);
+	while (nvme_read_cqe(nvmeq, &cqe)) {
+		nvme_handle_cqe(nvmeq, &cqe);
+		consumed++;
 
-		if (tag == -1)
-			return 1;
+		if (tag == cqe.command_id) {
+			found = 1;
+			break;
+		}
 	}
 
-	return 0;
+	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
#7 lore raw

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

drivers/nvme/host/pci.c static int __nvme_process_cq(struct nvme_queue *nvmeq, int budget, int *tag) · line 808

Quoted 4 lines

static int nvme_process_cq(struct nvme_queue *nvmeq) {

drivers/nvme/host/pci.c +1 -1
-	return __nvme_process_cq(nvmeq, INT_MAX, NULL);
+	return __nvme_process_cq(nvmeq, INT_MAX);

At this point __nvme_process_cq and nvme_process_cq can be merged.

drivers/nvme/host/pci.c static irqreturn_t nvme_irq_check(int irq, void *data) · line 836

Quoted 4 lines

static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag) {

drivers/nvme/host/pci.c +2 -0
+	struct nvme_completion cqe;
+	int found = 0, consumed = 0;

found should be a bool.

Otherwise this looks fine to me.

author maintainer
#8 lore raw

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

Quoted 7 lines

drivers/nvme/host/pci.c static irqreturn_t nvme_irq_check(int irq, void *data) · line 836

Quoted 5 lines

static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag) {

drivers/nvme/host/pci.c +2 -0
+	struct nvme_completion cqe;
+	int found = 0, consumed = 0;

found should be a bool.

I'm returning found back from ->poll() callout so I think I'll keep it int, unless you want me to modify poll_fn signature?

maintainer
#9 lore raw

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

Quoted 11 lines · Sagi Grimberg
Quoted 7 lines

drivers/nvme/host/pci.c static irqreturn_t nvme_irq_check(int irq, void *data) · line 836

Quoted 5 lines

static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag) {

drivers/nvme/host/pci.c +2 -0
+	struct nvme_completion cqe;
+	int found = 0, consumed = 0;

found should be a bool.

I'm returning found back from ->poll() callout so I think I'll keep it int, unless you want me to modify poll_fn signature?

Let's keep it as-is for now.