AHCI: Conserve interrupts with pci_enable_msi_block_part() interface

drivers/pci/msi.c +2 -3

-int pci_enable_msi_block_part(struct pci_dev *dev,
-			      unsigned int nvec, int nvec_mme)
+int pci_get_msi_cap(struct pci_dev *dev)
 {
-	int status, maxvec;
+	int ret;
 	u16 msgctl;
 

Hello, Alexander.

On Thu, Sep 05, 2013 at 02:52:47PM +0200, Alexander Gordeev wrote:
One curiosity - with the above factored out, is pci_enable_msi_block_part() returning positive number still necessary? I followed most of code paths in x86 and nothing seems to need it and positive return seems to be just causing confusion - ie. returning 1 on multiple msi config failure from some functions, which is silly.

Thanks.

Signature · 1 line

tejun

You mean we could treat positive numbers returned by architecture as failures and translate it into negative error codes?
If so, I would prefer not to do this for two reasons:
1. It will not be possible to call pci_enable_msi_block_part() in a loop. Although there are no consumers right now I think the very possibility is better to keep.
2. The semantics of pci_enable_msi_block_part() is very close to pci_enable_msi_block(). I believe having a consisting interface for these two helps readability.

The thing is, do we even have cases where arch code returns positive return to indicate possible partial allocation? If not, the whole interface is convoluted for no good reason and we can just make everything return 0 or -errno, which is a lot simpler. No?

Thanks.

Signature · 1 line

tejun

Quoted 4 lines · Tejun Heo

The thing is, do we even have cases where arch code returns positive return to indicate possible partial allocation? If not, the whole interface is convoluted for no good reason and we can just make everything return 0 or -errno, which is a lot simpler. No?

As I mentioned in my other note, at least PPC has a concept of MSI quota, so exceeding it would be the very case, I believe.

Given that multiple MSI is something which isn't too popular / already superseded and that the condition is highly unlikely, do we really care about possible partial success? This sort of interface is unnecessarily complex and actively harmful. It forces all users to wonder what could possibly happen and implement all sorts of nutty fallback logic which is highly likely to be error-prone on both the software and hardware side. Seriously, how much testing would such code path would get both on the driver and firmware sides?

It's an operation which isn't too likely to fail with a firm known-to-work fallback. It's pointless and error-prone to try to extract the last point zero zero one percent.

Thanks.

Signature · 1 line

tejun

I assume reasons for having this type of interface at the moment of taking design decision about pci_enable_msi_block() still hold true. I do not know what those reasons were, but I think the fact multiple MSIs are rarely used and MSI-X exists does not invalidate them now.

I did consider the other argument - since pci_enable_msi_block_part() is explicitly provided with a value of MME the caller will not be satisfied with any other value and hence a repeated call with a lesser MME does not make sense for the caller. Therefore we could just fail in case the architecture returned a positive value. Same result, but different reasoning.

At the moment I still prefer pci_enable_msi_block_part() to be similar to pci_enable_msi_block(). I do agree the fallback logic is error-prone, but I would not dare to scrap it all right away.

Hello, Alexander.

Quoted 4 lines · Alexander Gordeev

I assume reasons for having this type of interface at the moment of taking design decision about pci_enable_msi_block() still hold true. I do not know what those reasons were, but I think the fact multiple MSIs are rarely used and MSI-X exists does not invalidate them now.

Well, it does change the underlying assumptions to make trade-offs against. If something is widely used, expected to continue to expand, additional complexity to achieve better outcome is likely to be more justifiable. Nothing exists in vacuum. That said, I'm not even sure whether we want this sort of interface even if multiple MSI were still the hot thing.

Quoted 6 lines

I did consider the other argument - since pci_enable_msi_block_part() is explicitly provided with a value of MME the caller will not be satisfied with any other value and hence a repeated call with a lesser MME does not make sense for the caller. Therefore we could just fail in case the architecture returned a positive value. Same result, but different reasoning.

Just making the whole thing including arch methods to return 0/-errno would probably be a lot cleaner.

Quoted 3 lines

At the moment I still prefer pci_enable_msi_block_part() to be similar to pci_enable_msi_block(). I do agree the fallback logic is error-prone, but I would not dare to scrap it all right away.

Yeah, of course, pci_enable_msi_block() would need to be updated to match too. I understand this is going a bit off the original scope of the patchset but I can't help but cringing at the interface and the resulting "fallback" logic it ends up creating in its users. Bjorn, what do you think?

Sorry, I haven't jumped in here yet because I saw your discussion and was hoping you guys would figure something out without my help. It will take me a few hours to look into this and come up with anything constructive to say.

I do remember disliking the complicated interface of pci_enable_msi_block() (return negative errno, return positive "we might be able to do this" values, or zero), but I'll have to do some more research before I can say much more than that.

Hello, Bjorn.

Quoted 5 lines · Bjorn Helgaas

Sorry, I haven't jumped in here yet because I saw your discussion and was hoping you guys would figure something out without my help. It will take me a few hours to look into this and come up with anything constructive to say.

Quoted 4 lines

I do remember disliking the complicated interface of pci_enable_msi_block() (return negative errno, return positive "we might be able to do this" values, or zero), but I'll have to do some more research before I can say much more than that.

According to Alexander, it doesn't even seem like we have any actual use case for the positive return numbers. I say just rip it out and do the regular 0/-errno all the way through.

I propose that you rework it that way, and at least find out what (if anything) would break if we do that. Or maybe we just give up some optimization; it would be nice to quantify that, too.

Bjorn

Signed-off-by Alexander Gordeev <[email protected]>

---
  arch/s390/pci/pci.c |    2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)

