Re: [PATCH V2 1/3] PCI/MSI: preference to returning -ENOSPC from pci_alloc_irq_vectors_affinity

[Ming Lei <ming.lei@redhat.com>]

On Mon, Dec 31, 2018 at 04:00:59PM -0600, Bjorn Helgaas wrote:
> On Sat, Dec 29, 2018 at 11:26:48AM +0800, Ming Lei wrote:
> > The API of pci_alloc_irq_vectors_affinity() requires to return -ENOSPC
> > if leass than @min_vecs interrupt vectors are available for @dev.
> 
> s/leass/fewer/
> 
> > However, this way may be changed by falling back to
> > __pci_enable_msi_range(), for example, if the device isn't capable of
> > MSI, __pci_enable_msi_range() will return -EINVAL, and finally it is
> > returned to users of pci_alloc_irq_vectors_affinity() even though
> > there are quite MSIX vectors available. This way violates the interface.
> 
> I *think* the above means:
> 
>   If a device supports MSI-X but not MSI and a caller requests
>   @min_vecs that can't be satisfied by MSI-X, we previously returned
>   -EINVAL (from the failed attempt to enable MSI), not -ENOSPC.
> 
> and I agree that this doesn't match the documented API.

OK, will use the above comment log.

> 
> > Users of pci_alloc_irq_vectors_affinity() may try to reduce irq
> > vectors and allocate vectors again in case that -ENOSPC is returned, such
> > as NVMe, so we need to respect the current interface and give preference to
> > -ENOSPC.
> 
> I thought the whole point of the (min_vecs, max_vecs) tuple was to
> avoid this sort of "reduce and try again" iteration in the callers.

As Keith replied, in case of NVMe, we have to keep min_vecs same with
max_vecs.

> 
> > Cc: Jens Axboe <axboe@fb.com>,
> > Cc: Keith Busch <keith.busch@intel.com>,
> > Cc: linux-pci@vger.kernel.org,
> > Cc: Bjorn Helgaas <bhelgaas@google.com>,
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/pci/msi.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 7a1c8a09efa5..91b4f03fee91 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -1168,7 +1168,8 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
> >  				   const struct irq_affinity *affd)
> >  {
> >  	static const struct irq_affinity msi_default_affd;
> > -	int vecs = -ENOSPC;
> > +	int msix_vecs = -ENOSPC;
> > +	int msi_vecs = -ENOSPC;
> >  
> >  	if (flags & PCI_IRQ_AFFINITY) {
> >  		if (!affd)
> > @@ -1179,16 +1180,17 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
> >  	}
> >  
> >  	if (flags & PCI_IRQ_MSIX) {
> > -		vecs = __pci_enable_msix_range(dev, NULL, min_vecs, max_vecs,
> > -				affd);
> > -		if (vecs > 0)
> > -			return vecs;
> > +		msix_vecs = __pci_enable_msix_range(dev, NULL, min_vecs,
> > +						    max_vecs, affd);
> > +		if (msix_vecs > 0)
> > +			return msix_vecs;
> >  	}
> >  
> >  	if (flags & PCI_IRQ_MSI) {
> > -		vecs = __pci_enable_msi_range(dev, min_vecs, max_vecs, affd);
> > -		if (vecs > 0)
> > -			return vecs;
> > +		msi_vecs = __pci_enable_msi_range(dev, min_vecs, max_vecs,
> > +						  affd);
> > +		if (msi_vecs > 0)
> > +			return msi_vecs;
> >  	}
> >  
> >  	/* use legacy irq if allowed */
> > @@ -1199,7 +1201,7 @@ int pci_alloc_irq_vectors_affinity(struct pci_dev *dev, unsigned int min_vecs,
> >  		}
> >  	}
> >  
> > -	return vecs;
> > +	return msix_vecs == -ENOSPC ? msix_vecs : msi_vecs;
> 
> If you know you want to return -ENOSPC, just return that, not a
> variable that happens to contain it, i.e.,
> 
>   if (msix_vecs == -ENOSPC)
>     return -ENOSPC;
>   return msi_vecs;

OK.

Thanks,
Ming