convert nvme pci to use irq-poll service

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

We remove the cqe_seen indication as a preparation for irq-poll where we will schedule soft-irq context for polling so we should consider the interrupt as handled always.

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 +11 -9

+static int __nvme_process_cq(struct nvme_queue *nvmeq, int *tag)
+{
+	struct nvme_completion cqe;
+	int consumed = 0;
 
-	}
+	while (nvme_read_cqe(nvmeq, &cqe)) {
+		nvme_handle_cqe(nvmeq, &cqe);
 
-	/* If the controller ignores the cq head doorbell and continuously
-	 * writes to the queue, it is theoretically possible to wrap around
-	 * the queue twice and mistakenly return IRQ_NONE.  Linux only
-	 * requires that 0.1% of your interrupts are handled, so this isn't
-	 * a big problem.
-	 */
-	if (head == nvmeq->cq_head && phase == nvmeq->cq_phase)
-		return;
+		if (tag && *tag == cqe.command_id) {
+			*tag = -1;
+			break;
+		}
+	}
 

Quoted 31 lines

drivers/nvme/host/pci.c +13 -13

+static int __nvme_process_cq(struct nvme_queue *nvmeq, int *tag)
+{
+	struct nvme_completion cqe;
+	int consumed = 0;
 
-	}
+	while (nvme_read_cqe(nvmeq, &cqe)) {
+		nvme_handle_cqe(nvmeq, &cqe);
 
-	/* If the controller ignores the cq head doorbell and continuously
-	 * writes to the queue, it is theoretically possible to wrap around
-	 * the queue twice and mistakenly return IRQ_NONE.  Linux only
-	 * requires that 0.1% of your interrupts are handled, so this isn't
-	 * a big problem.
-	 */
-	if (head == nvmeq->cq_head && phase == nvmeq->cq_phase)
-		return;
+		if (tag && *tag == cqe.command_id) {
+			*tag = -1;
+			break;
+		}
+	}
 
-	if (likely(nvmeq->cq_vector >= 0))
-		writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
-	nvmeq->cq_head = head;
-	nvmeq->cq_phase = phase;
+	if (consumed)
+		nvme_ring_cq_doorbell(nvmeq);

drivers/nvme/host/pci.c static inline bool nvme_cqe_valid(struct nvme_queue *nvmeq, u16 head, · line 726

Quoted 3 lines

drivers/nvme/host/pci.c +1 -1

-	nvmeq->cqe_seen = 1;
+	return consumed;
 }

Won't 'consumed' always be 0 here and we thus never call nvme_ring_cq_doorbell()? Am I overlooking something here, or is this just for preparation of later patches?

Quoted 31 lines

Quoted 31 lines

drivers/nvme/host/pci.c +13 -13

+static int __nvme_process_cq(struct nvme_queue *nvmeq, int *tag)
+{
+	struct nvme_completion cqe;
+	int consumed = 0;
 
-	}
+	while (nvme_read_cqe(nvmeq, &cqe)) {
+		nvme_handle_cqe(nvmeq, &cqe);
 
-	/* If the controller ignores the cq head doorbell and continuously
-	 * writes to the queue, it is theoretically possible to wrap around
-	 * the queue twice and mistakenly return IRQ_NONE.  Linux only
-	 * requires that 0.1% of your interrupts are handled, so this isn't
-	 * a big problem.
-	 */
-	if (head == nvmeq->cq_head && phase == nvmeq->cq_phase)
-		return;
+		if (tag && *tag == cqe.command_id) {
+			*tag = -1;
+			break;
+		}
+	}
 
-	if (likely(nvmeq->cq_vector >= 0))
-		writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
-	nvmeq->cq_head = head;
-	nvmeq->cq_phase = phase;
+	if (consumed)
+		nvme_ring_cq_doorbell(nvmeq);

Quoted 7 lines

Quoted 3 lines

drivers/nvme/host/pci.c +1 -1

-	nvmeq->cqe_seen = 1;
+	return consumed;

}

Won't 'consumed' always be 0 here and we thus never call nvme_ring_cq_doorbell()? Am I overlooking something here, or is this just for preparation of later patches?

that incrementation was lost in a squash. I'll restore it in v2.

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