arch/s390/pci/pci.c +0 -0

index f17a834..c79c6e4 100644

This contradicts to the patch subject.

WBR, Sergei

The series is what it seems a direction to take.

Looks like we need PPC folks to agree on the quota check update for pSeries (yes, they do bail out with a positive return value from arch_msi_check_device()):

	quota = msi_quota_for_device(pdev, nvec);

	if (quota && quota < nvec)
		return quota;

Hello,

Quoted 1 line · Alexander Gordeev

The series is what it seems a direction to take.

It can definitely use better descriptions explaining why this is happening but the general direction seems good to me.

Quoted 8 lines

Looks like we need PPC folks to agree on the quota check update for pSeries (yes, they do bail out with a positive return value from arch_msi_check_device()):

	quota = msi_quota_for_device(pdev, nvec);

	if (quota && quota < nvec)
		return quota;

I think it should return -ENOSPC.

Thanks.

Signature · 1 line

tejun

Quoted 3 lines · Bjorn Helgaas

I propose that you rework it that way, and at least find out what (if anything) would break if we do that. Or maybe we just give up some optimization; it would be nice to quantify that, too.

Hi Bjorn,

The series is what it seems a direction to take.

Looks like we need PPC folks to agree on the quota check update for pSeries (yes, they do bail out with a positive return value from arch_msi_check_device()):

Quoted 5 lines · Alexander Gordeev

Quoted 3 lines · Bjorn Helgaas

I propose that you rework it that way, and at least find out what (if anything) would break if we do that. Or maybe we just give up some optimization; it would be nice to quantify that, too.

Quoted 7 lines

Hi Bjorn,

The series is what it seems a direction to take.

Looks like we need PPC folks to agree on the quota check update for pSeries (yes, they do bail out with a positive return value from arch_msi_check_device()):

Hi Ben,

An initiative to simplify MSI/MSI-X allocation interface is brewing. It seems pSeries quota thing is an obstacle. If it could be given up (patch 2/9).

How about no?

We have a small number of MSIs available, limited by hardware & firmware, if we don't impose a quota then the first device that probes will get most/all of the MSIs and other devices miss out.

