Skip to contentSkip to conversation

nvme: handle completions outside of the queue lock

PATCH New Jens Axboe 3 participants 14 messages May 17, 2018 NVMe

lore
Share & tools
lore

Jens Axboe

author maintainer
#0 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

Split the completion of events into a two part process:

1) Reap the events inside the queue lock
2) Complete the events outside the queue lock

Since we never wrap the queue, we can access the it locklessly after we've updated the completion queue head. This patch started off with batching events on the stack, but with this trick we don't have to. Keith Busch <keith.busch at intel.com> come up with that idea.

Note that this kills the ->cqe_seen as well. I haven't been able to trigger any ill effects of this. If we do race with polling every so often, it should be rare enough NOT to trigger any issues.

Signed-off-by Jens Axboe <axboe at kernel.dk>
---
 drivers/nvme/host/pci.c | 87 ++++++++++++++++++++++++++++---------------------
 1 file changed, 49 insertions(+), 38 deletions(-)
drivers/nvme/host/pci.c +49 -38
index 79bbfadcb7b9..845fb398e728 100644
@@ -160,7 +160,6 @@ struct nvme_queue {
 	u16 cq_head;
 	u16 qid;
 	u8 cq_phase;
-	u8 cqe_seen;
 	u32 *dbbuf_sq_db;
 	u32 *dbbuf_cq_db;
 	u32 *dbbuf_sq_ei;

@@ -930,7 +929,7 @@ 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)
+				   volatile struct nvme_completion *cqe)
 {
 	struct request *req;
 

@@ -950,21 +949,17 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq,
 	if (unlikely(nvmeq->qid == 0 &&
 			cqe->command_id >= NVME_AQ_BLK_MQ_DEPTH)) {
 		nvme_complete_async_event(&nvmeq->dev->ctrl,
-				cqe->status, &cqe->result);
+				cqe->status, (union nvme_result *) &cqe->result);
 		return;
 	}
 
-	nvmeq->cqe_seen = 1;
 	req = blk_mq_tag_to_rq(*nvmeq->tags, cqe->command_id);
 	nvme_end_request(req, cqe->status, cqe->result);
 }
 
