* Re: [PATCH 2/2] nvme pci: try to allocate multiple irq vectors again in case of -EINVAL
[not found] ` <20181226182027.GA5866@lst.de>
@ 2018-12-27 8:21 ` Ming Lei
2018-12-27 13:08 ` Christoph Hellwig
0 siblings, 1 reply; 4+ messages in thread
From: Ming Lei @ 2018-12-27 8:21 UTC (permalink / raw)
To: Christoph Hellwig, Bjorn Helgaas
Cc: Keith Busch, Jens Axboe, linux-nvme, linux-pci
On Wed, Dec 26, 2018 at 07:20:27PM +0100, Christoph Hellwig wrote:
> On Wed, Dec 26, 2018 at 06:37:55PM +0800, Ming Lei wrote:
> > It is observed on QEMU that pci_alloc_irq_vectors_affinity() may
> > returns -EINVAL when the requested number is too big(such as 64).
>
> Which is not how this API is supposed to work and documented to work.
>
> We need to fix pci_alloc_irq_vectors_affinity to not return a spurious
> error and just return the allocated number of vectors instead of
> hacking around that in drivers.
Yeah, you are right.
The issue is that QEMU nvme-pci is MSIX-capable only, and hasn't MSI
capability.
__pci_enable_msix_range() actually returns -ENOSPC, but __pci_enable_msi_range()
returns -EINVAL because dev->msi_cap is zero.
Maybe we need the following fix?
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 265ed3e4c920..b0bf260dc154 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1186,7 +1186,7 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
return vecs;
}
- if (flags & PCI_IRQ_MSI) {
+ if ((flags & PCI_IRQ_MSI) && dev->msi_cap) {
vecs = __pci_enable_msi_range(dev, min_vecs, max_vecs, affd);
if (vecs > 0)
return vecs;
Thanks,
Ming
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] nvme pci: try to allocate multiple irq vectors again in case of -EINVAL
2018-12-27 8:21 ` [PATCH 2/2] nvme pci: try to allocate multiple irq vectors again in case of -EINVAL Ming Lei
@ 2018-12-27 13:08 ` Christoph Hellwig
2018-12-27 22:16 ` Ming Lei
2018-12-31 21:51 ` Bjorn Helgaas
0 siblings, 2 replies; 4+ messages in thread
From: Christoph Hellwig @ 2018-12-27 13:08 UTC (permalink / raw)
To: Ming Lei
Cc: Christoph Hellwig, Bjorn Helgaas, Keith Busch, Jens Axboe,
linux-nvme, linux-pci
On Thu, Dec 27, 2018 at 04:21:38PM +0800, Ming Lei wrote:
> On Wed, Dec 26, 2018 at 07:20:27PM +0100, Christoph Hellwig wrote:
> > On Wed, Dec 26, 2018 at 06:37:55PM +0800, Ming Lei wrote:
> > > It is observed on QEMU that pci_alloc_irq_vectors_affinity() may
> > > returns -EINVAL when the requested number is too big(such as 64).
> >
> > Which is not how this API is supposed to work and documented to work.
> >
> > We need to fix pci_alloc_irq_vectors_affinity to not return a spurious
> > error and just return the allocated number of vectors instead of
> > hacking around that in drivers.
>
> Yeah, you are right.
>
> The issue is that QEMU nvme-pci is MSIX-capable only, and hasn't MSI
> capability.
>
> __pci_enable_msix_range() actually returns -ENOSPC, but __pci_enable_msi_range()
> returns -EINVAL because dev->msi_cap is zero.
>
> Maybe we need the following fix?
Should it matter? We still get a negative vecs back, and still fall
back to the next option. Unless ther are no irqs available at all
for the selected types pci_alloc_irq_vectors_affinity should never
return an error.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] nvme pci: try to allocate multiple irq vectors again in case of -EINVAL
2018-12-27 13:08 ` Christoph Hellwig
@ 2018-12-27 22:16 ` Ming Lei
2018-12-31 21:51 ` Bjorn Helgaas
1 sibling, 0 replies; 4+ messages in thread
From: Ming Lei @ 2018-12-27 22:16 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, linux-pci, linux-nvme, Keith Busch, Bjorn Helgaas
On Thu, Dec 27, 2018 at 02:08:34PM +0100, Christoph Hellwig wrote:
> On Thu, Dec 27, 2018 at 04:21:38PM +0800, Ming Lei wrote:
> > On Wed, Dec 26, 2018 at 07:20:27PM +0100, Christoph Hellwig wrote:
> > > On Wed, Dec 26, 2018 at 06:37:55PM +0800, Ming Lei wrote:
> > > > It is observed on QEMU that pci_alloc_irq_vectors_affinity() may
> > > > returns -EINVAL when the requested number is too big(such as 64).
> > >
> > > Which is not how this API is supposed to work and documented to work.
> > >
> > > We need to fix pci_alloc_irq_vectors_affinity to not return a spurious
> > > error and just return the allocated number of vectors instead of
> > > hacking around that in drivers.
> >
> > Yeah, you are right.
> >
> > The issue is that QEMU nvme-pci is MSIX-capable only, and hasn't MSI
> > capability.
> >
> > __pci_enable_msix_range() actually returns -ENOSPC, but __pci_enable_msi_range()
> > returns -EINVAL because dev->msi_cap is zero.
> >
> > Maybe we need the following fix?
>
> Should it matter? We still get a negative vecs back, and still fall
> back to the next option. Unless ther are no irqs available at all
> for the selected types pci_alloc_irq_vectors_affinity should never
> return an error.
The patch in last email does fix this issue.
In this case, the number of NVMe PCI's MSI-X table entries is 64, so
__pci_enable_msix_range() return -ENOSPC when we ask for 65.
However, the following __pci_enable_msi_range() returns -EINVAL because
the NVMe PCI isn't capable of MSI, then this error is returned from
pci_alloc_irq_vectors_affinity() finally to NVMe driver.
Of course, -EINVAL makes a difference because the current code only
tries to assign one irq vector in this case, and it shouldn't be
returned from pci_alloc_irq_vectors_affinity(), given there is enough
msix entries for fallback, right?
Thanks,
Ming
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] nvme pci: try to allocate multiple irq vectors again in case of -EINVAL
2018-12-27 13:08 ` Christoph Hellwig
2018-12-27 22:16 ` Ming Lei
@ 2018-12-31 21:51 ` Bjorn Helgaas
1 sibling, 0 replies; 4+ messages in thread
From: Bjorn Helgaas @ 2018-12-31 21:51 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Ming Lei, Jens Axboe, linux-pci, linux-nvme, Keith Busch
On Thu, Dec 27, 2018 at 02:08:34PM +0100, Christoph Hellwig wrote:
> On Thu, Dec 27, 2018 at 04:21:38PM +0800, Ming Lei wrote:
> > On Wed, Dec 26, 2018 at 07:20:27PM +0100, Christoph Hellwig wrote:
> > > On Wed, Dec 26, 2018 at 06:37:55PM +0800, Ming Lei wrote:
> > > > It is observed on QEMU that pci_alloc_irq_vectors_affinity() may
> > > > returns -EINVAL when the requested number is too big(such as 64).
> > >
> > > Which is not how this API is supposed to work and documented to work.
> > >
> > > We need to fix pci_alloc_irq_vectors_affinity to not return a spurious
> > > error and just return the allocated number of vectors instead of
> > > hacking around that in drivers.
> >
> > Yeah, you are right.
> >
> > The issue is that QEMU nvme-pci is MSIX-capable only, and hasn't MSI
> > capability.
> >
> > __pci_enable_msix_range() actually returns -ENOSPC, but __pci_enable_msi_range()
> > returns -EINVAL because dev->msi_cap is zero.
> >
> > Maybe we need the following fix?
>
> Should it matter? We still get a negative vecs back, and still fall
> back to the next option.
I'm not sure how it matters either, since
pci_alloc_irq_vectors_affinity() will fail either way. It *would* be
nice to return the correct error in case the caller uses it to emit a
message.
But if the caller wants to use -ENOSPC to reduce @min_vecs and try
again, that sounds like an incorrect use of the interface -- the
caller should have just used the smaller @min_vecs the first time
around.
> Unless ther are no irqs available at all
> for the selected types pci_alloc_irq_vectors_affinity should never
> return an error.
I don't quite understand this last sentence. If @min_vecs == 5 and
the device only supports 4 MSI-X and 4 MSI vectors, the function
comment says we should fail with -ENOSPC.
Bjorn
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-12-31 21:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20181226103755.2101-1-ming.lei@redhat.com>
[not found] ` <20181226103755.2101-3-ming.lei@redhat.com>
[not found] ` <20181226182027.GA5866@lst.de>
2018-12-27 8:21 ` [PATCH 2/2] nvme pci: try to allocate multiple irq vectors again in case of -EINVAL Ming Lei
2018-12-27 13:08 ` Christoph Hellwig
2018-12-27 22:16 ` Ming Lei
2018-12-31 21:51 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).