Anyway I don't see what problem you're trying to solve? I agree the -ve/0/+ve return value pattern is ugly, but it's hardly the end of the world.

cheers

Quoted 5 lines · Michael Ellerman

How about no?

We have a small number of MSIs available, limited by hardware & firmware, if we don't impose a quota then the first device that probes will get most/all of the MSIs and other devices miss out.

Out of curiosity - how pSeries has had done it without quotas before 448e2ca ("powerpc/pseries: Implement a quota system for MSIs")?

Quoted 3 lines

Anyway I don't see what problem you're trying to solve? I agree the -ve/0/+ve return value pattern is ugly, but it's hardly the end of the world.

Well, the interface recently has been re-classified from "ugly" to "unnecessarily complex and actively harmful" in Tejun's words ;)

Indeed, I checked most of the drivers and it is incredible how people are creative in misusing the interface: from innocent pci_disable_msix() calls when if pci_enable_msix() failed to assuming MSI-Xs were enabled if pci_enable_msix() returned a positive value (apparently untested).

Roughly third of the drivers just do not care and bail out once pci_enable_msix() has not succeeded. Not sure how many of these are mandated by the hardware.

	/*
	 * Retrieving 'nvec' by means other than pci_msix_table_size()
	 */

	rc = pci_get_msix_limit(pdev);
	if (rc < 0)
		return rc;

	/*
	 * nvec = min(rc, nvec);

	 */

	for (i = 0; i < nvec; i++)
		msix_entry[i].entry = i;

	rc = pci_enable_msix(pdev, msix_entry, nvec);
	if (rc)
		return rc;

Quoted 6 lines

Quoted 3 lines

We have a small number of MSIs available, limited by hardware & firmware, if we don't impose a quota then the first device that probes will get most/all of the MSIs and other devices miss out.

Out of curiosity - how pSeries has had done it without quotas before 448e2ca ("powerpc/pseries: Implement a quota system for MSIs")?

Hmmm... do we need to treat this any differently? If the platform can't allocate full range of requested MSIs, just failing should be enough regardless of why such allocation can't be met, no?

I really think what we should do is

* Determine the number of MSIs the controller wants. Don't worry about quotas or limits or anything. Just determine the number necessary to enable enhanced interrupt handling.

* Try allocating that number of MSIs. If it fails, then just revert to single interrupt mode. It's not the end of the world and mostly guaranteed to work. Let's please not even try to do partial multiple interrupts. I really don't think it's worth the risk or complexity.

Thanks.

Signature · 1 line

tejun

* Make pci_enable_msix() return 0/-errno

Quoted 1 line · Alexander Gordeev

* Make pci_enable_msix() return 0/-errno

My choice would be this one.

Quoted 4 lines · Tejun Heo

Quoted 1 line · Alexander Gordeev

* Make pci_enable_msix() return 0/-errno

My choice would be this one.

I agree; it sounds like you've identified several bugs related to the current confusing interface, so fixing that seems like the first step.

I hope we can avoid adding a plethora of interfaces to address unusual corner cases. But if we do the above and it turns out not to be enough, we can always extend it later.

Bjorn

