[PATCHv2 0/3] nvme-pci: process cq improvements

[PATCHv2 0/3] nvme-pci: process cq improvements

[Keith Busch]

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

[PATCHv2 1/3] nvme-pci: Remove tag from process cq

[Keith Busch]

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

Re: [PATCHv2 1/3] nvme-pci: Remove tag from process cq

[Sagi Grimberg]

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

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

Re: [PATCHv2 1/3] nvme-pci: Remove tag from process cq

[Christoph Hellwig]

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

[PATCHv2 2/3] nvme-pci: Remove two-pass completions

[Keith Busch]

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

Re: [PATCHv2 2/3] nvme-pci: Remove two-pass completions

[Dongli Zhang]

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

Re: [PATCHv2 2/3] nvme-pci: Remove two-pass completions

[Keith Busch]

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

Re: [PATCHv2 2/3] nvme-pci: Remove two-pass completions

[Sagi Grimberg]

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

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

Re: [PATCHv2 2/3] nvme-pci: Remove two-pass completions

[Christoph Hellwig]

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

[PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable

[Keith Busch]

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

Re: [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable

[Sagi Grimberg]

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

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

Re: [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable

[Christoph Hellwig]

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

Re: [PATCHv2 3/3] nvme-pci: Simplify nvme_poll_irqdisable

[Keith Busch]

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