All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHv2 0/3] nvme-pci: process cq improvements
@ 2020-03-04 18:12 Keith Busch
  2020-03-04 18:12 ` [PATCHv2 1/3] nvme-pci: Remove tag from process cq Keith Busch
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Keith Busch @ 2020-03-04 18:12 UTC (permalink / raw)
  To: linux-nvme, hch, sagi; +Cc: Keith Busch, bijan.mottahedeh

Chances since v1:

  Rebased to linux 5.6-rc4

  Didn't alter the post/pre increment for 'found' completions

  Don't use the return value from nvme_poll_irqdisable()

  Newly addeded last patch removes nvme_poll_irqdisable() handling for
  polled queues

Keith Busch (3):
  nvme-pci: Remove tag from process cq
  nvme-pci: Remove two-pass completions
  nvme-pci: Simplify nvme_poll_irqdisable

 drivers/nvme/host/pci.c | 83 ++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 56 deletions(-)

-- 
2.24.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCHv2 1/3] nvme-pci: Remove tag from process cq
  2020-03-04 18:12 [PATCHv2 0/3] nvme-pci: process cq improvements Keith Busch
@ 2020-03-04 18:12 ` Keith Busch
  2020-03-05 20:53   ` Sagi Grimberg
  2020-03-10 16:56   ` Christoph Hellwig
  2020-03-04 18:12 ` [PATCHv2 2/3] nvme-pci: Remove two-pass completions Keith Busch
  2020-03-04 18:12 ` [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable Keith Busch
  2 siblings, 2 replies; 13+ messages in thread
From: Keith Busch @ 2020-03-04 18:12 UTC (permalink / raw)
  To: linux-nvme, hch, sagi; +Cc: Keith Busch, bijan.mottahedeh

The only user for tagged completion was for timeout handling. That user,
though, really only cares if the timed out command is completed, which
we can safely check within the timeout handler.

Remove the tag check to simplify completion handling.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/pci.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index cdc9b6149d38..98d8ddd7aa0f 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -989,14 +989,13 @@ static inline void nvme_update_cq_head(struct nvme_queue *nvmeq)
 }
 
 static inline int nvme_process_cq(struct nvme_queue *nvmeq, u16 *start,
-				  u16 *end, unsigned int tag)
+				  u16 *end)
 {
 	int found = 0;
 
 	*start = nvmeq->cq_head;
 	while (nvme_cqe_pending(nvmeq)) {
-		if (tag == -1U || nvmeq->cqes[nvmeq->cq_head].command_id == tag)
-			found++;
+		found++;
 		nvme_update_cq_head(nvmeq);
 	}
 	*end = nvmeq->cq_head;
@@ -1017,7 +1016,7 @@ static irqreturn_t nvme_irq(int irq, void *data)
 	 * the irq handler, even if that was on another CPU.
 	 */
 	rmb();
-	nvme_process_cq(nvmeq, &start, &end, -1);
+	nvme_process_cq(nvmeq, &start, &end);
 	wmb();
 
 	if (start != end) {
@@ -1040,7 +1039,7 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
  * Poll for completions any queue, including those not dedicated to polling.
  * Can be called from any context.
  */
-static int nvme_poll_irqdisable(struct nvme_queue *nvmeq, unsigned int tag)
+static int nvme_poll_irqdisable(struct nvme_queue *nvmeq)
 {
 	struct pci_dev *pdev = to_pci_dev(nvmeq->dev->dev);
 	u16 start, end;
@@ -1053,11 +1052,11 @@ static int nvme_poll_irqdisable(struct nvme_queue *nvmeq, unsigned int tag)
 	 */
 	if (test_bit(NVMEQ_POLLED, &nvmeq->flags)) {
 		spin_lock(&nvmeq->cq_poll_lock);
-		found = nvme_process_cq(nvmeq, &start, &end, tag);
+		found = nvme_process_cq(nvmeq, &start, &end);
 		spin_unlock(&nvmeq->cq_poll_lock);
 	} else {
 		disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
-		found = nvme_process_cq(nvmeq, &start, &end, tag);
+		found = nvme_process_cq(nvmeq, &start, &end);
 		enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
 	}
 
@@ -1075,8 +1074,7 @@ static int nvme_poll(struct blk_mq_hw_ctx *hctx)
 		return 0;
 
 	spin_lock(&nvmeq->cq_poll_lock);
-	found = nvme_process_cq(nvmeq, &start, &end, -1);
-	nvme_complete_cqes(nvmeq, start, end);
+	found = nvme_process_cq(nvmeq, &start, &end);
 	spin_unlock(&nvmeq->cq_poll_lock);
 
 	return found;
@@ -1253,7 +1251,8 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 	/*
 	 * Did we miss an interrupt?
 	 */
-	if (nvme_poll_irqdisable(nvmeq, req->tag)) {
+	nvme_poll_irqdisable(nvmeq);
+	if (blk_mq_request_completed(req)) {
 		dev_warn(dev->ctrl.device,
 			 "I/O %d QID %d timeout, completion polled\n",
 			 req->tag, nvmeq->qid);
@@ -1396,7 +1395,7 @@ static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown)
 	else
 		nvme_disable_ctrl(&dev->ctrl);
 
-	nvme_poll_irqdisable(nvmeq, -1);
+	nvme_poll_irqdisable(nvmeq);
 }
 
 /*
@@ -1411,7 +1410,7 @@ static void nvme_reap_pending_cqes(struct nvme_dev *dev)
 	int i;
 
 	for (i = dev->ctrl.queue_count - 1; i > 0; i--) {
-		nvme_process_cq(&dev->queues[i], &start, &end, -1);
+		nvme_process_cq(&dev->queues[i], &start, &end);
 		nvme_complete_cqes(&dev->queues[i], start, end);
 	}
 }
-- 
2.24.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv2 2/3] nvme-pci: Remove two-pass completions
  2020-03-04 18:12 [PATCHv2 0/3] nvme-pci: process cq improvements Keith Busch
  2020-03-04 18:12 ` [PATCHv2 1/3] nvme-pci: Remove tag from process cq Keith Busch
@ 2020-03-04 18:12 ` Keith Busch
  2020-03-05  9:50   ` Dongli Zhang
                     ` (2 more replies)
  2020-03-04 18:12 ` [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable Keith Busch
  2 siblings, 3 replies; 13+ messages in thread
From: Keith Busch @ 2020-03-04 18:12 UTC (permalink / raw)
  To: linux-nvme, hch, sagi; +Cc: Keith Busch, bijan.mottahedeh

Completion handling had been done in two steps: find all new completions
under a lock, then handle those completions outside the lock. This was
done to make the locked section as short as possible so that other
threads using the same lock wait less time.

The driver no longer shares locks during completion, and is in fact
lockless for interrupt driven queues, so the optimization no longer
serves its original purpose. Replace the two-pass completion queue
handler with a single pass that completes entries immediately.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/pci.c | 42 ++++++++++-------------------------------
 1 file changed, 10 insertions(+), 32 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 98d8ddd7aa0f..02f22c63adcf 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -971,15 +971,6 @@ static inline void nvme_handle_cqe(struct nvme_queue *nvmeq, u16 idx)
 	nvme_end_request(req, cqe->status, cqe->result);
 }
 
-static void nvme_complete_cqes(struct nvme_queue *nvmeq, u16 start, u16 end)
-{
-	while (start != end) {
-		nvme_handle_cqe(nvmeq, start);
-		if (++start == nvmeq->q_depth)
-			start = 0;
-	}
-}
-
 static inline void nvme_update_cq_head(struct nvme_queue *nvmeq)
 {
 	if (++nvmeq->cq_head == nvmeq->q_depth) {
@@ -988,19 +979,17 @@ static inline void nvme_update_cq_head(struct nvme_queue *nvmeq)
 	}
 }
 
-static inline int nvme_process_cq(struct nvme_queue *nvmeq, u16 *start,
-				  u16 *end)
+static inline int nvme_process_cq(struct nvme_queue *nvmeq)
 {
 	int found = 0;
 
-	*start = nvmeq->cq_head;
 	while (nvme_cqe_pending(nvmeq)) {
 		found++;
+		nvme_handle_cqe(nvmeq, nvmeq->cq_head);
 		nvme_update_cq_head(nvmeq);
 	}
-	*end = nvmeq->cq_head;
 
-	if (*start != *end)
+	if (found)
 		nvme_ring_cq_doorbell(nvmeq);
 	return found;
 }
@@ -1009,21 +998,16 @@ static irqreturn_t nvme_irq(int irq, void *data)
 {
 	struct nvme_queue *nvmeq = data;
 	irqreturn_t ret = IRQ_NONE;
-	u16 start, end;
 
 	/*
 	 * The rmb/wmb pair ensures we see all updates from a previous run of
 	 * the irq handler, even if that was on another CPU.
 	 */
 	rmb();
-	nvme_process_cq(nvmeq, &start, &end);
+	if (nvme_process_cq(nvmeq))
+		ret = IRQ_HANDLED;
 	wmb();
 
-	if (start != end) {
-		nvme_complete_cqes(nvmeq, start, end);
-		return IRQ_HANDLED;
-	}
-
 	return ret;
 }
 
@@ -1042,7 +1026,6 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
 static int nvme_poll_irqdisable(struct nvme_queue *nvmeq)
 {
 	struct pci_dev *pdev = to_pci_dev(nvmeq->dev->dev);
-	u16 start, end;
 	int found;
 
 	/*
@@ -1052,29 +1035,27 @@ static int nvme_poll_irqdisable(struct nvme_queue *nvmeq)
 	 */
 	if (test_bit(NVMEQ_POLLED, &nvmeq->flags)) {
 		spin_lock(&nvmeq->cq_poll_lock);
-		found = nvme_process_cq(nvmeq, &start, &end);
+		found = nvme_process_cq(nvmeq);
 		spin_unlock(&nvmeq->cq_poll_lock);
 	} else {
 		disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
-		found = nvme_process_cq(nvmeq, &start, &end);
+		found = nvme_process_cq(nvmeq);
 		enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
 	}
 
-	nvme_complete_cqes(nvmeq, start, end);
 	return found;
 }
 
 static int nvme_poll(struct blk_mq_hw_ctx *hctx)
 {
 	struct nvme_queue *nvmeq = hctx->driver_data;
-	u16 start, end;
 	bool found;
 
 	if (!nvme_cqe_pending(nvmeq))
 		return 0;
 
 	spin_lock(&nvmeq->cq_poll_lock);
-	found = nvme_process_cq(nvmeq, &start, &end);
+	found = nvme_process_cq(nvmeq);
 	spin_unlock(&nvmeq->cq_poll_lock);
 
 	return found;
@@ -1406,13 +1387,10 @@ static void nvme_disable_admin_queue(struct nvme_dev *dev, bool shutdown)
  */
 static void nvme_reap_pending_cqes(struct nvme_dev *dev)
 {
-	u16 start, end;
 	int i;
 
-	for (i = dev->ctrl.queue_count - 1; i > 0; i--) {
-		nvme_process_cq(&dev->queues[i], &start, &end);
-		nvme_complete_cqes(&dev->queues[i], start, end);
-	}
+	for (i = dev->ctrl.queue_count - 1; i > 0; i--)
+		nvme_process_cq(&dev->queues[i]);
 }
 
 static int nvme_cmb_qdepth(struct nvme_dev *dev, int nr_io_queues,
-- 
2.24.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable
  2020-03-04 18:12 [PATCHv2 0/3] nvme-pci: process cq improvements Keith Busch
  2020-03-04 18:12 ` [PATCHv2 1/3] nvme-pci: Remove tag from process cq Keith Busch
  2020-03-04 18:12 ` [PATCHv2 2/3] nvme-pci: Remove two-pass completions Keith Busch
@ 2020-03-04 18:12 ` Keith Busch
  2020-03-05 20:55   ` Sagi Grimberg
  2020-03-10 17:02   ` Christoph Hellwig
  2 siblings, 2 replies; 13+ messages in thread
From: Keith Busch @ 2020-03-04 18:12 UTC (permalink / raw)
  To: linux-nvme, hch, sagi; +Cc: Keith Busch, bijan.mottahedeh

The timeout handler can use the existing nvme_poll() if it needs to
check a polled queue, allowing nvme_poll_irqdisable() to handle only
irq driven queues for the remaining callers.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/pci.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 02f22c63adcf..227e2aed7e08 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1020,35 +1020,20 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
 }
 
 /*
- * Poll for completions any queue, including those not dedicated to polling.
+ * Poll for completions for any interrupt driven queue
  * Can be called from any context.
  */
-static int nvme_poll_irqdisable(struct nvme_queue *nvmeq)
+static void nvme_poll_irqdisable(struct nvme_queue *nvmeq)
 {
 	struct pci_dev *pdev = to_pci_dev(nvmeq->dev->dev);
-	int found;
 
-	/*
-	 * For a poll queue we need to protect against the polling thread
-	 * using the CQ lock.  For normal interrupt driven threads we have
-	 * to disable the interrupt to avoid racing with it.
-	 */
-	if (test_bit(NVMEQ_POLLED, &nvmeq->flags)) {
-		spin_lock(&nvmeq->cq_poll_lock);
-		found = nvme_process_cq(nvmeq);
-		spin_unlock(&nvmeq->cq_poll_lock);
-	} else {
-		disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
-		found = nvme_process_cq(nvmeq);
-		enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
-	}
-
-	return found;
+	disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
+	nvme_process_cq(nvmeq);
+	enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
 }
 
-static int nvme_poll(struct blk_mq_hw_ctx *hctx)
+static int __nvme_poll(struct nvme_queue *nvmeq)
 {
-	struct nvme_queue *nvmeq = hctx->driver_data;
 	bool found;
 
 	if (!nvme_cqe_pending(nvmeq))
@@ -1061,6 +1046,11 @@ static int nvme_poll(struct blk_mq_hw_ctx *hctx)
 	return found;
 }
 
+static int nvme_poll(struct blk_mq_hw_ctx *hctx)
+{
+	return __nvme_poll(hctx->driver_data);
+}
+
 static void nvme_pci_submit_async_event(struct nvme_ctrl *ctrl)
 {
 	struct nvme_dev *dev = to_nvme_dev(ctrl);
@@ -1232,7 +1222,11 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 	/*
 	 * Did we miss an interrupt?
 	 */
-	nvme_poll_irqdisable(nvmeq);
+	if (!test_bit(NVMEQ_POLLED, &nvmeq->flags))
+		nvme_poll_irqdisable(nvmeq);
+	else
+		__nvme_poll(nvmeq);
+
 	if (blk_mq_request_completed(req)) {
 		dev_warn(dev->ctrl.device,
 			 "I/O %d QID %d timeout, completion polled\n",
-- 
2.24.1


_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCHv2 2/3] nvme-pci: Remove two-pass completions
  2020-03-04 18:12 ` [PATCHv2 2/3] nvme-pci: Remove two-pass completions Keith Busch
@ 2020-03-05  9:50   ` Dongli Zhang
  2020-03-05 15:15     ` Keith Busch
  2020-03-05 20:53   ` Sagi Grimberg
  2020-03-10 16:57   ` Christoph Hellwig
  2 siblings, 1 reply; 13+ messages in thread
From: Dongli Zhang @ 2020-03-05  9:50 UTC (permalink / raw)
  To: Keith Busch, linux-nvme, hch, sagi; +Cc: bijan.mottahedeh

Hi Keith,

On 3/4/20 10:12 AM, Keith Busch wrote:
> Completion handling had been done in two steps: find all new completions
> under a lock, then handle those completions outside the lock. This was
> done to make the locked section as short as possible so that other
> threads using the same lock wait less time.
> 
> The driver no longer shares locks during completion, and is in fact
> lockless for interrupt driven queues, so the optimization no longer

About "no longer shares locks", would you mind share is it a specific nvme
driver commit, or the transition from sq to mq?

Thank you very much!

Dongli Zhang

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2 2/3] nvme-pci: Remove two-pass completions
  2020-03-05  9:50   ` Dongli Zhang
@ 2020-03-05 15:15     ` Keith Busch
  0 siblings, 0 replies; 13+ messages in thread
From: Keith Busch @ 2020-03-05 15:15 UTC (permalink / raw)
  To: Dongli Zhang; +Cc: bijan.mottahedeh, hch, linux-nvme, sagi

On Thu, Mar 05, 2020 at 01:50:58AM -0800, Dongli Zhang wrote:
> Hi Keith,
> 
> On 3/4/20 10:12 AM, Keith Busch wrote:
> > Completion handling had been done in two steps: find all new completions
> > under a lock, then handle those completions outside the lock. This was
> > done to make the locked section as short as possible so that other
> > threads using the same lock wait less time.
> > 
> > The driver no longer shares locks during completion, and is in fact
> > lockless for interrupt driven queues, so the optimization no longer
> 
> About "no longer shares locks", would you mind share is it a specific nvme
> driver commit, or the transition from sq to mq?

They're all nvme specific commits. For the record, nvme never
transitioned from 'sq' to 'mq'. We were bio-based before 'mq'
existed.

Commit splitting locks: 1ab0cd6966fc4a7e9dfbd7c6eda917ae9c977f42

Commit removing cq lock: 3a7afd8ee42a68d4f24ab9c947a4ef82d4d52375

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2 1/3] nvme-pci: Remove tag from process cq
  2020-03-04 18:12 ` [PATCHv2 1/3] nvme-pci: Remove tag from process cq Keith Busch
@ 2020-03-05 20:53   ` Sagi Grimberg
  2020-03-10 16:56   ` Christoph Hellwig
  1 sibling, 0 replies; 13+ messages in thread
From: Sagi Grimberg @ 2020-03-05 20:53 UTC (permalink / raw)
  To: Keith Busch, linux-nvme, hch; +Cc: bijan.mottahedeh

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2 2/3] nvme-pci: Remove two-pass completions
  2020-03-04 18:12 ` [PATCHv2 2/3] nvme-pci: Remove two-pass completions Keith Busch
  2020-03-05  9:50   ` Dongli Zhang
@ 2020-03-05 20:53   ` Sagi Grimberg
  2020-03-10 16:57   ` Christoph Hellwig
  2 siblings, 0 replies; 13+ messages in thread
From: Sagi Grimberg @ 2020-03-05 20:53 UTC (permalink / raw)
  To: Keith Busch, linux-nvme, hch; +Cc: bijan.mottahedeh

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable
  2020-03-04 18:12 ` [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable Keith Busch
@ 2020-03-05 20:55   ` Sagi Grimberg
  2020-03-10 17:02   ` Christoph Hellwig
  1 sibling, 0 replies; 13+ messages in thread
From: Sagi Grimberg @ 2020-03-05 20:55 UTC (permalink / raw)
  To: Keith Busch, linux-nvme, hch; +Cc: bijan.mottahedeh

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2 1/3] nvme-pci: Remove tag from process cq
  2020-03-04 18:12 ` [PATCHv2 1/3] nvme-pci: Remove tag from process cq Keith Busch
  2020-03-05 20:53   ` Sagi Grimberg
@ 2020-03-10 16:56   ` Christoph Hellwig
  1 sibling, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2020-03-10 16:56 UTC (permalink / raw)
  To: Keith Busch; +Cc: bijan.mottahedeh, hch, linux-nvme, sagi

On Wed, Mar 04, 2020 at 10:12:44AM -0800, Keith Busch wrote:
> The only user for tagged completion was for timeout handling. That user,
> though, really only cares if the timed out command is completed, which
> we can safely check within the timeout handler.
> 
> Remove the tag check to simplify completion handling.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2 2/3] nvme-pci: Remove two-pass completions
  2020-03-04 18:12 ` [PATCHv2 2/3] nvme-pci: Remove two-pass completions Keith Busch
  2020-03-05  9:50   ` Dongli Zhang
  2020-03-05 20:53   ` Sagi Grimberg
@ 2020-03-10 16:57   ` Christoph Hellwig
  2 siblings, 0 replies; 13+ messages in thread
From: Christoph Hellwig @ 2020-03-10 16:57 UTC (permalink / raw)
  To: Keith Busch; +Cc: bijan.mottahedeh, hch, linux-nvme, sagi

On Wed, Mar 04, 2020 at 10:12:45AM -0800, Keith Busch wrote:
> Completion handling had been done in two steps: find all new completions
> under a lock, then handle those completions outside the lock. This was
> done to make the locked section as short as possible so that other
> threads using the same lock wait less time.
> 
> The driver no longer shares locks during completion, and is in fact
> lockless for interrupt driven queues, so the optimization no longer
> serves its original purpose. Replace the two-pass completion queue
> handler with a single pass that completes entries immediately.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable
  2020-03-04 18:12 ` [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable Keith Busch
  2020-03-05 20:55   ` Sagi Grimberg
@ 2020-03-10 17:02   ` Christoph Hellwig
  2020-03-10 19:16     ` Keith Busch
  1 sibling, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2020-03-10 17:02 UTC (permalink / raw)
  To: Keith Busch; +Cc: bijan.mottahedeh, hch, linux-nvme, sagi

On Wed, Mar 04, 2020 at 10:12:46AM -0800, Keith Busch wrote:
> The timeout handler can use the existing nvme_poll() if it needs to
> check a polled queue, allowing nvme_poll_irqdisable() to handle only
> irq driven queues for the remaining callers.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/host/pci.c | 38 ++++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 02f22c63adcf..227e2aed7e08 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1020,35 +1020,20 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
>  }
>  
>  /*
> - * Poll for completions any queue, including those not dedicated to polling.
> + * Poll for completions for any interrupt driven queue
>   * Can be called from any context.
>   */
> -static int nvme_poll_irqdisable(struct nvme_queue *nvmeq)
> +static void nvme_poll_irqdisable(struct nvme_queue *nvmeq)
>  {
>  	struct pci_dev *pdev = to_pci_dev(nvmeq->dev->dev);
>  
> +	disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
> +	nvme_process_cq(nvmeq);
> +	enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));

This could use a:

	WARN_ON_ONCE(test_bit(NVMEQ_POLLED, &nvmeq->flags));

>  }
>  
> -static int nvme_poll(struct blk_mq_hw_ctx *hctx)
> +static int __nvme_poll(struct nvme_queue *nvmeq)

Do we really need the magic __nvme_poll?  struct request has a
pointer to the blk_mq_hw_ctx, so we could just pass that.

> +	if (!test_bit(NVMEQ_POLLED, &nvmeq->flags))
> +		nvme_poll_irqdisable(nvmeq);
> +	else
> +		__nvme_poll(nvmeq);
> +

Any reason for the odd inversion and not simply:

	if (test_bit(NVMEQ_POLLED, &nvmeq->flags))
		nvme_poll(req->mq_hctx);
	else
		nvme_poll_irqdisable(nvmeq);

?

Otherwise looks good.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable
  2020-03-10 17:02   ` Christoph Hellwig
@ 2020-03-10 19:16     ` Keith Busch
  0 siblings, 0 replies; 13+ messages in thread
From: Keith Busch @ 2020-03-10 19:16 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: sagi, linux-nvme, bijan.mottahedeh

On Tue, Mar 10, 2020 at 06:02:26PM +0100, Christoph Hellwig wrote:
> On Wed, Mar 04, 2020 at 10:12:46AM -0800, Keith Busch wrote:
> >  /*
> > - * Poll for completions any queue, including those not dedicated to polling.
> > + * Poll for completions for any interrupt driven queue
> >   * Can be called from any context.
> >   */
> > -static int nvme_poll_irqdisable(struct nvme_queue *nvmeq)
> > +static void nvme_poll_irqdisable(struct nvme_queue *nvmeq)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(nvmeq->dev->dev);
> >  
> > +	disable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
> > +	nvme_process_cq(nvmeq);
> > +	enable_irq(pci_irq_vector(pdev, nvmeq->cq_vector));
> 
> This could use a:
> 
> 	WARN_ON_ONCE(test_bit(NVMEQ_POLLED, &nvmeq->flags));

No problem.
 
> >  }
> >  
> > -static int nvme_poll(struct blk_mq_hw_ctx *hctx)
> > +static int __nvme_poll(struct nvme_queue *nvmeq)
> 
> Do we really need the magic __nvme_poll?  struct request has a
> pointer to the blk_mq_hw_ctx, so we could just pass that.

Sure, I just felt like req->mq_hctx should be private to blk-mq. It
isn't, so we can use it from the timeout path.

> > +	if (!test_bit(NVMEQ_POLLED, &nvmeq->flags))
> > +		nvme_poll_irqdisable(nvmeq);
> > +	else
> > +		__nvme_poll(nvmeq);
> > +
> 
> Any reason for the odd inversion and not simply:
> 
> 	if (test_bit(NVMEQ_POLLED, &nvmeq->flags))
> 		nvme_poll(req->mq_hctx);
> 	else
> 		nvme_poll_irqdisable(nvmeq);
> 
> ?
> 
> Otherwise looks good.

Thanks, I'll rework the suggestions in.

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-03-10 19:16 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-04 18:12 [PATCHv2 0/3] nvme-pci: process cq improvements Keith Busch
2020-03-04 18:12 ` [PATCHv2 1/3] nvme-pci: Remove tag from process cq Keith Busch
2020-03-05 20:53   ` Sagi Grimberg
2020-03-10 16:56   ` Christoph Hellwig
2020-03-04 18:12 ` [PATCHv2 2/3] nvme-pci: Remove two-pass completions Keith Busch
2020-03-05  9:50   ` Dongli Zhang
2020-03-05 15:15     ` Keith Busch
2020-03-05 20:53   ` Sagi Grimberg
2020-03-10 16:57   ` Christoph Hellwig
2020-03-04 18:12 ` [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable Keith Busch
2020-03-05 20:55   ` Sagi Grimberg
2020-03-10 17:02   ` Christoph Hellwig
2020-03-10 19:16     ` Keith Busch

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.