Unfortunately, pSeries is a shows-topper here :( It seems we have to introduce pci_get_msi{,x}_limit() interfaces to honour the quota thing. I just hope the hardware set for pSeries is limited and we won't need to use it for all drivers.

Hello,

Quoted 4 lines · Alexander Gordeev

Unfortunately, pSeries is a shows-topper here :( It seems we have to introduce pci_get_msi{,x}_limit() interfaces to honour the quota thing. I just hope the hardware set for pSeries is limited and we won't need to use it for all drivers.

Can you please go into a bit of detail on that? Why does it matter?

Is it because you're worried you might cause performance regression by forcing prevoius partial multiple allocations to single interrupt operation?

Thanks.

Signature · 1 line

tejun

Subject: Re: [PATCH v2 2/6] PCI/MSI: Factor out pci_get_msi_cap() interface

Quoted 8 lines · Tejun Heo

Hello,

Quoted 4 lines · Alexander Gordeev

Unfortunately, pSeries is a shows-topper here :( It seems we have to introduce pci_get_msi{,x}_limit() interfaces to honour the quota thing. I just hope the hardware set for pSeries is limited and we won't need to use it for all drivers.

Quoted 1 line

Can you please go into a bit of detail on that? Why does it matter?

Because otherwise we will re-introduce a problem described by Michael:
"We have a small number of MSIs available, limited by hardware & firmware, if we don't impose a quota then the first device that probes will get most/all of the MSIs and other devices miss out."

Would it be possible to do some kind of 2-stage allocation. In the first pass the driver would pass a minimum and desired number of MSI-X interrupts, but not actually be given any.

Interrupts could then be allocated after it is known how many are required and how many are available.

David

Quoted 3 lines

Would it be possible to do some kind of 2-stage allocation. In the first pass the driver would pass a minimum and desired number of MSI-X interrupts, but not actually be given any.

Repeated calls to msi_enable_msi/msix() is what we are trying to avoid.

I was thinking that the first call would be done during driver probe (assuming such a time exists) so that the subsystem could determine how many interrupts all the drivers would like, so it can then hand out a smaller number to some of the early drivers in order to have some left to satisfy the minimum requirement of later ones.

So all it would do is sum the requirements of all the drivers.

David

Quoted 1 line

Can you please go into a bit of detail on that? Why does it matter?

Because otherwise we will re-introduce a problem described by Michael:
"We have a small number of MSIs available, limited by hardware & firmware, if we don't impose a quota then the first device that probes will get most/all of the MSIs and other devices miss out."

Quoted 3 lines

Is it because you're worried you might cause performance regression by forcing prevoius partial multiple allocations to single interrupt operation?

Well, not really. I think it won't be possible to force people not to use partial allocations anyway. Some controllers just do not care how many MSIs they are configured with. Some drivers derive the number of MSIs desired from the number of CPUs online - in such cases allocating more MSIs (i.e. a number the controller advertised) could cause a performance degradation even.

Quoted 4 lines

Because otherwise we will re-introduce a problem described by Michael:
"We have a small number of MSIs available, limited by hardware & firmware, if we don't impose a quota then the first device that probes will get most/all of the MSIs and other devices miss out."

Still not following. Why wouldn't just letting the drivers request the optimal number they want and falling back to single interrupt mode work? ie. why can't we just have an all or nothing interface?

tejun

I can imagine a scenario where the first device probes in, requests its

optimal number, acquires that number and exhausts MSIs in pSeries firmware. The next few devices possibly end up with single MSI, since no MSIs left to satisfy their optimal numbers. If one of those single-MSI'ed devices happened to be a high-performance HBA hitting a degraded performance that alone would force (IBM) to introduce the quotas. Now, if the same/another device happened does not support the legacy interrupt mode and no MSI resources have left in the platform firmware at all...

Quoted 8 lines · Tejun Heo

Quoted 4 lines

Because otherwise we will re-introduce a problem described by Michael:
"We have a small number of MSIs available, limited by hardware & firmware, if we don't impose a quota then the first device that probes will get most/all of the MSIs and other devices miss out."

Still not following. Why wouldn't just letting the drivers request the optimal number they want and falling back to single interrupt mode work? ie. why can't we just have an all or nothing interface?

I can imagine a scenario where the first device probes in, requests its optimal number, acquires that number and exhausts MSIs in pSeries firmware. The next few devices possibly end up with single MSI, since no MSIs left to satisfy their optimal numbers. If one of those single-MSI'ed devices happened to be a high-performance HBA hitting a degraded performance that alone would force (IBM) to introduce the quotas.

Actually, I do not see much contradiction with what I proposed. The key words here "determine the number of MSIs the controller wants".

In general case it is not what pci_msix_table_size() returns (or at least we should not limit ourselves to it) - there could be non- standard means to report number of MSIs: hardcoded, version-dependant, device-specific registers etc.

Next, if we opt to determine the number of MSIs by non-MSI standard means then there is no reason not to call pci_get_msix_limit() (or whatever) at this step.

= X number of MSIs and that's the number it should request.

Being Captain Obvious here, but it is up to the device driver to handle a failure. There could be no such option as single MSI mode after all :)

Hello,

Quoted 11 lines · Alexander Gordeev

Actually, I do not see much contradiction with what I proposed. The key words here "determine the number of MSIs the controller wants".

In general case it is not what pci_msix_table_size() returns (or at least we should not limit ourselves to it) - there could be non- standard means to report number of MSIs: hardcoded, version-dependant, device-specific registers etc.

Next, if we opt to determine the number of MSIs by non-MSI standard means then there is no reason not to call pci_get_msix_limit() (or whatever) at this step.

Yeah, that's all fine. My point is that we shouldn't try to use "degraded" multiple MSI mode where the number of MSIs allocated is smaller than performing full multiple MSI operation. How that number is determined doesn't really matter but that number is a property which is solely decided by the device driver, right? If a device needs full multiple MSI mode, given specific configuration, it needs

Quoted 1 line

= X number of MSIs and that's the number it should request.

Quoted 2 lines

Being Captain Obvious here, but it is up to the device driver to handle a failure. There could be no such option as single MSI mode after all :)

I don't think there actually is a mainstream device which can't fallback to single interrupt. Anyways, the point is the same, let's please not try to create an interface which encourages complex retry logic in its users which are likely to involve less traveled and tested paths in both the driver and firmware.

Hello,

Quoted 9 lines · Alexander Gordeev

Quoted 5 lines · Michael Ellerman

How about no?

We have a small number of MSIs available, limited by hardware & firmware, if we don't impose a quota then the first device that probes will get most/all of the MSIs and other devices miss out.

Out of curiosity - how pSeries has had done it without quotas before 448e2ca ("powerpc/pseries: Implement a quota system for MSIs")?

Hmmm... do we need to treat this any differently? If the platform can't allocate full range of requested MSIs, just failing should be enough regardless of why such allocation can't be met, no?

Quoted 6 lines

Quoted 3 lines

Anyway I don't see what problem you're trying to solve? I agree the -ve/0/+ve return value pattern is ugly, but it's hardly the end of the world.

Well, the interface recently has been re-classified from "ugly" to "unnecessarily complex and actively harmful" in Tejun's words ;)

LOL. :)

