* [PATCH 2/4] nvme-pci: Distribute io queue types after creation
2019-01-03 21:09 [PATCH 1/4] nvme-pci: Set tagset nr_maps just once Keith Busch
@ 2019-01-03 21:09 ` Keith Busch
2019-01-03 21:09 ` [PATCH 3/4] pci/irq: Handle vector reduce and retry Keith Busch
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Keith Busch @ 2019-01-03 21:09 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig, Sagi Grimberg, Ming Lei,
linux-nvme, Bjorn Helgaas, linux-pci
Cc: Keith Busch
The dev->io_queues types were set based on the results of the nvme set
feature "number of queues" and the IRQ allocation. This does not mean
that we're going to actually successfully allocate and create those IO
queues. A failure there will cause blk-mq to have NULL hctx's because
the map's nr_hw_queues accounts for more queues than were actually
created.
Adjust the io_queue types after we've created them when we've less than
originally desired.
Fixes: 3b6592f70ad7b ("nvme: utilize two queue maps, one for reads and one for writes")
Signed-off-by: Keith Busch <keith.busch@intel.com>
---
drivers/nvme/host/pci.c | 46 ++++++++++++++++++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 6 deletions(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 98332d0a80f0..1481bb6d9c42 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1733,6 +1733,30 @@ static int nvme_pci_configure_admin_queue(struct nvme_dev *dev)
return result;
}
+static void nvme_distribute_queues(struct nvme_dev *dev, unsigned int io_queues)
+{
+ unsigned int irq_queues, this_p_queues = dev->io_queues[HCTX_TYPE_POLL],
+ this_w_queues = dev->io_queues[HCTX_TYPE_DEFAULT];
+
+ if (!io_queues) {
+ dev->io_queues[HCTX_TYPE_POLL] = 0;
+ dev->io_queues[HCTX_TYPE_DEFAULT] = 0;
+ dev->io_queues[HCTX_TYPE_READ] = 0;
+ return;
+ }
+
+ if (this_p_queues >= io_queues)
+ this_p_queues = io_queues - 1;
+ irq_queues = io_queues - this_p_queues;
+
+ if (this_w_queues > irq_queues)
+ this_w_queues = irq_queues;
+
+ dev->io_queues[HCTX_TYPE_POLL] = this_p_queues;
+ dev->io_queues[HCTX_TYPE_DEFAULT] = this_w_queues;
+ dev->io_queues[HCTX_TYPE_READ] = irq_queues - this_w_queues;
+}
+
static int nvme_create_io_queues(struct nvme_dev *dev)
{
unsigned i, max, rw_queues;
@@ -1761,6 +1785,13 @@ static int nvme_create_io_queues(struct nvme_dev *dev)
break;
}
+ /*
+ * If we've created less than expected io queues, redistribute the
+ * dev->io_queues[] types accordingly.
+ */
+ if (dev->online_queues - 1 != dev->max_qid)
+ nvme_distribute_queues(dev, dev->online_queues - 1);
+
/*
* Ignore failing Create SQ/CQ commands, we can continue with less
* than the desired amount of queues, and even a controller without
@@ -2185,11 +2216,6 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
result = max(result - 1, 1);
dev->max_qid = result + dev->io_queues[HCTX_TYPE_POLL];
- dev_info(dev->ctrl.device, "%d/%d/%d default/read/poll queues\n",
- dev->io_queues[HCTX_TYPE_DEFAULT],
- dev->io_queues[HCTX_TYPE_READ],
- dev->io_queues[HCTX_TYPE_POLL]);
-
/*
* Should investigate if there's a performance win from allocating
* more queues than interrupt vectors; it might allow the submission
@@ -2203,7 +2229,15 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
return result;
}
set_bit(NVMEQ_ENABLED, &adminq->flags);
- return nvme_create_io_queues(dev);
+ result = nvme_create_io_queues(dev);
+
+ if (!result)
+ dev_info(dev->ctrl.device, "%d/%d/%d default/read/poll queues\n",
+ dev->io_queues[HCTX_TYPE_DEFAULT],
+ dev->io_queues[HCTX_TYPE_READ],
+ dev->io_queues[HCTX_TYPE_POLL]);
+ return result;
+
}
static void nvme_del_queue_end(struct request *req, blk_status_t error)
--
2.14.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH 3/4] pci/irq: Handle vector reduce and retry
2019-01-03 21:09 [PATCH 1/4] nvme-pci: Set tagset nr_maps just once Keith Busch
2019-01-03 21:09 ` [PATCH 2/4] nvme-pci: Distribute io queue types after creation Keith Busch
@ 2019-01-03 21:09 ` Keith Busch
2019-01-03 21:39 ` Bjorn Helgaas
2019-01-03 21:09 ` [PATCH 4/4] nvme-pci: Use PCI to handle IRQ " Keith Busch
2019-01-04 18:15 ` [PATCH 1/4] nvme-pci: Set tagset nr_maps just once Christoph Hellwig
3 siblings, 1 reply; 8+ messages in thread
From: Keith Busch @ 2019-01-03 21:09 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig, Sagi Grimberg, Ming Lei,
linux-nvme, Bjorn Helgaas, linux-pci
Cc: Keith Busch
Adding the nr_sets forced the driver to handle reducing the vector count
on allocation failures because the set distributino counts are driver
specific. The change to this API is very different to use than before,
and introduced new error corner cases that weren't being handled. It
is also less efficient since the driver doesn't actually know what a
proper vector count it should use since it only sees the error code and
can only reduce by one instead of going straight to a possible vector
count like PCI is able to do.
Provide a driver specific callback for managed irq set creation so that
PCI can take a min and max vectors as before to handle the reduce and
retry logic.
Signed-off-by: Keith Busch <keith.busch@intel.com>
---
drivers/pci/msi.c | 20 ++++++--------------
include/linux/interrupt.h | 4 ++++
2 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 7a1c8a09efa5..b93ac49be18d 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1035,13 +1035,6 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
if (maxvec < minvec)
return -ERANGE;
- /*
- * If the caller is passing in sets, we can't support a range of
- * vectors. The caller needs to handle that.
- */
- if (affd && affd->nr_sets && minvec != maxvec)
- return -EINVAL;
-
if (WARN_ON_ONCE(dev->msi_enabled))
return -EINVAL;
@@ -1061,6 +1054,9 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
return -ENOSPC;
}
+ if (nvec != maxvec && affd && affd->recalc_sets)
+ affd->recalc_sets((struct irq_affinity *)affd, nvec);
+
rc = msi_capability_init(dev, nvec, affd);
if (rc == 0)
return nvec;
@@ -1093,13 +1089,6 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
if (maxvec < minvec)
return -ERANGE;
- /*
- * If the caller is passing in sets, we can't support a range of
- * supported vectors. The caller needs to handle that.
- */
- if (affd && affd->nr_sets && minvec != maxvec)
- return -EINVAL;
-
if (WARN_ON_ONCE(dev->msix_enabled))
return -EINVAL;
@@ -1110,6 +1099,9 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
return -ENOSPC;
}
+ if (nvec != maxvec && affd && affd->recalc_sets)
+ affd->recalc_sets((struct irq_affinity *)affd, nvec);
+
rc = __pci_enable_msix(dev, entries, nvec, affd);
if (rc == 0)
return nvec;
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index c672f34235e7..326c9bd05f62 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -249,12 +249,16 @@ struct irq_affinity_notify {
* the MSI(-X) vector space
* @nr_sets: Length of passed in *sets array
* @sets: Number of affinitized sets
+ * @recalc_sets: Recalculate sets original requested allocation failed
+ * @priv: Driver private data
*/
struct irq_affinity {
int pre_vectors;
int post_vectors;
int nr_sets;
int *sets;
+ void (*recalc_sets)(struct irq_affinity *, unsigned int);
+ void *priv;
};
/**
--
2.14.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 3/4] pci/irq: Handle vector reduce and retry
2019-01-03 21:09 ` [PATCH 3/4] pci/irq: Handle vector reduce and retry Keith Busch
@ 2019-01-03 21:39 ` Bjorn Helgaas
2019-01-03 21:51 ` Keith Busch
0 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2019-01-03 21:39 UTC (permalink / raw)
To: Keith Busch
Cc: Jens Axboe, Christoph Hellwig, Sagi Grimberg, Ming Lei,
linux-nvme, linux-pci
Hi Keith,
Thanks a lot for jumping on this.
I know I'm a broken record. Please run "git log --oneline
drivers/pci/msi.c" and make your subject match.
On Thu, Jan 03, 2019 at 02:09:53PM -0700, Keith Busch wrote:
> Adding the nr_sets forced the driver to handle reducing the vector count
> on allocation failures because the set distributino counts are driver
> specific. The change to this API is very different to use than before,
> and introduced new error corner cases that weren't being handled. It
> is also less efficient since the driver doesn't actually know what a
> proper vector count it should use since it only sees the error code and
> can only reduce by one instead of going straight to a possible vector
> count like PCI is able to do.
>
> Provide a driver specific callback for managed irq set creation so that
> PCI can take a min and max vectors as before to handle the reduce and
> retry logic.
s/distributino/distribution/
s/irq/IRQ/
Can you also add some explanation to Documentation/PCI/MSI-HOWTO.txt
about the concept of "sets" and how to use
nr_sets/sets/recalc_sets/priv?
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
> drivers/pci/msi.c | 20 ++++++--------------
> include/linux/interrupt.h | 4 ++++
> 2 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 7a1c8a09efa5..b93ac49be18d 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1035,13 +1035,6 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
> if (maxvec < minvec)
> return -ERANGE;
>
> - /*
> - * If the caller is passing in sets, we can't support a range of
> - * vectors. The caller needs to handle that.
> - */
> - if (affd && affd->nr_sets && minvec != maxvec)
> - return -EINVAL;
> -
> if (WARN_ON_ONCE(dev->msi_enabled))
> return -EINVAL;
>
> @@ -1061,6 +1054,9 @@ static int __pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec,
> return -ENOSPC;
> }
>
> + if (nvec != maxvec && affd && affd->recalc_sets)
> + affd->recalc_sets((struct irq_affinity *)affd, nvec);
> +
> rc = msi_capability_init(dev, nvec, affd);
> if (rc == 0)
> return nvec;
> @@ -1093,13 +1089,6 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
> if (maxvec < minvec)
> return -ERANGE;
>
> - /*
> - * If the caller is passing in sets, we can't support a range of
> - * supported vectors. The caller needs to handle that.
> - */
> - if (affd && affd->nr_sets && minvec != maxvec)
> - return -EINVAL;
> -
> if (WARN_ON_ONCE(dev->msix_enabled))
> return -EINVAL;
>
> @@ -1110,6 +1099,9 @@ static int __pci_enable_msix_range(struct pci_dev *dev,
> return -ENOSPC;
> }
>
> + if (nvec != maxvec && affd && affd->recalc_sets)
> + affd->recalc_sets((struct irq_affinity *)affd, nvec);
> +
> rc = __pci_enable_msix(dev, entries, nvec, affd);
> if (rc == 0)
> return nvec;
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index c672f34235e7..326c9bd05f62 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -249,12 +249,16 @@ struct irq_affinity_notify {
> * the MSI(-X) vector space
> * @nr_sets: Length of passed in *sets array
> * @sets: Number of affinitized sets
> + * @recalc_sets: Recalculate sets original requested allocation failed
This sentence is missing something and doesn't parse quite right.
"if"? Is "original" superfluous?
> + * @priv: Driver private data
> */
> struct irq_affinity {
> int pre_vectors;
> int post_vectors;
> int nr_sets;
> int *sets;
> + void (*recalc_sets)(struct irq_affinity *, unsigned int);
> + void *priv;
> };
>
> /**
> --
> 2.14.4
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 3/4] pci/irq: Handle vector reduce and retry
2019-01-03 21:39 ` Bjorn Helgaas
@ 2019-01-03 21:51 ` Keith Busch
0 siblings, 0 replies; 8+ messages in thread
From: Keith Busch @ 2019-01-03 21:51 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jens Axboe, Christoph Hellwig, Sagi Grimberg, Ming Lei,
linux-nvme, linux-pci
On Thu, Jan 03, 2019 at 03:39:01PM -0600, Bjorn Helgaas wrote:
> Hi Keith,
>
> Thanks a lot for jumping on this.
>
> I know I'm a broken record. Please run "git log --oneline
> drivers/pci/msi.c" and make your subject match.
Sure thing, my mistake.
> On Thu, Jan 03, 2019 at 02:09:53PM -0700, Keith Busch wrote:
> Can you also add some explanation to Documentation/PCI/MSI-HOWTO.txt
> about the concept of "sets" and how to use
> nr_sets/sets/recalc_sets/priv?
Yes, will do. Do you prefer documentation in a separate patch or the same
one?
> > diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> > index c672f34235e7..326c9bd05f62 100644
> > --- a/include/linux/interrupt.h
> > +++ b/include/linux/interrupt.h
> > @@ -249,12 +249,16 @@ struct irq_affinity_notify {
> > * the MSI(-X) vector space
> > * @nr_sets: Length of passed in *sets array
> > * @sets: Number of affinitized sets
> > + * @recalc_sets: Recalculate sets original requested allocation failed
>
> This sentence is missing something and doesn't parse quite right.
> "if"? Is "original" superfluous?
Yikes, that was a very poor merge of two different thought streams. I'll
fix that up.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 4/4] nvme-pci: Use PCI to handle IRQ reduce and retry
2019-01-03 21:09 [PATCH 1/4] nvme-pci: Set tagset nr_maps just once Keith Busch
2019-01-03 21:09 ` [PATCH 2/4] nvme-pci: Distribute io queue types after creation Keith Busch
2019-01-03 21:09 ` [PATCH 3/4] pci/irq: Handle vector reduce and retry Keith Busch
@ 2019-01-03 21:09 ` Keith Busch
2019-01-04 18:15 ` [PATCH 1/4] nvme-pci: Set tagset nr_maps just once Christoph Hellwig
3 siblings, 0 replies; 8+ messages in thread
From: Keith Busch @ 2019-01-03 21:09 UTC (permalink / raw)
To: Jens Axboe, Christoph Hellwig, Sagi Grimberg, Ming Lei,
linux-nvme, Bjorn Helgaas, linux-pci
Cc: Keith Busch
Restore the previous error handling for vector allocation to the PCI core.
Signed-off-by: Keith Busch <keith.busch@intel.com>
---
drivers/nvme/host/pci.c | 77 ++++++++++++++-----------------------------------
1 file changed, 21 insertions(+), 56 deletions(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 1481bb6d9c42..f3ef09a8e8f9 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2059,37 +2059,43 @@ static int nvme_setup_host_mem(struct nvme_dev *dev)
return ret;
}
-static void nvme_calc_io_queues(struct nvme_dev *dev, unsigned int irq_queues)
+static void nvme_calc_io_queues(struct irq_affinity *affd, unsigned int nvecs)
{
+ struct nvme_dev *dev = affd->priv;
unsigned int this_w_queues = write_queues;
/*
* Setup read/write queue split
*/
- if (irq_queues == 1) {
+ if (nvecs == 1) {
dev->io_queues[HCTX_TYPE_DEFAULT] = 1;
dev->io_queues[HCTX_TYPE_READ] = 0;
- return;
+ goto set_sets;
}
/*
* If 'write_queues' is set, ensure it leaves room for at least
* one read queue
*/
- if (this_w_queues >= irq_queues)
- this_w_queues = irq_queues - 1;
+ if (this_w_queues >= nvecs - 1)
+ this_w_queues = nvecs - 1;
/*
* If 'write_queues' is set to zero, reads and writes will share
* a queue set.
*/
if (!this_w_queues) {
- dev->io_queues[HCTX_TYPE_DEFAULT] = irq_queues;
+ dev->io_queues[HCTX_TYPE_DEFAULT] = nvecs - 1;
dev->io_queues[HCTX_TYPE_READ] = 0;
} else {
dev->io_queues[HCTX_TYPE_DEFAULT] = this_w_queues;
- dev->io_queues[HCTX_TYPE_READ] = irq_queues - this_w_queues;
+ dev->io_queues[HCTX_TYPE_READ] = nvecs - this_w_queues - 1;
}
+set_sets:
+ affd->sets[0] = dev->io_queues[HCTX_TYPE_DEFAULT];
+ affd->sets[1] = dev->io_queues[HCTX_TYPE_READ];
+ if (!affd->sets[1])
+ affd->nr_sets = 1;
}
static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
@@ -2100,9 +2106,10 @@ static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
.pre_vectors = 1,
.nr_sets = ARRAY_SIZE(irq_sets),
.sets = irq_sets,
+ .recalc_sets = nvme_calc_io_queues,
+ .priv = dev,
};
- int result = 0;
- unsigned int irq_queues, this_p_queues;
+ unsigned int nvecs, this_p_queues;
/*
* Poll queues don't need interrupts, but we need at least one IO
@@ -2111,56 +2118,14 @@ static int nvme_setup_irqs(struct nvme_dev *dev, unsigned int nr_io_queues)
this_p_queues = poll_queues;
if (this_p_queues >= nr_io_queues) {
this_p_queues = nr_io_queues - 1;
- irq_queues = 1;
+ nvecs = 2;
} else {
- irq_queues = nr_io_queues - this_p_queues;
+ nvecs = nr_io_queues - this_p_queues + 1;
}
dev->io_queues[HCTX_TYPE_POLL] = this_p_queues;
-
- /*
- * For irq sets, we have to ask for minvec == maxvec. This passes
- * any reduction back to us, so we can adjust our queue counts and
- * IRQ vector needs.
- */
- do {
- nvme_calc_io_queues(dev, irq_queues);
- irq_sets[0] = dev->io_queues[HCTX_TYPE_DEFAULT];
- irq_sets[1] = dev->io_queues[HCTX_TYPE_READ];
- if (!irq_sets[1])
- affd.nr_sets = 1;
-
- /*
- * If we got a failure and we're down to asking for just
- * 1 + 1 queues, just ask for a single vector. We'll share
- * that between the single IO queue and the admin queue.
- */
- if (result >= 0 && irq_queues > 1)
- irq_queues = irq_sets[0] + irq_sets[1] + 1;
-
- result = pci_alloc_irq_vectors_affinity(pdev, irq_queues,
- irq_queues,
- PCI_IRQ_ALL_TYPES | PCI_IRQ_AFFINITY, &affd);
-
- /*
- * Need to reduce our vec counts. If we get ENOSPC, the
- * platform should support mulitple vecs, we just need
- * to decrease our ask. If we get EINVAL, the platform
- * likely does not. Back down to ask for just one vector.
- */
- if (result == -ENOSPC) {
- irq_queues--;
- if (!irq_queues)
- return result;
- continue;
- } else if (result == -EINVAL) {
- irq_queues = 1;
- continue;
- } else if (result <= 0)
- return -EIO;
- break;
- } while (1);
-
- return result;
+ nvme_calc_io_queues(&affd, nvecs);
+ return pci_alloc_irq_vectors_affinity(pdev, affd.pre_vectors, nvecs,
+ PCI_IRQ_ALL_TYPES | PCI_IRQ_AFFINITY, &affd);
}
static int nvme_setup_io_queues(struct nvme_dev *dev)
--
2.14.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 1/4] nvme-pci: Set tagset nr_maps just once
2019-01-03 21:09 [PATCH 1/4] nvme-pci: Set tagset nr_maps just once Keith Busch
` (2 preceding siblings ...)
2019-01-03 21:09 ` [PATCH 4/4] nvme-pci: Use PCI to handle IRQ " Keith Busch
@ 2019-01-04 18:15 ` Christoph Hellwig
2019-01-04 18:19 ` Keith Busch
3 siblings, 1 reply; 8+ messages in thread
From: Christoph Hellwig @ 2019-01-04 18:15 UTC (permalink / raw)
To: Keith Busch
Cc: Jens Axboe, Christoph Hellwig, Sagi Grimberg, Ming Lei,
linux-nvme, Bjorn Helgaas, linux-pci
On Thu, Jan 03, 2019 at 02:09:51PM -0700, Keith Busch wrote:
> The driver overwrites the intermediate nr_map assignments to
> HCTX_MAX_TYPES, so remove those unnecessary temporary settings.
>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
Actually the more complex calculation is the right one here.
I've just merged a patch from Jianchao to remove the other line.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/4] nvme-pci: Set tagset nr_maps just once
2019-01-04 18:15 ` [PATCH 1/4] nvme-pci: Set tagset nr_maps just once Christoph Hellwig
@ 2019-01-04 18:19 ` Keith Busch
0 siblings, 0 replies; 8+ messages in thread
From: Keith Busch @ 2019-01-04 18:19 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Sagi Grimberg, linux-pci, linux-nvme, Ming Lei,
Bjorn Helgaas
On Fri, Jan 04, 2019 at 07:15:28PM +0100, Christoph Hellwig wrote:
> On Thu, Jan 03, 2019 at 02:09:51PM -0700, Keith Busch wrote:
> > The driver overwrites the intermediate nr_map assignments to
> > HCTX_MAX_TYPES, so remove those unnecessary temporary settings.
> >
> > Signed-off-by: Keith Busch <keith.busch@intel.com>
>
> Actually the more complex calculation is the right one here.
> I've just merged a patch from Jianchao to remove the other line.
Yep, I just saw that. Jianchao's is the right way to go here.
^ permalink raw reply [flat|nested] 8+ messages in thread