[PATCHv2] NVMe: Stripe queue IRQ vector assignments

[PATCHv2] NVMe: Stripe queue IRQ vector assignments

[Jon Derrick]

This patch will stripe io queue IRQ vector assignments across the
available vector resources. This also changes the current implementation
which reduces the number of io queues to the number of vectors enabled.
This fixes another issue where the admin queue and io queue 0 were
unnecessarily sharing an vector.

The benefits of this are two-fold:
a) There is a known issue in one controller where irq coalescing cannot
occur on vectors to which the admin queue is assigned. This patch assigns
the admin queue its own vector, as long as there are enough vectors
available to assign one to each io queue and the admin queue.
b) If a suitable number of vector resources cannot be acquired to match
the number of io queues, the current implementation will reduce the
number of io queues. There is a likely performance benefit to keeping
the number of io queues equal to the number of possible cpus, even if
the vectors have to be shared among the queues.

Signed-off-by: Jon Derrick <jonathan.derrick at intel.com>
---
 drivers/block/nvme-core.c | 12 ++++++------
 include/linux/nvme.h      |  1 +
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 2072ae8..ef450b8 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -1507,7 +1507,7 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
 	struct nvme_dev *dev = nvmeq->dev;
 	int result;
 
-	nvmeq->cq_vector = qid - 1;
+	nvmeq->cq_vector = qid % dev->vec_count;
 	result = adapter_alloc_cq(dev, qid, nvmeq);
 	if (result < 0)
 		return result;
@@ -2175,11 +2175,11 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	if (!pdev->irq)
 		pci_disable_msix(pdev);
 
-	for (i = 0; i < nr_io_queues; i++)
+	for (i = 0; i <= nr_io_queues; i++)
 		dev->entry[i].entry = i;