Quoted 5 lines

Indeed, I checked most of the drivers and it is incredible how people are creative in misusing the interface: from innocent pci_disable_msix() calls when if pci_enable_msix() failed to assuming MSI-Xs were enabled if pci_enable_msix() returned a positive value (apparently untested).

Quoted 3 lines

Roughly third of the drivers just do not care and bail out once pci_enable_msix() has not succeeded. Not sure how many of these are mandated by the hardware.

Yeah, I mean, this type of interface is a trap. People have to actively resist to avoid doing silly stuff which is a lot to ask.

Quoted 10 lines

	/*
	 * Retrieving 'nvec' by means other than pci_msix_table_size()
	 */

	rc = pci_get_msix_limit(pdev);
	if (rc < 0)
		return rc;

	/*
	 * nvec = min(rc, nvec);

Quoted 8 lines

	 */

	for (i = 0; i < nvec; i++)
		msix_entry[i].entry = i;

	rc = pci_enable_msix(pdev, msix_entry, nvec);
	if (rc)
		return rc;

I really think what we should do is

* Determine the number of MSIs the controller wants. Don't worry about quotas or limits or anything. Just determine the number necessary to enable enhanced interrupt handling.

* Try allocating that number of MSIs. If it fails, then just revert to single interrupt mode. It's not the end of the world and mostly guaranteed to work. Let's please not even try to do partial multiple interrupts. I really don't think it's worth the risk or complexity.

