[PATCH v2 0/4] refactor nvme-pci cq processing

[PATCH v2 0/4] refactor nvme-pci cq processing

[Sagi Grimberg]

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(-)

-- 
2.7.4

[PATCH v2 1/4] nvme-pci: Introduce nvme_ring_cq_doorbell

[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>
---
 drivers/nvme/host/pci.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 0f09a2d5cf7a..042cfe5ef8e9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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;
 }
 
-- 
2.7.4

[PATCH v2 1/4] nvme-pci: Introduce nvme_ring_cq_doorbell

[Christoph Hellwig]

On Sun, Jun 18, 2017@05:28:07PM +0300, Sagi Grimberg wrote:
> 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>

[PATCH v2 1/4] nvme-pci: Introduce nvme_ring_cq_doorbell

[Keith Busch]

On Sun, Jun 18, 2017@05:28:07PM +0300, Sagi Grimberg wrote:
> 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>

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

[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(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 042cfe5ef8e9..26eb1743f8bc 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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)
-- 
2.7.4

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

[Christoph Hellwig]

On Sun, Jun 18, 2017@05:28:08PM +0300, Sagi Grimberg wrote:
> 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(-)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 042cfe5ef8e9..26eb1743f8bc 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -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.
> +	 */

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

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

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

[Keith Busch]

On Sun, Jun 18, 2017@05:28:08PM +0300, Sagi Grimberg wrote:
> 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>

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

[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(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 26eb1743f8bc..d309b6c90511 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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)
-- 
2.7.4

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

[Christoph Hellwig]

Looks good,

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

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

[Keith Busch]

On Sun, Jun 18, 2017@05:28:09PM +0300, Sagi Grimberg wrote:
> 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. :)

> -	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?

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

[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. :)

:(

>> -	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...

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

[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(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index d309b6c90511..2a9ee769ce9e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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)
-- 
2.7.4

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

[Christoph Hellwig]

Looks good,

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

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

[Keith Busch]

On Sun, Jun 18, 2017@05:28:10PM +0300, Sagi Grimberg wrote:
> 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>