Quoted 31 lines

Quoted 31 lines

drivers/nvme/host/pci.c +13 -13

+static int __nvme_process_cq(struct nvme_queue *nvmeq, int *tag)
+{
+	struct nvme_completion cqe;
+	int consumed = 0;
 
-	}
+	while (nvme_read_cqe(nvmeq, &cqe)) {
+		nvme_handle_cqe(nvmeq, &cqe);
 
-	/* If the controller ignores the cq head doorbell and continuously
-	 * writes to the queue, it is theoretically possible to wrap around
-	 * the queue twice and mistakenly return IRQ_NONE.  Linux only
-	 * requires that 0.1% of your interrupts are handled, so this isn't
-	 * a big problem.
-	 */
-	if (head == nvmeq->cq_head && phase == nvmeq->cq_phase)
-		return;
+		if (tag && *tag == cqe.command_id) {
+			*tag = -1;
+			break;
+		}
+	}
 
-	if (likely(nvmeq->cq_vector >= 0))
-		writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
-	nvmeq->cq_head = head;
-	nvmeq->cq_phase = phase;
+	if (consumed)
+		nvme_ring_cq_doorbell(nvmeq);

Quoted 7 lines

drivers/nvme/host/pci.c static inline bool nvme_cqe_valid(struct nvme_queue *nvmeq, u16 head, · line 726

Quoted 3 lines

drivers/nvme/host/pci.c +1 -1

-	nvmeq->cqe_seen = 1;
+	return consumed;
 }

Won't 'consumed' always be 0 here and we thus never call nvme_ring_cq_doorbell()? Am I overlooking something here, or is this just for preparation of later patches?

that incrementation was lost in a squash. I'll restore it in v2.

Prepare for irq-poll logic where we budget the completion queue processing context.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
 drivers/nvme/host/pci.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/nvme/host/pci.c +1 -1

index 1d8b35a319d0..85bfd76fbee9 100644
@@ -705,7 +705,7 @@ static inline int nvme_read_cqe(struct nvme_queue *nvmeq,
 	return 0;
 }
 
-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;

irq-poll interface allows us to implement correct cq polling in terms of completion context processing (helps drivers to _not_ abuse irq context) and maintain fairness between multiple completion queues that share the same MSI/MSIX assignments. It is done by mainataining per-cpu completion processing contexts queues and dispatching the completion processing routines a budget that they need to obay.

Also, select IRQ_POLL in Kconfig as we rely on it now.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
 drivers/nvme/host/Kconfig |  1 +
 drivers/nvme/host/pci.c   | 28 +++++++++++++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

drivers/nvme/host/Kconfig +1 -0

index db39d53cdfb9..1523fdaff537 100644
@@ -5,6 +5,7 @@ config BLK_DEV_NVME
 	tristate "NVM Express block device"
 	depends on PCI && BLOCK
 	select NVME_CORE
+	select IRQ_POLL
 	---help---
 	  The NVM Express driver is for solid state drives directly
 	  connected to the PCI or PCI Express bus.  If you know you

drivers/nvme/host/pci.c +1 -0

index 85bfd76fbee9..f37fad10007f 100644
@@ -42,6 +42,7 @@
 #include <linux/types.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <asm/unaligned.h>
+#include <linux/irq_poll.h>
 
 #include "nvme.h"
 

drivers/nvme/host/pci.c line 53

Quoted 6 lines

drivers/nvme/host/pci.c +1 -0

@@ -49,6 +50,7 @@
 #define NVME_AQ_DEPTH		256
 #define SQ_SIZE(depth)		(depth * sizeof(struct nvme_command))
 #define CQ_SIZE(depth)		(depth * sizeof(struct nvme_completion))
+#define NVME_POLL_BUDGET_IRQ	256

Is there a reason for the 256 or is it just a nicely suited value?

Especially
as you've been using INT_MAX as a budget for CQ processing before (which I think is kinda high).

Quoted 8 lines

drivers/nvme/host/pci.c line 53

Quoted 6 lines

drivers/nvme/host/pci.c +1 -0