-	vecs = pci_enable_msix_range(pdev, dev->entry, 1, nr_io_queues);
+	vecs = pci_enable_msix_range(pdev, dev->entry, 1, nr_io_queues + 1);
 	if (vecs < 0) {
-		vecs = pci_enable_msi_range(pdev, 1, min(nr_io_queues, 32));
+		vecs = pci_enable_msi_range(pdev, 1, min(nr_io_queues + 1, 32));
 		if (vecs < 0) {
 			vecs = 1;
 		} else {
@@ -2194,7 +2194,7 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	 * path to scale better, even if the receive path is limited by the
 	 * number of interrupts.
 	 */
-	nr_io_queues = vecs;
+	dev->vec_count = vecs;
 	dev->max_qid = nr_io_queues;
 
 	result = queue_request_irq(dev, adminq, adminq->irqname);
@@ -2990,7 +2990,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	dev = kzalloc_node(sizeof(*dev), GFP_KERNEL, node);
 	if (!dev)
 		return -ENOMEM;
-	dev->entry = kzalloc_node(num_possible_cpus() * sizeof(*dev->entry),
+	dev->entry = kzalloc_node((num_possible_cpus() + 1) * sizeof(*dev->entry),
 							GFP_KERNEL, node);
 	if (!dev->entry)
 		goto free;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index c0d94ed..a8e64aa 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -79,6 +79,7 @@ struct nvme_dev {
 	struct dma_pool *prp_small_pool;
 	int instance;
 	unsigned queue_count;
+	unsigned vec_count;
 	unsigned online_queues;
 	unsigned max_qid;
 	int q_depth;
-- 
2.1.4

[PATCHv2] NVMe: Stripe queue IRQ vector assignments

[Christoph Hellwig]

On Wed, Jun 10, 2015@10:41:30AM -0600, Jon Derrick wrote:
> a) There is a known issue in one controller where irq coalescing cannot
> occur on vectors to which the admin queue is assigned. This patch assigns
> the admin queue its own vector, as long as there are enough vectors
> available to assign one to each io queue and the admin queue.

I think this should be a quirk for that particular PCI ID, and needs
a comment int the code explaining the reason.

[PATCHv2] NVMe: Stripe queue IRQ vector assignments

[Keith Busch]

On Wed, 10 Jun 2015, Christoph Hellwig wrote:
> On Wed, Jun 10, 2015@10:41:30AM -0600, Jon Derrick wrote:
>> a) There is a known issue in one controller where irq coalescing cannot
>> occur on vectors to which the admin queue is assigned. This patch assigns
>> the admin queue its own vector, as long as there are enough vectors
>> available to assign one to each io queue and the admin queue.
>
> I think this should be a quirk for that particular PCI ID, and needs
> a comment int the code explaining the reason.

Let's just remove this from the commit message. The real win is the second
part, which we know to be a worthwhile gain in senarios where h/w queues
and cpus exceed available irq's. This other coalescing benefit is a nice
bonus, but not necessary to consider for this commit.

[PATCHv2] NVMe: Stripe queue IRQ vector assignments

[Christoph Hellwig]

On Thu, Jun 11, 2015@12:22:27AM +0000, Keith Busch wrote:
> On Wed, 10 Jun 2015, Christoph Hellwig wrote:
> >On Wed, Jun 10, 2015@10:41:30AM -0600, Jon Derrick wrote:
> >>a) There is a known issue in one controller where irq coalescing cannot
> >>occur on vectors to which the admin queue is assigned. This patch assigns
> >>the admin queue its own vector, as long as there are enough vectors
> >>available to assign one to each io queue and the admin queue.
> >
> >I think this should be a quirk for that particular PCI ID, and needs
> >a comment int the code explaining the reason.
> 
> Let's just remove this from the commit message. The real win is the second
> part, which we know to be a worthwhile gain in senarios where h/w queues
> and cpus exceed available irq's. This other coalescing benefit is a nice
> bonus, but not necessary to consider for this commit.

Sorry if I wasn't clear - I meant the fact that we avoid the vectors
that have the admin queue assigned for I?O queue vectors.  While it's
fine to use separate vectors if we have enough I'd rather share an I/O
queue with the admin queue than with another I/O queue if that's not the
case.

The limitation seems odd enough and specific to a piece of silicone that
I think we really need a comment and even better a quirk to document it.

[PATCHv2] NVMe: Stripe queue IRQ vector assignments

[Keith Busch]

On Thu, 11 Jun 2015, Christoph Hellwig wrote:
> Sorry if I wasn't clear - I meant the fact that we avoid the vectors
> that have the admin queue assigned for I?O queue vectors.  While it's
> fine to use separate vectors if we have enough I'd rather share an I/O
> queue with the admin queue than with another I/O queue if that's not the
> case.

Right, I think Jon's patch captures that. If there are not enough for
1:1, the irqs are shared modulo the number of vectors, so the first
shared vector is the admin queue's vector 0.

> The limitation seems odd enough and specific to a piece of silicone that
> I think we really need a comment and even better a quirk to document it.

It's an errata, but I can imagine what the implementers were thinking:
per the spec, the admin queue never participates in interrupt coalescing,
and the admin queue always gets interrupt vector 0. The feature was
incorrectly tied to the vector rather than the queue.

[PATCHv2] NVMe: Stripe queue IRQ vector assignments

[Christoph Hellwig]

On Thu, Jun 11, 2015@02:10:17PM +0000, Keith Busch wrote:
> Right, I think Jon's patch captures that. If there are not enough for
> 1:1, the irqs are shared modulo the number of vectors, so the first
> shared vector is the admin queue's vector 0.

Right.  Reading through it that's indeed the case, it's just the
description that confused me.

[PATCHv2] NVMe: Stripe queue IRQ vector assignments

[Keith Busch]

On Wed, 10 Jun 2015, Jon Derrick wrote:
> This patch will stripe io queue IRQ vector assignments across the
> available vector resources. This also changes the current implementation
> which reduces the number of io queues to the number of vectors enabled.
> This fixes another issue where the admin queue and io queue 0 were
> unnecessarily sharing an vector.

Just want to make a couple points, but these concerns shouldn't hold
up acceptance since this is still better than what we had before, and
I don't see an obvious solution to either problem.

1, This will break the irq affinity hints.

2, We don't control or know which CPUs share the h/w queues when you assign
the queue's vector, so we could create sub-optimal sharing.