* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 9:53 ` Matthew Wilcox
@ 2009-05-07 10:19 ` Sheng Yang
2009-05-07 10:32 ` Michael S. Tsirkin
` (2 more replies)
2009-05-07 10:19 ` Sheng Yang
` (2 subsequent siblings)
3 siblings, 3 replies; 38+ messages in thread
From: Sheng Yang @ 2009-05-07 10:19 UTC (permalink / raw)
To: Matthew Wilcox
Cc: kvm, Michael S. Tsirkin, jbarnes, linux-kernel, linux-pci,
Matthew Wilcox, virtualization
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 10:19 ` Sheng Yang
@ 2009-05-07 10:32 ` Michael S. Tsirkin
2009-05-07 23:55 ` Rusty Russell
2009-05-07 23:55 ` Rusty Russell
2 siblings, 0 replies; 38+ messages in thread
From: Michael S. Tsirkin @ 2009-05-07 10:32 UTC (permalink / raw)
To: Sheng Yang
Cc: kvm, Matthew Wilcox, linux-pci, linux-kernel, jbarnes,
virtualization, Matthew Wilcox
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] msi-x: let drivers retry when not enough vectors
@ 2009-05-07 10:32 ` Michael S. Tsirkin
0 siblings, 0 replies; 38+ messages in thread
From: Michael S. Tsirkin @ 2009-05-07 10:32 UTC (permalink / raw)
To: Sheng Yang
Cc: Matthew Wilcox, kvm, jbarnes, linux-kernel, linux-pci,
Matthew Wilcox, virtualization
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 10:19 ` Sheng Yang
2009-05-07 10:32 ` Michael S. Tsirkin
@ 2009-05-07 23:55 ` Rusty Russell
2009-05-08 0:22 ` Matthew Wilcox
` (4 more replies)
2009-05-07 23:55 ` Rusty Russell
2 siblings, 5 replies; 38+ messages in thread
From: Rusty Russell @ 2009-05-07 23:55 UTC (permalink / raw)
To: virtualization
Cc: Sheng Yang, Matthew Wilcox, kvm, Michael S. Tsirkin, linux-pci,
linux-kernel, jbarnes, Matthew Wilcox
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.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 23:55 ` Rusty Russell
@ 2009-05-08 0:22 ` Matthew Wilcox
2009-05-08 0:22 ` Matthew Wilcox
` (3 subsequent siblings)
4 siblings, 0 replies; 38+ messages in thread
From: Matthew Wilcox @ 2009-05-08 0:22 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Matthew Wilcox, linux-pci, Michael S. Tsirkin, linux-kernel,
jbarnes, virtualization
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.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 23:55 ` Rusty Russell
2009-05-08 0:22 ` Matthew Wilcox
@ 2009-05-08 0:22 ` Matthew Wilcox
2009-05-08 0:28 ` Michael Ellerman
` (2 subsequent siblings)
4 siblings, 0 replies; 38+ messages in thread
From: Matthew Wilcox @ 2009-05-08 0:22 UTC (permalink / raw)
To: Rusty Russell
Cc: virtualization, Sheng Yang, Matthew Wilcox, kvm,
Michael S. Tsirkin, linux-pci, linux-kernel, jbarnes
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.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 23:55 ` Rusty Russell
@ 2009-05-08 0:28 ` Michael Ellerman
2009-05-08 0:22 ` Matthew Wilcox
` (3 subsequent siblings)
4 siblings, 0 replies; 38+ messages in thread
From: Michael Ellerman @ 2009-05-08 0:28 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Matthew Wilcox, linux-pci, Michael S. Tsirkin, linux-kernel,
jbarnes, virtualization, Matthew Wilcox
[-- Attachment #1.1: Type: text/plain, Size: 1955 bytes --]
On Fri, 2009-05-08 at 09:25 +0930, Rusty Russell wrote:
> 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.
It's not horrid, though it is tricky - but only for drivers that care,
you can still do:
if (pci_enable_msix(..))
bail;
> If it actually enabled that number and returned it, it might make sense (cf.
> write() returning less bytes than you give it).
It could do that, but I think that would be worse. The driver, on
finding out it can only get a certain number of MSIs might need to make
a decision about how it allocates those - eg. in a network driver,
sharing them between TX/RX/management.
And in practice most of the drivers just bail if they can't get what
they asked for, so enabling less than they wanted would just mean they
have to go and disable them.
> 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.
Looking at just this patch, I would agree, but unfortunately it's not
that simple. The first limitation on the number of MSIs the driver can
have is the number the device supports, that's what this patch does. But
there are others, and they come out of the arch code, or even the
firmware. So to implement pci_how_many_msis(), we'd need a parallel API
all the way down to the arch code, or a flag to all the existing
routines saying "don't really allocate, just find out". That would be
horrid IMHO.
cheers
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
[-- Attachment #2: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] msi-x: let drivers retry when not enough vectors
@ 2009-05-08 0:28 ` Michael Ellerman
0 siblings, 0 replies; 38+ messages in thread
From: Michael Ellerman @ 2009-05-08 0:28 UTC (permalink / raw)
To: Rusty Russell
Cc: virtualization, Sheng Yang, Matthew Wilcox, kvm,
Michael S. Tsirkin, linux-pci, linux-kernel, jbarnes,
Matthew Wilcox
[-- Attachment #1: Type: text/plain, Size: 1955 bytes --]
On Fri, 2009-05-08 at 09:25 +0930, Rusty Russell wrote:
> 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.
It's not horrid, though it is tricky - but only for drivers that care,
you can still do:
if (pci_enable_msix(..))
bail;
> If it actually enabled that number and returned it, it might make sense (cf.
> write() returning less bytes than you give it).
It could do that, but I think that would be worse. The driver, on
finding out it can only get a certain number of MSIs might need to make
a decision about how it allocates those - eg. in a network driver,
sharing them between TX/RX/management.
And in practice most of the drivers just bail if they can't get what
they asked for, so enabling less than they wanted would just mean they
have to go and disable them.
> 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.
Looking at just this patch, I would agree, but unfortunately it's not
that simple. The first limitation on the number of MSIs the driver can
have is the number the device supports, that's what this patch does. But
there are others, and they come out of the arch code, or even the
firmware. So to implement pci_how_many_msis(), we'd need a parallel API
all the way down to the arch code, or a flag to all the existing
routines saying "don't really allocate, just find out". That would be
horrid IMHO.
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 23:55 ` Rusty Russell
` (2 preceding siblings ...)
2009-05-08 0:28 ` Michael Ellerman
@ 2009-05-12 21:28 ` Michael S. Tsirkin
2009-05-12 21:28 ` Michael S. Tsirkin
4 siblings, 0 replies; 38+ messages in thread
From: Michael S. Tsirkin @ 2009-05-12 21:28 UTC (permalink / raw)
To: Rusty Russell
Cc: virtualization, Sheng Yang, Matthew Wilcox, kvm, linux-pci,
linux-kernel, jbarnes, Matthew Wilcox
On Fri, May 08, 2009 at 09:25:00AM +0930, Rusty Russell wrote:
> 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.
I do agree that returning a positive value from pci_enable_msix
it an ugly API (but note that this is the API that linux currently has).
Here's a wrapper that I ended up with in my driver:
static int enable_msix(struct pci_dev *dev, struct msix_entry *entries,
int *options, int noptions)
{
int i;
for (i = 0; i < noptions; ++i)
if (!pci_enable_msix(dev, entries, options[i]))
return options[i];
return -EBUSY;
}
This gets an array of options for # of vectors and tries them one after
the other until an option that the system can support is found.
On success, we get the # of vectors actually enabled, and
driver can then use them as it sees fit.
Is there interest in moving something like this to pci.h?
--
MST
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 23:55 ` Rusty Russell
` (3 preceding siblings ...)
2009-05-12 21:28 ` Michael S. Tsirkin
@ 2009-05-12 21:28 ` Michael S. Tsirkin
4 siblings, 0 replies; 38+ messages in thread
From: Michael S. Tsirkin @ 2009-05-12 21:28 UTC (permalink / raw)
To: Rusty Russell
Cc: kvm, Matthew Wilcox, linux-pci, linux-kernel, jbarnes,
virtualization, Matthew Wilcox
On Fri, May 08, 2009 at 09:25:00AM +0930, Rusty Russell wrote:
> 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.
I do agree that returning a positive value from pci_enable_msix
it an ugly API (but note that this is the API that linux currently has).
Here's a wrapper that I ended up with in my driver:
static int enable_msix(struct pci_dev *dev, struct msix_entry *entries,
int *options, int noptions)
{
int i;
for (i = 0; i < noptions; ++i)
if (!pci_enable_msix(dev, entries, options[i]))
return options[i];
return -EBUSY;
}
This gets an array of options for # of vectors and tries them one after
the other until an option that the system can support is found.
On success, we get the # of vectors actually enabled, and
driver can then use them as it sees fit.
Is there interest in moving something like this to pci.h?
--
MST
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 10:19 ` Sheng Yang
2009-05-07 10:32 ` Michael S. Tsirkin
2009-05-07 23:55 ` Rusty Russell
@ 2009-05-07 23:55 ` Rusty Russell
2 siblings, 0 replies; 38+ messages in thread
From: Rusty Russell @ 2009-05-07 23:55 UTC (permalink / raw)
To: virtualization
Cc: kvm, Matthew Wilcox, linux-pci, Michael S. Tsirkin, linux-kernel,
jbarnes, Matthew Wilcox
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.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 9:53 ` Matthew Wilcox
2009-05-07 10:19 ` Sheng Yang
@ 2009-05-07 10:19 ` Sheng Yang
2009-05-07 10:23 ` Michael Ellerman
2009-05-07 10:23 ` Michael Ellerman
3 siblings, 0 replies; 38+ messages in thread
From: Sheng Yang @ 2009-05-07 10:19 UTC (permalink / raw)
To: Matthew Wilcox
Cc: kvm, Michael S. Tsirkin, linux-pci, linux-kernel, jbarnes,
virtualization, Matthew Wilcox
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 9:53 ` Matthew Wilcox
2009-05-07 10:19 ` Sheng Yang
2009-05-07 10:19 ` Sheng Yang
@ 2009-05-07 10:23 ` Michael Ellerman
2009-05-07 10:23 ` Michael Ellerman
3 siblings, 0 replies; 38+ messages in thread
From: Michael Ellerman @ 2009-05-07 10:23 UTC (permalink / raw)
To: Matthew Wilcox
Cc: kvm, Michael S. Tsirkin, linux-pci, linux-kernel, jbarnes,
virtualization, Matthew Wilcox
[-- Attachment #1.1: Type: text/plain, Size: 1738 bytes --]
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.
cheers
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
[-- Attachment #2: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 9:53 ` Matthew Wilcox
` (2 preceding siblings ...)
2009-05-07 10:23 ` Michael Ellerman
@ 2009-05-07 10:23 ` Michael Ellerman
2009-05-07 10:28 ` Sheng Yang
` (3 more replies)
3 siblings, 4 replies; 38+ messages in thread
From: Michael Ellerman @ 2009-05-07 10:23 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Sheng Yang, kvm, Michael S. Tsirkin, jbarnes, linux-kernel,
linux-pci, Matthew Wilcox, virtualization
[-- Attachment #1: Type: text/plain, Size: 1738 bytes --]
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.
cheers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 10:23 ` Michael Ellerman
@ 2009-05-07 10:28 ` Sheng Yang
2009-05-07 10:28 ` Sheng Yang
` (2 subsequent siblings)
3 siblings, 0 replies; 38+ messages in thread
From: Sheng Yang @ 2009-05-07 10:28 UTC (permalink / raw)
To: michael
Cc: Matthew Wilcox, kvm, Michael S. Tsirkin, jbarnes, linux-kernel,
linux-pci, Matthew Wilcox, virtualization
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 10:23 ` Michael Ellerman
2009-05-07 10:28 ` Sheng Yang
@ 2009-05-07 10:28 ` Sheng Yang
2009-05-07 10:44 ` Avi Kivity
2009-05-07 10:44 ` Avi Kivity
3 siblings, 0 replies; 38+ messages in thread
From: Sheng Yang @ 2009-05-07 10:28 UTC (permalink / raw)
To: michael
Cc: kvm, Michael S. Tsirkin, linux-pci, Matthew Wilcox, linux-kernel,
jbarnes, virtualization, Matthew Wilcox
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 10:23 ` Michael Ellerman
2009-05-07 10:28 ` Sheng Yang
2009-05-07 10:28 ` Sheng Yang
@ 2009-05-07 10:44 ` Avi Kivity
2009-05-07 10:44 ` Avi Kivity
3 siblings, 0 replies; 38+ messages in thread
From: Avi Kivity @ 2009-05-07 10:44 UTC (permalink / raw)
To: michael
Cc: kvm, Michael S. Tsirkin, linux-pci, Matthew Wilcox, linux-kernel,
jbarnes, virtualization, Matthew Wilcox
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 10:23 ` Michael Ellerman
` (2 preceding siblings ...)
2009-05-07 10:44 ` Avi Kivity
@ 2009-05-07 10:44 ` Avi Kivity
2009-05-07 11:57 ` Matthew Wilcox
2009-05-07 11:57 ` Matthew Wilcox
3 siblings, 2 replies; 38+ messages in thread
From: Avi Kivity @ 2009-05-07 10:44 UTC (permalink / raw)
To: michael
Cc: Matthew Wilcox, Sheng Yang, kvm, Michael S. Tsirkin, jbarnes,
linux-kernel, linux-pci, Matthew Wilcox, virtualization
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 10:44 ` Avi Kivity
@ 2009-05-07 11:57 ` Matthew Wilcox
2009-05-07 11:57 ` Matthew Wilcox
1 sibling, 0 replies; 38+ messages in thread
From: Matthew Wilcox @ 2009-05-07 11:57 UTC (permalink / raw)
To: Avi Kivity
Cc: kvm, Michael S. Tsirkin, linux-pci, linux-kernel, jbarnes,
virtualization, michael, Matthew Wilcox
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."
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] msi-x: let drivers retry when not enough vectors
2009-05-07 10:44 ` Avi Kivity
2009-05-07 11:57 ` Matthew Wilcox
@ 2009-05-07 11:57 ` Matthew Wilcox
1 sibling, 0 replies; 38+ messages in thread
From: Matthew Wilcox @ 2009-05-07 11:57 UTC (permalink / raw)
To: Avi Kivity
Cc: michael, Sheng Yang, kvm, Michael S. Tsirkin, jbarnes,
linux-kernel, linux-pci, Matthew Wilcox, virtualization
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."
^ permalink raw reply [flat|nested] 38+ messages in thread