[PATCH] msi-x: let drivers retry when not enough vectors

On Thu, May 07, 2009 at 04:51:24PM +0800, Sheng Yang wrote:
> On Thursday 07 May 2009 16:28:41 Michael S. Tsirkin wrote:
> > pci_enable_msix currently returns -EINVAL if you ask
> > for more vectors than supported by the device, which would
> > typically cause fallback to regular interrupts.
> >
> > It's better to return the table size, making the driver retry
> > MSI-X with less vectors.
>
> Hi Michael
>
> I think driver should read from capability list to know how many vector
> supported by this device before enable MSI-X for device, as
> pci_msix_table_size() did...

Drivers can do this, but it's more code. Since pci_enable_msix
calls pci_msix_table_size already, let it do the work. Right?


> --
> regards
> Yang, Sheng
>
> >
> > Signed-off-by: Michael S. Tsirkin <mst RemoveThis @redhat.com>
> > ---
> >
> > Hi Jesse,
> > This came up when I was adding MSI-X support to virtio pci driver,
> > which does not know the exact table size upfront.
> > Could you consider this patch for 2.6.31 please?
> >
> >
> > drivers/pci/msi.c | 6 +++---
> > 1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 6f2e629..f5bd1c9 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -687,8 +687,8 @@ int pci_msix_table_size(struct pci_dev *dev)
> > * indicates the successful configuration of MSI-X capability structure
> > * with new allocated MSI-X irqs. A return of < 0 indicates a failure.
> > * Or a return of > 0 indicates that driver request is exceeding the
> > number - * of irqs available. Driver should use the returned value to
> > re-send - * its request.
> > + * of irqs or MSI-X vectors available. Driver should use the returned
> > value to + * re-send its request.
> > **/
> > int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries, int
> > nvec) {
> > @@ -704,7 +704,7 @@ int pci_enable_msix(struct pci_dev* dev, struct
> > msix_entry *entries, int nvec)
> >
> > nr_entries = pci_msix_table_size(dev);
> > if (nvec > nr_entries)
> > - return -EINVAL;
> > + return nr_entries;
> >
> > /* Check for any invalid entries */
> > for (i = 0; i < nvec; i++) {
>

--
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thursday 07 May 2009 17:05:06 Michael S. Tsirkin wrote:
> On Thu, May 07, 2009 at 04:51:24PM +0800, Sheng Yang wrote:
> > On Thursday 07 May 2009 16:28:41 Michael S. Tsirkin wrote:
> > > pci_enable_msix currently returns -EINVAL if you ask
> > > for more vectors than supported by the device, which would
> > > typically cause fallback to regular interrupts.
> > >
> > > It's better to return the table size, making the driver retry
> > > MSI-X with less vectors.
> >
> > Hi Michael
> >
> > I think driver should read from capability list to know how many vector
> > supported by this device before enable MSI-X for device, as
> > pci_msix_table_size() did...
>
> Drivers can do this, but it's more code. Since pci_enable_msix
> calls pci_msix_table_size already, let it do the work. Right?

If you don't know the vectors number before you enable MSI-X, how can you
setup vectors?

I don't think it's proper way to go.

--
regards
Yang, Sheng

>
> > --
> > regards
> > Yang, Sheng
> >
> > > Signed-off-by: Michael S. Tsirkin <mst.DeleteThis@redhat.com>
> > > ---
> > >
> > > Hi Jesse,
> > > This came up when I was adding MSI-X support to virtio pci driver,
> > > which does not know the exact table size upfront.
> > > Could you consider this patch for 2.6.31 please?
> > >
> > >
> > > drivers/pci/msi.c | 6 +++---
> > > 1 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > > index 6f2e629..f5bd1c9 100644
> > > --- a/drivers/pci/msi.c
> > > +++ b/drivers/pci/msi.c
> > > @@ -687,8 +687,8 @@ int pci_msix_table_size(struct pci_dev *dev)
> > > * indicates the successful configuration of MSI-X capability
> > > structure * with new allocated MSI-X irqs. A return of < 0 indicates a
> > > failure. * Or a return of > 0 indicates that driver request is
> > > exceeding the number - * of irqs available. Driver should use the
> > > returned value to re-send - * its request.
> > > + * of irqs or MSI-X vectors available. Driver should use the returned
> > > value to + * re-send its request.
> > > **/
> > > int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries,
> > > int nvec) {
> > > @@ -704,7 +704,7 @@ int pci_enable_msix(struct pci_dev* dev, struct
> > > msix_entry *entries, int nvec)
> > >
> > > nr_entries = pci_msix_table_size(dev);
> > > if (nvec > nr_entries)
> > > - return -EINVAL;
> > > + return nr_entries;
> > >
> > > /* Check for any invalid entries */
> > > for (i = 0; i < nvec; i++) {


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thu, May 07, 2009 at 04:51:24PM +0800, Sheng Yang wrote:
> On Thursday 07 May 2009 16:28:41 Michael S. Tsirkin wrote:
> > pci_enable_msix currently returns -EINVAL if you ask
> > for more vectors than supported by the device, which would
> > typically cause fallback to regular interrupts.
> >
> > It's better to return the table size, making the driver retry
> > MSI-X with less vectors.
>
> Hi Michael
>
> I think driver should read from capability list to know how many vector
> supported by this device before enable MSI-X for device, as
> pci_msix_table_size() did...

I think Michael's patch makes sense. It reduces the amount of work the
driver has to do without requiring any additional work in the core. I
don't see the disadvantage to it.

Reviewed-by: Matthew Wilcox <willy.DeleteThis@linux.intel.com>

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thu, May 07, 2009 at 05:10:46PM +0800, Sheng Yang wrote:
> > > I think driver should read from capability list to know how many vector
> > > supported by this device before enable MSI-X for device, as
> > > pci_msix_table_size() did...
> >
> > Drivers can do this, but it's more code. Since pci_enable_msix
> > calls pci_msix_table_size already, let it do the work. Right?
>
> If you don't know the vectors number before you enable MSI-X, how can you
> setup vectors?

I don't know how many irqs I will be able to get anyway.
vectors that can't be assigned are useless ...

--
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thursday 07 May 2009 17:27:31 Matthew Wilcox wrote:
> On Thu, May 07, 2009 at 04:51:24PM +0800, Sheng Yang wrote:
> > On Thursday 07 May 2009 16:28:41 Michael S. Tsirkin wrote:
> > > pci_enable_msix currently returns -EINVAL if you ask
> > > for more vectors than supported by the device, which would
> > > typically cause fallback to regular interrupts.
> > >
> > > It's better to return the table size, making the driver retry
> > > MSI-X with less vectors.
> >
> > Hi Michael
> >
> > I think driver should read from capability list to know how many vector
> > supported by this device before enable MSI-X for device, as
> > pci_msix_table_size() did...
>
> I think Michael's patch makes sense. It reduces the amount of work the
> driver has to do without requiring any additional work in the core. I
> don't see the disadvantage to it.
>
> Reviewed-by: Matthew Wilcox <willy DeleteThis @linux.intel.com>

It's indeed weird. Why the semantic of pci_enable_msix can be changed to
"enable msix, or tell me how many vector do you have"? You can simply call
pci_msix_table_size() to get what you want, also without any more work, no? I
can't understand...

--
regards
Yang, Sheng

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thu, May 07, 2009 at 05:40:15PM +0800, Sheng Yang wrote:
> It's indeed weird. Why the semantic of pci_enable_msix can be changed to
> "enable msix, or tell me how many vector do you have"? You can simply call
> pci_msix_table_size() to get what you want, also without any more work, no? I
> can't understand...

Here's a good example. Let's suppose you have a driver which supports
two different models of cards, one has 16 MSI-X interrupts, the other
has 10. You can call pci_enable_msix() asking for 16 vectors. If your
card is model A, you get 16 interrupts. If your card is model B, it says
"you can have 10".

This is less work in the driver (since it must implement falling back to
a smaller number of interrupts *anyway*) than interrogating the card to
find out how many interrupts there are, then requesting the right number,
and still having the fallback path which is going to be less tested.

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thursday 07 May 2009 17:53:02 Matthew Wilcox wrote:
> On Thu, May 07, 2009 at 05:40:15PM +0800, Sheng Yang wrote:
> > It's indeed weird. Why the semantic of pci_enable_msix can be changed to
> > "enable msix, or tell me how many vector do you have"? You can simply
> > call pci_msix_table_size() to get what you want, also without any more
> > work, no? I can't understand...
>
> Here's a good example. Let's suppose you have a driver which supports
> two different models of cards, one has 16 MSI-X interrupts, the other
> has 10. You can call pci_enable_msix() asking for 16 vectors. If your
> card is model A, you get 16 interrupts. If your card is model B, it says
> "you can have 10".
>
> This is less work in the driver (since it must implement falling back to
> a smaller number of interrupts *anyway*) than interrogating the card to
> find out how many interrupts there are, then requesting the right number,
> and still having the fallback path which is going to be less tested.

Yeah, partly understand now.

But the confusing of return value is not that pleasure compared to this
benefit. And even you have to fall back if return > 0 anyway, but in the past,
you just need fall back once at most; but now you may fall back twice. This
make thing more complex - you need either two ifs or a simple loop. And just
one "if" can deal with it before. All that required is one call for
pci_msix_table_size(), and I believe most driver would like to know how much
vector it have before it fill the vectors, so mostly no extra cost. But for
this ambiguous return meaning, you have to add more code for fall back - yes,
the driver may can assert that the positive return value always would be irq
numbers if it call pci_msix_table_size() before, but is it safe in logic?

--
regards
Yang, Sheng
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thursday 07 May 2009 18:23:50 Michael Ellerman wrote:
> On Thu, 2009-05-07 at 03:53 -0600, Matthew Wilcox wrote:
> > On Thu, May 07, 2009 at 05:40:15PM +0800, Sheng Yang wrote:
> > > It's indeed weird. Why the semantic of pci_enable_msix can be changed
> > > to "enable msix, or tell me how many vector do you have"? You can
> > > simply call pci_msix_table_size() to get what you want, also without
> > > any more work, no? I can't understand...
> >
> > Here's a good example. Let's suppose you have a driver which supports
> > two different models of cards, one has 16 MSI-X interrupts, the other
> > has 10. You can call pci_enable_msix() asking for 16 vectors. If your
> > card is model A, you get 16 interrupts. If your card is model B, it says
> > "you can have 10".
> >
> > This is less work in the driver (since it must implement falling back to
> > a smaller number of interrupts *anyway*) than interrogating the card to
> > find out how many interrupts there are, then requesting the right number,
> > and still having the fallback path which is going to be less tested.
>
> Not to mention that there's no guarantee that you'll get as many
> interrupts as the device supports, so you should really be coding to
> cope with that anyway. Like the example in MSI-HOWTO.txt:
>
> 197 static int foo_driver_enable_msix(struct foo_adapter *adapter, int
> nvec) 198 {
> 199 while (nvec >= FOO_DRIVER_MINIMUM_NVEC) {
> 200 rc = pci_enable_msix(adapter->pdev,
> 201 adapter->msix_entries, nvec);
> 202 if (rc > 0)
> 203 nvec = rc;
> 204 else
> 205 return rc;
> 206 }
> 207
> 208 return -ENOSPC;
> 209 }
>
> So I agree, this patch is an improvement.
>
Oh yeah.

Forgot irq counts can also be changed from time to time.

OK, there should be a loop, so that's fine.

--
regards
Yang, Sheng
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thu, May 07, 2009 at 06:19:53PM +0800, Sheng Yang wrote:
> On Thursday 07 May 2009 17:53:02 Matthew Wilcox wrote:
> > On Thu, May 07, 2009 at 05:40:15PM +0800, Sheng Yang wrote:
> > > It's indeed weird. Why the semantic of pci_enable_msix can be changed to
> > > "enable msix, or tell me how many vector do you have"? You can simply
> > > call pci_msix_table_size() to get what you want, also without any more
> > > work, no? I can't understand...
> >
> > Here's a good example. Let's suppose you have a driver which supports
> > two different models of cards, one has 16 MSI-X interrupts, the other
> > has 10. You can call pci_enable_msix() asking for 16 vectors. If your
> > card is model A, you get 16 interrupts. If your card is model B, it says
> > "you can have 10".
> >
> > This is less work in the driver (since it must implement falling back to
> > a smaller number of interrupts *anyway*) than interrogating the card to
> > find out how many interrupts there are, then requesting the right number,
> > and still having the fallback path which is going to be less tested.
>
> Yeah, partly understand now.
>
> But the confusing of return value is not that pleasure compared to this
> benefit. And even you have to fall back if return > 0 anyway, but in the past,
> you just need fall back once at most; but now you may fall back twice.

I don't think that's right - you might not be able to get the
number of interrupts that pci_enable_msix reported.

> This
> make thing more complex - you need either two ifs or a simple loop. And just
> one "if" can deal with it before. All that required is one call for
> pci_msix_table_size(), and I believe most driver would like to know how much
> vector it have before it fill the vectors, so mostly no extra cost. But for
> this ambiguous return meaning, you have to add more code for fall back - yes,
> the driver may can assert that the positive return value always would be irq
> numbers if it call pci_msix_table_size() before, but is it safe in logic?

If you know how many vectors the card has, then the only failure mode
is when you are out of irqs. No change there.

--
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Michael Ellerman wrote:
> Not to mention that there's no guarantee that you'll get as many
> interrupts as the device supports, so you should really be coding to
> cope with that anyway. Like the example in MSI-HOWTO.txt:
>
> 197 static int foo_driver_enable_msix(struct foo_adapter *adapter, int nvec)
> 198 {
> 199 while (nvec >= FOO_DRIVER_MINIMUM_NVEC) {
> 200 rc = pci_enable_msix(adapter->pdev,
> 201 adapter->msix_entries, nvec);
> 202 if (rc > 0)
> 203 nvec = rc;
> 204 else
> 205 return rc;
> 206 }
> 207
> 208 return -ENOSPC;
> 209 }
>
> So I agree, this patch is an improvement.
>

I imagine this loop is present in many drivers. So why not add a helper

static int pci_enable_msix_min(struct foo_adapter *adapter, int min_nvec)
{
int nvec = 2048;

while (nvec >= min_nvec) {
rc = pci_enable_msix(adapter->pdev,
adapter->msix_entries, nvec);
if (rc == 0)
return nvec;
else if (rc > 0)
nvec = rc;
else
return rc;
}
return -ENOSPC;
}



--
error compiling committee.c: too many arguments to function

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thu, May 07, 2009 at 01:44:38PM +0300, Avi Kivity wrote:
> I imagine this loop is present in many drivers. So why not add a helper

Let's look!

arch/x86/kernel/amd_iommu_init.c : Needs an exact number of vectors.
drivers/block/cciss.c : If it doesn't get all 4 vectors, falls back to pin mode (instead of MSI mode -- bug!)
drivers/dma/ioat_dma.c : Falls back down to 1 vector if it can't get nvec
drivers/infiniband/hw/mthca/mthca_main.c : Reverts to MSI if it can't get 3.
drivers/net/benet/be_main.c : Falls back to MSI if it can't get 2.
drivers/net/bnx2.c : Falls back to MSI if it can't get 9.
drivers/net/bnx2x_main.c : Falls back to MSI if it can't get N.
drivers/net/e1000e/netdev.c: Falls back to MSI if it can't get N.
drivers/net/enic/enic_main.c: Falls back to MSI if it can't get N.
drivers/net/forcedeth.c: Falls back to MSI if it can't get N.
drivers/net/igb/igb_main.c: Falls back to MSI if it can't get N.
drivers/net/igbvf/netdev.c: Falls back to MSI if it can't get 3.
drivers/net/myri10ge/myri10ge.c: Falls back to Pin if it can't get N.
drivers/net/netxen/netxen_nic_main.c: Falls back to MSI if it can't get N.
drivers/net/qlge/qlge_main.c: Falls back to MSI if it can't get N.
drivers/net/s2io.c: Falls back to MSI if it can't get N.
drivers/net/vxge/vxge-main.c: Falls back once to a second number.
drivers/pci/pcie/portdrv_core.c: Falls back to MSI if it can't get all of them.
drivers/scsi/lpfc/lpfc_init.c: Falls back to MSI if it can't get N.
drivers/scsi/mpt2sas/mpt2sas_base.c: Only allocates 1.

drivers/net/cxgb3/cxgb3_main.c: Actually falls back ... in a bit of a weird way. This one could definitely do with the proposed loop.
drivers/net/ixgbe/ixgbe_main.c: Could also be improved with this loop.
drivers/net/mlx4/main.c: Nasty goto-based loop.
drivers/net/niu.c: Ditto
drivers/net/sfc/efx.c: Only falls back once. Would benefit from loop.
drivers/scsi/qla2xxx/qla_isr.c: Only falls back once. Would benefit from loop.
drivers/staging/sxg/sxg.c: /*Should try with less vector returned.*/

so ... 7 drivers would benefit from this loop. 20 seem like they wouldn't.

What a lot of drivers seem to do is fall back either to a single MSI or
just pin-based interrupts when they can't get as many interrupts as they
would like. They don't try to get a single MSI-X interrupt. I feel an
update to the MSI-HOWTO coming on.

--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thu, 7 May 2009 07:49:53 pm Sheng Yang wrote:
> On Thursday 07 May 2009 17:53:02 Matthew Wilcox wrote:
> > Here's a good example. Let's suppose you have a driver which supports
> > two different models of cards, one has 16 MSI-X interrupts, the other
> > has 10. You can call pci_enable_msix() asking for 16 vectors. If your
> > card is model A, you get 16 interrupts. If your card is model B, it says
> > "you can have 10".

Sheng is absolutely right, that's a horrid API.

If it actually enabled that number and returned it, it might make sense (cf.
write() returning less bytes than you give it). But overloading the return
value to save an explicit call is just ugly; it's not worth saving a few lines
of code at cost of making all the drivers subtle and tricksy.

Fail with -ENOSPC or something.

Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, May 08, 2009 at 09:25:00AM +0930, Rusty Russell wrote:
> Sheng is absolutely right, that's a horrid API.

But the API already exists, and is in use by 27 drivers. If this were a
change to the API, I'd be against it, but it is the existing API.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/