Quoted 6 lines

Quoted 3 lines

Roughly third of the drivers just do not care and bail out once pci_enable_msix() has not succeeded. Not sure how many of these are mandated by the hardware.

Yeah, I mean, this type of interface is a trap. People have to actively resist to avoid doing silly stuff which is a lot to ask.

I really think you're overstating the complexity here.

Functions typically return a boolean -> nothing to see here
This function returns a tristate value -> brain explosion!

Quoted 9 lines

* Determine the number of MSIs the controller wants. Don't worry about quotas or limits or anything. Just determine the number necessary to enable enhanced interrupt handling.

* Try allocating that number of MSIs. If it fails, then just revert to single interrupt mode. It's not the end of the world and mostly guaranteed to work. Let's please not even try to do partial multiple interrupts. I really don't think it's worth the risk or complexity.

It will potentially break existing setups on our hardware.

Hello,

Quoted 7 lines · Michael Ellerman

Quoted 6 lines

Quoted 3 lines

Roughly third of the drivers just do not care and bail out once pci_enable_msix() has not succeeded. Not sure how many of these are mandated by the hardware.

Yeah, I mean, this type of interface is a trap. People have to actively resist to avoid doing silly stuff which is a lot to ask.

Quoted 4 lines

I really think you're overstating the complexity here.

Functions typically return a boolean -> nothing to see here
This function returns a tristate value -> brain explosion!

It is an interface which forces the driver writers to write complicated fallback code which won't usually be excercised.

Quoted 10 lines

Quoted 9 lines

* Determine the number of MSIs the controller wants. Don't worry about quotas or limits or anything. Just determine the number necessary to enable enhanced interrupt handling.

* Try allocating that number of MSIs. If it fails, then just revert to single interrupt mode. It's not the end of the world and mostly guaranteed to work. Let's please not even try to do partial multiple interrupts. I really don't think it's worth the risk or complexity.

Quoted 1 line

It will potentially break existing setups on our hardware.

I think it'd be much better to have a separate interface for the drivers which actually require it *in practice* rather than forcing everyone to go "oh this interface supports that, I don't know if I need it but let's implement fallback logic which I won't and have no means of testing".

Are we talking about some limited number of device drivers here?

Also, is the quota still necessary for machines in production today?

Quoted 2 lines

It is an interface which forces the driver writers to write complicated fallback code which won't usually be excercised.

It does not force anyone to do anything. That's just bull.

Code which is unwilling or unable to cope with the extra complexity can simply do:

if (pci_enable_msix(..))

	goto fail;

It's as simple as that.

Quoted 1 line

Are we talking about some limited number of device drivers here?

I don't have a list, but yeah there are certain drivers that folks care about.

Quoted 1 line

Also, is the quota still necessary for machines in production today?

As far as I know yes. The number of MSIs is growing on modern systems, but so is the number of cpus and devices.

The last pattern makes most of sense to me and could be updated with a more clear sequence - a call to (bit modified) pci_msix_table_size() followed by a call to pci_enable_msix(). I think this pattern can effectively supersede the currently recommended "loop" practice.

Quoted 4 lines · Alexander Gordeev

The last pattern makes most of sense to me and could be updated with a more clear sequence - a call to (bit modified) pci_msix_table_size() followed by a call to pci_enable_msix(). I think this pattern can effectively supersede the currently recommended "loop" practice.

The loop is still necessary, because there's a race between those two calls, so that pci_enable_msix() can still fail due to lack of MSIX slots.

Quoted 8 lines · Mark Lord

Quoted 4 lines · Alexander Gordeev