-static inline bool nvme_read_cqe(struct nvme_queue *nvmeq,
-		struct nvme_completion *cqe)
+static inline bool nvme_read_cqe(struct nvme_queue *nvmeq)
 {
 	if (nvme_cqe_valid(nvmeq, nvmeq->cq_head, nvmeq->cq_phase)) {
-		*cqe = nvmeq->cqes[nvmeq->cq_head];
-
 		if (++nvmeq->cq_head == nvmeq->q_depth) {
 			nvmeq->cq_head = 0;
 			nvmeq->cq_phase = !nvmeq->cq_phase;

@@ -974,30 +969,54 @@ static inline bool nvme_read_cqe(struct nvme_queue *nvmeq,
 	return false;
 }
 
-static void nvme_process_cq(struct nvme_queue *nvmeq)
+static inline void nvme_process_cq(struct nvme_queue *nvmeq, u16 *start,
+				   u16 *end)
+{
+	*start = nvmeq->cq_head;
+	while (nvme_read_cqe(nvmeq))
+		;
+	*end = nvmeq->cq_head;
+
+	if (*start != *end)
+		nvme_ring_cq_doorbell(nvmeq);
+}
+
+static bool nvme_complete_cqes(struct nvme_queue *nvmeq, u16 start, u16 end,
+			       unsigned int tag)
 {
-	struct nvme_completion cqe;
-	int consumed = 0;
+	bool found = false;
 
-	while (nvme_read_cqe(nvmeq, &cqe)) {
-		nvme_handle_cqe(nvmeq, &cqe);
-		consumed++;
+	if (start == end)
+		return false;
+
+	while (start != end) {
+		volatile struct nvme_completion *cqe = &nvmeq->cqes[start];
+
+		if (!found && tag == cqe->command_id)
+			found = true;
+		nvme_handle_cqe(nvmeq, cqe);
+		if (++start == nvmeq->q_depth)
+			start = 0;
 	}
 
-	if (consumed)
-		nvme_ring_cq_doorbell(nvmeq);
+	return found;
 }
 
 static irqreturn_t nvme_irq(int irq, void *data)
 {
-	irqreturn_t result;
 	struct nvme_queue *nvmeq = data;
+	u16 start, end;
+
 	spin_lock(&nvmeq->q_lock);
-	nvme_process_cq(nvmeq);
-	result = nvmeq->cqe_seen ? IRQ_HANDLED : IRQ_NONE;
-	nvmeq->cqe_seen = 0;
+	nvme_process_cq(nvmeq, &start, &end);
 	spin_unlock(&nvmeq->q_lock);
-	return result;
+
+	if (start != end) {
+		nvme_complete_cqes(nvmeq, start, end, -1);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
 }
 
 static irqreturn_t nvme_irq_check(int irq, void *data)

@@ -1010,28 +1029,16 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
 
 static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag)
 {
-	struct nvme_completion cqe;
-	int found = 0, consumed = 0;
+	u16 start, end;
 
 	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 == cqe.command_id) {
-			found = 1;
-			break;
-		}
-       }
-
-	if (consumed)
-		nvme_ring_cq_doorbell(nvmeq);
+	nvme_process_cq(nvmeq, &start, &end);
 	spin_unlock_irq(&nvmeq->q_lock);
 
-	return found;
+	return nvme_complete_cqes(nvmeq, start, end, tag);
 }
 
 static int nvme_poll(struct blk_mq_hw_ctx *hctx, unsigned int tag)

@@ -1340,6 +1347,7 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown)
 {
 	struct nvme_queue *nvmeq = &dev->queues[0];
+	u16 start, end;
 
 	if (shutdown)
 		nvme_shutdown_ctrl(&dev->ctrl);

@@ -1347,8 +1355,9 @@ static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown)
 		nvme_disable_ctrl(&dev->ctrl, dev->ctrl.cap);
 
 	spin_lock_irq(&nvmeq->q_lock);
-	nvme_process_cq(nvmeq);
+	nvme_process_cq(nvmeq, &start, &end);
 	spin_unlock_irq(&nvmeq->q_lock);
+	nvme_complete_cqes(nvmeq, start, end, -1U);
 }
 
 static int nvme_cmb_qdepth(struct nvme_dev *dev, int nr_io_queues,

@@ -1995,6 +2004,7 @@ static void nvme_del_queue_end(struct request *req, blk_status_t error)
 static void nvme_del_cq_end(struct request *req, blk_status_t error)
 {
 	struct nvme_queue *nvmeq = req->end_io_data;
+	u16 start, end;
 
 	if (!error) {
 		unsigned long flags;

@@ -2006,8 +2016,9 @@ static void nvme_del_cq_end(struct request *req, blk_status_t error)
 		 */
 		spin_lock_irqsave_nested(&nvmeq->q_lock, flags,
 					SINGLE_DEPTH_NESTING);
-		nvme_process_cq(nvmeq);
+		nvme_process_cq(nvmeq, &start, &end);
 		spin_unlock_irqrestore(&nvmeq->q_lock, flags);
+		nvme_complete_cqes(nvmeq, start, end, -1U);
 	}
 
 	nvme_del_queue_end(req, error);
Signature · 4 lines
2.7.4

-- 
Jens Axboe
#1 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

Looks good to me. Queued up for 4.18 with just a minor changelog update.

#2 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

Quoted 2 lines · Keith Busch

Looks good to me. Queued up for 4.18 with just a minor changelog update.

Folks, can we please at least wait a night to give other ins a different time zone to review? In fact for things not entirely critical and non-trivial a few days would be good.

#3 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Keith Busch

Looks good to me. Queued up for 4.18 with just a minor changelog update.

Folks, can we please at least wait a night to give other ins a different time zone to review? In fact for things not entirely critical and non-trivial a few days would be good.

And here is the patch with my suggestions from the last round:

---

Quoted 1 line

From 816554a6bffe4266d33a60de042f4269597fad2c Mon Sep 17 00:00:00 2001

From: Christoph Hellwig <[email protected]>
Date: Thu, 17 May 2018 09:35:14 +0200
Subject: nvme-pci: streamling the CQ batch processing

Also fixes poll to return once we found the command we polled for and propagates the volatile status for the result field properly,

Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/core.c |  2 +-
 drivers/nvme/host/nvme.h |  2 +-
 drivers/nvme/host/pci.c  | 75 +++++++++++++++++-----------------------
 3 files changed, 34 insertions(+), 45 deletions(-)
drivers/nvme/host/core.c +1 -1
index 62262fac7a5d..b070c659391f 100644
@@ -3344,7 +3344,7 @@ static void nvme_fw_act_work(struct work_struct *work)
 }
 
 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
-		union nvme_result *res)
+		volatile union nvme_result *res)
 {
 	u32 result = le32_to_cpu(res->u32);
drivers/nvme/host/nvme.h +1 -1
index 061fecfd44f5..7a872b0d53a7 100644
@@ -400,7 +400,7 @@ int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
 		bool send);
 
 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
-		union nvme_result *res);
+		volatile union nvme_result *res);
 
 void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
drivers/nvme/host/pci.c +32 -43

148 lines not on this page · raw message

Signature · 1 line
2.17.0
author maintainer
#4 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

Quoted 10 lines · Christoph Hellwig
Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Keith Busch

Looks good to me. Queued up for 4.18 with just a minor changelog update.

Folks, can we please at least wait a night to give other ins a different time zone to review? In fact for things not entirely critical and non-trivial a few days would be good.

And here is the patch with my suggestions from the last round:

Looks good to me, also tests out fine.

Reviewed-by Jens Axboe <axboe at kernel.dk>
Signature · 1 line
Jens Axboe
#5 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

In reply to Christoph Hellwig

