All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Ming Lei <ming.lei@redhat.com>
Cc: linux-nvme@lists.infradead.org, Christoph Hellwig <hch@lst.de>,
	Jens Axboe <axboe@fb.com>, Keith Busch <keith.busch@intel.com>,
	linux-pci@vger.kernel.org
Subject: Re: [PATCH V2 1/3] PCI/MSI: preference to returning -ENOSPC from pci_alloc_irq_vectors_affinity
Date: Mon, 31 Dec 2018 16:00:59 -0600	[thread overview]
Message-ID: <20181231220059.GI159477@google.com> (raw)
In-Reply-To: <20181229032650.27256-2-ming.lei@redhat.com>

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.

> 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.

> 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;

>  }
>  EXPORT_SYMBOL(pci_alloc_irq_vectors_affinity);
>  
> -- 
> 2.9.5
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

WARNING: multiple messages have this Message-ID (diff)
From: helgaas@kernel.org (Bjorn Helgaas)
Subject: [PATCH V2 1/3] PCI/MSI: preference to returning -ENOSPC from pci_alloc_irq_vectors_affinity
Date: Mon, 31 Dec 2018 16:00:59 -0600	[thread overview]
Message-ID: <20181231220059.GI159477@google.com> (raw)
In-Reply-To: <20181229032650.27256-2-ming.lei@redhat.com>

On Sat, Dec 29, 2018@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.

> 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.

> Cc: Jens Axboe <axboe at fb.com>,
> Cc: Keith Busch <keith.busch at intel.com>,
> Cc: linux-pci at vger.kernel.org,
> Cc: Bjorn Helgaas <bhelgaas at google.com>,
> Signed-off-by: Ming Lei <ming.lei at 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;

>  }
>  EXPORT_SYMBOL(pci_alloc_irq_vectors_affinity);
>  
> -- 
> 2.9.5
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

  reply	other threads:[~2018-12-31 22:01 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-29  3:26 [PATCH V2 0/3] nvme pci: two fixes on nvme_setup_irqs Ming Lei
2018-12-29  3:26 ` Ming Lei
2018-12-29  3:26 ` [PATCH V2 1/3] PCI/MSI: preference to returning -ENOSPC from pci_alloc_irq_vectors_affinity Ming Lei
2018-12-29  3:26   ` Ming Lei
2018-12-31 22:00   ` Bjorn Helgaas [this message]
2018-12-31 22:00     ` Bjorn Helgaas
2018-12-31 22:41     ` Keith Busch
2018-12-31 22:41       ` Keith Busch
2019-01-01  5:24     ` Ming Lei
2019-01-01  5:24       ` Ming Lei
2019-01-02 21:02       ` Bjorn Helgaas
2019-01-02 21:02         ` Bjorn Helgaas
2019-01-02 22:46         ` Keith Busch
2019-01-02 22:46           ` Keith Busch
2018-12-29  3:26 ` [PATCH V2 2/3] nvme pci: fix nvme_setup_irqs() Ming Lei
2018-12-29  3:26 ` [PATCH V2 3/3] nvme pci: introduce module parameter of 'default_queues' Ming Lei
2018-12-31 21:24   ` Bjorn Helgaas
2019-01-01  5:47     ` Ming Lei
2019-01-02  2:14       ` Shan Hai
     [not found]         ` <20190102073607.GA25590@ming.t460p>
     [not found]           ` <d59007c6-af13-318c-5c9d-438ad7d9149d@oracle.com>
     [not found]             ` <20190102083901.GA26881@ming.t460p>
2019-01-03  2:04               ` Shan Hai
2019-01-02 20:11       ` Bjorn Helgaas
2019-01-03  2:12         ` Ming Lei
2019-01-03  2:52           ` Shan Hai
2019-01-03  3:11             ` Shan Hai
2019-01-03  3:31               ` Ming Lei
2019-01-03  4:36                 ` Shan Hai
2019-01-03 10:34                   ` Ming Lei
2019-01-04  2:53                     ` Shan Hai
2019-01-03  4:51                 ` Shan Hai
2019-01-03  3:21             ` Ming Lei
2019-01-14 13:13 ` [PATCH V2 0/3] nvme pci: two fixes on nvme_setup_irqs John Garry
2019-01-14 13:13   ` John Garry

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181231220059.GI159477@google.com \
    --to=helgaas@kernel.org \
    --cc=axboe@fb.com \
    --cc=hch@lst.de \
    --cc=keith.busch@intel.com \
    --cc=linux-nvme@lists.infradead.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=ming.lei@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.