@@ -49,6 +50,7 @@
 #define NVME_AQ_DEPTH		256
 #define SQ_SIZE(depth)		(depth * sizeof(struct nvme_command))
 #define CQ_SIZE(depth)		(depth * sizeof(struct nvme_completion))
+#define NVME_POLL_BUDGET_IRQ	256

Is there a reason for the 256 or is it just a nicely suited value?

Umm, No good reason behind it, I used this value before for irq-poll and it seemed to fit best. We can try other budgets or have it configurable. The point is to not abuse the soft-irq context for too long and maintain fairness between completion queues so we just need a reasonable value.

Quoted 5 lines

Especially
as you've been using INT_MAX as a budget for CQ processing before (which I think is kinda high).

That was just for keeping the existing logic before we change to irq-poll.

Now that we have irq-poll we don't need to flow control our queues from the submission path. It doesn't make sense to completely drain the completion queue for each submit (or budgeting it either).

Also, now we can remove the forward declaration of nvme_process_cq.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---

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 | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

Quoted 9 lines

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 | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

I don't really see how the call by value vs. call by reference change is a better trade-off than open coding __nvme_process_cq(), as we'd have to duplicate any possible fix for this function then (yes I see it's quite simple but I have some doubt it's 100% bugfree).

Quoted 10 lines

Quoted 9 lines

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 | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

Quoted 4 lines

I don't really see how the call by value vs. call by reference change is a better trade-off than open coding __nvme_process_cq(), as we'd have to duplicate any possible fix for this function then (yes I see it's quite simple but I have some doubt it's 100% bugfree).

I just figured it'd be nicer to remove the tag that we carry in all the call sites because we want to reuse the code in nvme_poll(). It made better sense before, but now that its simpler (for me at least) we can lose it.

Having said that, we can easily remove this patch if we're not all on board...

Now that we use irq-poll and will never call bio_endio from hard irq context we don't need it.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---

I ran some tests with this and it seemed to work pretty well with my low-end nvme devices. One phenomenon I've encountered was that for single core long queue-depth'ed randread workload I saw around ~8-10% iops decrease. However when running multi-core IO I didn't see any noticeable performance degradation. non-polling Canonical randread latency doesn't seem to be affected as well. And also polling mode IO is not affected as expected.

So in addition for review and feedback, this is a call for testing and benchmarking as this touches the critical data path.

Quoted 11 lines

I ran some tests with this and it seemed to work pretty well with my low-end nvme devices. One phenomenon I've encountered was that for single core long queue-depth'ed randread workload I saw around ~8-10% iops decrease. However when running multi-core IO I didn't see any noticeable performance degradation. non-polling Canonical randread latency doesn't seem to be affected as well. And also polling mode IO is not affected as expected.

So in addition for review and feedback, this is a call for testing and benchmarking as this touches the critical data path.

Hi Sagi,

Just reconfirming your findings with another data point, I ran this on controllers with 3D Xpoint media, and single depth 4k random read latency increased almost 7%. I'll try see if there's anything else we can do to bring that in.

Quoted 14 lines

Quoted 11 lines

I ran some tests with this and it seemed to work pretty well with my low-end nvme devices. One phenomenon I've encountered was that for single core long queue-depth'ed randread workload I saw around ~8-10% iops decrease. However when running multi-core IO I didn't see any noticeable performance degradation. non-polling Canonical randread latency doesn't seem to be affected as well. And also polling mode IO is not affected as expected.

So in addition for review and feedback, this is a call for testing and benchmarking as this touches the critical data path.

Hi Sagi,

Quoted 4 lines

Just reconfirming your findings with another data point, I ran this on controllers with 3D Xpoint media, and single depth 4k random read latency increased almost 7%. I'll try see if there's anything else we can do to bring that in.

Actually I didn't notice latency increase with QD=1 but I'm using low-end devices so I might have missed it.
Did you use libaio or psync (for polling mode)?

I'm a bit surprised that scheduling soft-irq (on the same core) is so expensive (the networking folks are using it all over...) Perhaps we need to look into napi and see if we're doing something wrong there...

I wander if we kept the cq processing in queue_rq but budget it to something normally balanced? maybe poll budget to 4 completions?

Does this have any effect with your Xpoint?

--

drivers/nvme/host/pci.c +1 -1