The last pattern makes most of sense to me and could be updated with a more clear sequence - a call to (bit modified) pci_msix_table_size() followed by a call to pci_enable_msix(). I think this pattern can effectively supersede the currently recommended "loop" practice.

The loop is still necessary, because there's a race between those two calls, so that pci_enable_msix() can still fail due to lack of MSIX slots.

Moreover, the existing loop pattern is racy and could fail just as easily ;)

But (1) that is something drivers should expect and (2) there is basically nothing to race against - that is probably the reason it has not been a problem for pSeries. So I think we should not care about this.

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

The last pattern makes most of sense to me and could be updated with a more clear sequence - a call to (bit modified) pci_msix_table_size() followed by a call to pci_enable_msix(). I think this pattern can effectively supersede the currently recommended "loop" practice.

The loop is still necessary, because there's a race between those two calls, so that pci_enable_msix() can still fail due to lack of MSIX slots.

Moreover, the existing loop pattern is racy and could fail just as easily ;)

Yes, but it then loops again to correct things.

Quoted 5 lines · Alexander Gordeev

The last pattern makes most of sense to me and could be updated with a more clear sequence - a call to (bit modified) pci_msix_table_size() followed by a call to pci_enable_msix(). I think this pattern can effectively supersede the currently recommended "loop" practice.

The loop is still necessary, because there's a race between those two calls, so that pci_enable_msix() can still fail due to lack of MSIX slots.

Quoted 5 lines · Michael Ellerman

How about no?

We have a small number of MSIs available, limited by hardware & firmware, if we don't impose a quota then the first device that probes will get most/all of the MSIs and other devices miss out.

Out of curiosity - how pSeries has had done it without quotas before 448e2ca ("powerpc/pseries: Implement a quota system for MSIs")?

Quoted 3 lines

Anyway I don't see what problem you're trying to solve? I agree the -ve/0/+ve return value pattern is ugly, but it's hardly the end of the world.

Well, the interface recently has been re-classified from "ugly" to "unnecessarily complex and actively harmful" in Tejun's words ;)

Indeed, I checked most of the drivers and it is incredible how people are creative in misusing the interface: from innocent pci_disable_msix() calls when if pci_enable_msix() failed to assuming MSI-Xs were enabled if pci_enable_msix() returned a positive value (apparently untested).

Roughly third of the drivers just do not care and bail out once pci_enable_msix() has not succeeded. Not sure how many of these are mandated by the hardware.

Another quite common pattern is a call to pci_enable_msix() to figure out the number of MSI-Xs available and a repeated call of pci_enable_msix() to enable those MSI-Xs, this time.

The recommended practice would be:

	/*
	 * Retrieving 'nvec' by means other than pci_msix_table_size()
	 */

	rc = pci_get_msix_limit(pdev);
	if (rc < 0)
		return rc;

	/*
	 * nvec = min(rc, nvec);
	 */

	for (i = 0; i < nvec; i++)
		msix_entry[i].entry = i;

	rc = pci_enable_msix(pdev, msix_entry, nvec);
	if (rc)
		return rc;

Thoughts?

The disadvantage is that any restriction imposed on us above the quota can only be reported as an error from pci_enable_msix().

The quota code, called from pci_get_msix_limit(), can only do so much to interogate firmware about the limitations. The ultimate way to check if firmware will give us enough MSIs is to try and allocate them. But we can't do that from pci_get_msix_limit() because the driver is not asking us to enable MSIs, just query them.

You'll also need to add another arch hook, for the quota check, and we'll have to add it to our per-platform indirection as well.

All a lot of bother for no real gain IMHO.

cheers

Quoted 8 lines · Michael Ellerman

The disadvantage is that any restriction imposed on us above the quota can only be reported as an error from pci_enable_msix().

The quota code, called from pci_get_msix_limit(), can only do so much to interogate firmware about the limitations. The ultimate way to check if firmware will give us enough MSIs is to try and allocate them. But we can't do that from pci_get_msix_limit() because the driver is not asking us to enable MSIs, just query them.

