| Message ID | 20190103210954.11129-3-keith.busch@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/4] nvme-pci: Set tagset nr_maps just once | expand |
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 >
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.
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; }; /**
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(-)