index 150941a1a730..28b33f518a3d 100644
@@ -608,6 +608,7 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
                goto out;
        }
        __nvme_submit_cmd(nvmeq, &cmnd);
+       __nvme_process_cq(nvmeq, 4);
        spin_unlock_irq(&nvmeq->q_lock);
        return BLK_MQ_RQ_QUEUE_OK;
 out:
--

Quoted 4 lines

Actually I didn't notice latency increase with QD=1 but I'm using low-end devices so I might have missed it.
Did you use libaio or psync (for polling mode)?

I used 'sync' ioengine for random read. For sequential, I used 'dd', and it showed the same difference.

If I use pvsync2 with --hipri in fio, then I see little to no difference.

Quoted 4 lines

I'm a bit surprised that scheduling soft-irq (on the same core) is so expensive (the networking folks are using it all over...) Perhaps we need to look into napi and see if we're doing something wrong there...

The latency increase I observed was ~.5 microseconds. That may get lost in the noise for most controllers. Maybe that's within the expected increase using this soft-irq method?

Quoted 4 lines

I wander if we kept the cq processing in queue_rq but budget it to something normally balanced? maybe poll budget to 4 completions?

Does this have any effect with your Xpoint?

No difference with that.

I was actually never sure about having this opprotunistic polling in the IO submission path. For one, it can create higher latency outliers if a different controller's interrupt affinitized to the same CPU posted a completion first.

Another thing we can do if we don't poll for completions in the submission path is split the single q_lock into submission and completion locks. Then we don't have to disable irq's in the submission path. I saw that demo'ed a while ago, and it was a small micro-improvement, but never saw the patch proposed on the public lists..

Currently we have a couple of problems with our completion processing scheme:

1. We abuse the polling context by doing it from hard-irq (which is why the threaded interrupts mode was introduced). We can possibly stay there for too long causing us to hard-lockup (can be triggered easily by running heavy randread workloads on systems with lots of cpu cores).

2. We lack fairness between completion queues that share the same MSI/MSIX assignment (completion queues that belong to different devices). We need to drain a completion queue completely before we can process another completion queue.

irq-poll service solves both by correctly budgeting the completion processing contexts and keeping per-cpu queues of completion queues. By using it we can reduce the number of overall nvme interrupts in the system which is a bonus.

I ran some tests with this and it seemed to work pretty well with my low-end nvme devices. One phenomenon I've encountered was that for single core long queue-depth'ed randread workload I saw around ~8-10% iops decrease. However when running multi-core IO I didn't see any noticeable performance degradation. non-polling Canonical randread latency doesn't seem to be affected as well. And also polling mode IO is not affected as expected.

So in addition for review and feedback, this is a call for testing and benchmarking as this touches the critical data path.

Sagi Grimberg (6):
nvme-pci: Split __nvme_process_cq to poll and handle
nvme-pci: Add budget to __nvme_process_cq
nvme-pci: Use irq-poll for completion processing
nvme: don't consume cq in queue_rq
nvme-pci: open-code polling logic in nvme_poll
nvme-pci: Get rid of threaded interrupts

 drivers/nvme/host/Kconfig |   1 +
 drivers/nvme/host/pci.c   | 179 ++++++++++++++++++++++++++--------------------
 2 files changed, 101 insertions(+), 79 deletions(-)

Signature · 1 line

2.7.4

Hi Sagi,

I'm wondering if I've somehow missed the v2 of this series?

Quoted 2 lines

Hi Sagi,

Hey Johannes,

Quoted 1 line

I'm wondering if I've somehow missed the v2 of this series?

You didn't miss it.

I currently have it in:
git://git.infradead.org/nvme-fabrics.git nvme-irq-poll

From my tests with Qemu nvme I still see ~10% performance degradation for 4K IOs and cannot explain where its coming from. But I think we need some real setup tests as well.

Hence, I don't feel comfortable getting this in at this point. I would appreciate if people can also try it out and provide some insight (Jens, Christoph, Keith).

The networking folks are using napi everywhere and can reach much lower latencies than NVMe so IMO we just need to identify some gaps. I have some micro-optimization patches for irq-poll but they don't seem to make a real difference.