drivers/nvme/host/pci.c static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag) · line 1029

Quoted 9 lines · Christoph Hellwig

static irqreturn_t nvme_irq_check(int irq, void *data)

drivers/nvme/host/pci.c +3 -1
@@ -1039,7 +1024,14 @@ static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag)
 	nvme_process_cq(nvmeq, &start, &end);
 	spin_unlock_irq(&nvmeq->q_lock);
 
-	return nvme_complete_cqes(nvmeq, start, end, tag);
+	while (start != end) {
+		if (nvme_handle_cqe(nvmeq, start, tag))
+			return 1;

We can't return early from here anymore since the new interface moved the CQ head. No one else is going to get to see those completions, so the first caller owns completing everything up to the end that it claimed.

#6 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

Quoted 6 lines · Keith Busch

drivers/nvme/host/pci.c static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag) · line 1029

Quoted 2 lines
drivers/nvme/host/pci.c +2 -0
+		if (nvme_handle_cqe(nvmeq, start, tag))
+			return 1;

We can't return early from here anymore since the new interface moved the CQ head. No one else is going to get to see those completions, so the first caller owns completing everything up to the end that it claimed.

Indeed. So either we have to complete everything, or we have to not do the batch processing for the poll case. Given that when we're polling we have a QD approaching one on a given core I suspect not batching for the poll case might be the right answer, but someone will have to run the numbers.

#7 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

Quoted 8 lines · Christoph Hellwig
Quoted 6 lines · Keith Busch

drivers/nvme/host/pci.c static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag) · line 1029

Quoted 2 lines
drivers/nvme/host/pci.c +2 -0
+		if (nvme_handle_cqe(nvmeq, start, tag))
+			return 1;

We can't return early from here anymore since the new interface moved the CQ head. No one else is going to get to see those completions, so the first caller owns completing everything up to the end that it claimed.

Quoted 5 lines

Indeed. So either we have to complete everything, or we have to not do the batch processing for the poll case. Given that when we're polling we have a QD approaching one on a given core I suspect not batching for the poll case might be the right answer, but someone will have to run the numbers.

Maybe the batch can take the tag and stop moving the head when it finds a match?

#8 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

In reply to Christoph Hellwig
Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Keith Busch

Looks good to me. Queued up for 4.18 with just a minor changelog update.

Folks, can we please at least wait a night to give other ins a different time zone to review? In fact for things not entirely critical and non-trivial a few days would be good.

Fair enough. I did rush this one. I can back it out as we go through your suggestions.

author maintainer
#9 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

Quoted 11 lines · Keith Busch
Quoted 7 lines · Christoph Hellwig
Quoted 2 lines · Keith Busch

Looks good to me. Queued up for 4.18 with just a minor changelog update.

Folks, can we please at least wait a night to give other ins a different time zone to review? In fact for things not entirely critical and non-trivial a few days would be good.

Fair enough. I did rush this one. I can back it out as we go through your suggestions.

Just apply it on top, doesn't really warrant a rebase imho. But yeah, we should probably cool the jets for a day next time :-)

Signature · 1 line
Jens Axboe
#10 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

Quoted 2 lines · Jens Axboe

Just apply it on top, doesn't really warrant a rebase imho. But yeah, we should probably cool the jets for a day next time :-)

Honestly, between the poll issue and things like the volatile annotations that should have been a separate prep patch I'd rather back out and restart the thing slowly. We should still have plenty of time to get it done for this merge window.

author maintainer
#11 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

Quoted 8 lines · Christoph Hellwig
Quoted 2 lines · Jens Axboe

Just apply it on top, doesn't really warrant a rebase imho. But yeah, we should probably cool the jets for a day next time :-)

Honestly, between the poll issue and things like the volatile annotations that should have been a separate prep patch I'd rather back out and restart the thing slowly. We should still have plenty of time to get it done for this merge window.

The poll issue is almost moot imho, unless you have a ton of other IO on the same hardware queue while you poll. It'll work fine, the testing I ran didn't find anything. Agree the volatile should have been a prep, but again not really a major thing.

Signature · 1 line
Jens Axboe
#12 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

Quoted 4 lines · Jens Axboe

The poll issue is almost moot imho, unless you have a ton of other IO on the same hardware queue while you poll. It'll work fine, the testing I ran didn't find anything. Agree the volatile should have been a prep, but again not really a major thing.

But it's not a whole lot of work to redo it either. I'll volunteer doing it while spending time on a call :)

author maintainer
#13 lore raw

PATCH 1/1 nvme: handle completions outside of the queue lock

Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · Jens Axboe

The poll issue is almost moot imho, unless you have a ton of other IO on the same hardware queue while you poll. It'll work fine, the testing I ran didn't find anything. Agree the volatile should have been a prep, but again not really a major thing.

But it's not a whole lot of work to redo it either. I'll volunteer doing it while spending time on a call :)

Alright, sold :-)

Signature · 1 line
Jens Axboe