If things are this way then pci_enable_msix() already exposed to this problem internally on pSeries.

I see that even successful quota checks in rtas_msi_check_device() and rtas_setup_msi_irqs() do not guarantee (as you say) that firmware will give enough MSIs. Hence, pci_enable_msix() might fail even though the its quota checks succeeded.

Therefore, nothing will really change if we make pci_get_msix_limit() check quota and hope the follow-up call to pci_enable_msix() succeeded.

(Of course, we could allocate-deallocate MSIs at check time, but I think it is an overkill).

Quoted 2 lines

You'll also need to add another arch hook, for the quota check, and we'll have to add it to our per-platform indirection as well.

Already, in a branch, hidden from Bjorn & Tejun eyes ;)

Quoted 1 line

All a lot of bother for no real gain IMHO.

Well, I do not have a strong opinion here. I leave it to the ones who have :) But few drivers have became clearer as result of this change (and messy ones are still messy).

Quoted 13 lines · Alexander Gordeev

Quoted 8 lines · Michael Ellerman

The disadvantage is that any restriction imposed on us above the quota can only be reported as an error from pci_enable_msix().

The quota code, called from pci_get_msix_limit(), can only do so much to interogate firmware about the limitations. The ultimate way to check if firmware will give us enough MSIs is to try and allocate them. But we can't do that from pci_get_msix_limit() because the driver is not asking us to enable MSIs, just query them.

If things are this way then pci_enable_msix() already exposed to this problem internally on pSeries.

Quoted 4 lines

I see that even successful quota checks in rtas_msi_check_device() and rtas_setup_msi_irqs() do not guarantee (as you say) that firmware will give enough MSIs. Hence, pci_enable_msix() might fail even though the its quota checks succeeded.

Yes, but it can report that failure to the caller, which can then retry.

Quoted 2 lines

Therefore, nothing will really change if we make pci_get_msix_limit() check quota and hope the follow-up call to pci_enable_msix() succeeded.

No that's not equivalent. Under your scheme if pci_enable_msix() fails then the caller just bails, it will never try again with a lower number.

cheers

Quoted 4 lines · Tejun Heo

The thing is, do we even have cases where arch code returns positive return to indicate possible partial allocation? If not, the whole interface is convoluted for no good reason and we can just make everything return 0 or -errno, which is a lot simpler. No?

As I mentioned in my other note, at least PPC has a concept of MSI quota, so exceeding it would be the very case, I believe.

Make use of the new pci_enable_msi_block_part() interface and conserve on othewise wasted interrupt resources for 10 of 16 unused MSI vectors on Intel chipsets.

Signed-off-by Alexander Gordeev <[email protected]>

One more question tho. How much does saving some interrupt vectors matter? Are int vectors contrained resource on some configurations?

Thanks.

Signature · 1 line

tejun

Do not trust the hardware and always check if MSI Revert to Single Message mode was enforced. Fall back to the single MSI mode in case it did. Not doing so might screw up the interrupt handling.

Signed-off-by Alexander Gordeev <[email protected]>

On Thu, Sep 05, 2013 at 02:54:17PM +0200, Alexander Gordeev wrote:
The patch should probably be re-sequenced to the head of the series and cc [email protected]. Not checking MRSM is scary.

Thanks.

Signature · 1 line

tejun

Do not trust the hardware and always check if MSI Revert to Single Message mode was enforced. Fall back to the single MSI mode in case it did. Not doing so might screw up the interrupt handling.

Signed-off-by Alexander Gordeev <[email protected]>

---
  drivers/ata/ahci.c |   16 ++++++++++++++++
  drivers/ata/ahci.h |    1 +
  2 files changed, 17 insertions(+), 0 deletions(-)

drivers/ata/ahci.c +0 -0

index a6bb618..af5d535 100644