Flush MSI-X table writes (rev 3)
PATCH 1/1 Flush MSI-X table writes (rev 3)
This patch fixes a kernel bug which is triggered when using the irqbalance daemon with MSI-X hardware.
Because both MSI-X interrupt messages and MSI-X table writes are posted, it's possible for them to cross while in-flight. This results in interrupts being received long after the kernel thinks they're disabled, and in interrupts being sent to stale vectors after rebalancing.
This patch performs a read flush after writes to the MSI-X table for mask and unmask operations. Since the SMP affinity is set while the interrupt is masked, and since it's unmasked immediately after, no additional flushes are required in the various affinity setting routines.
This patch has been validated with (unreleased) network hardware which uses MSI-X.
Revised with input from Eric Biederman.
drivers/pci/msi.c +25 -0
@@ -68,6 +68,29 @@ static void msix_set_enable(struct pci_d } } +static void msix_flush_writes (unsigned int irq) +{ +struct msi_desc *entry; + + entry =get_irq_msi (irq); +BUG_ON (!entry || !entry->dev); +switch (entry->msi_attrib.type) { +case PCI_CAP_ID_MSI : +/* nothing to do */ +break ; +case PCI_CAP_ID_MSIX : + { +int offset = entry->msi_attrib.entry_nr *PCI_MSIX_ENTRY_SIZE + +PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET ; +readl (entry->mask_base + offset); +break ; + } +default : +BUG (); +break ; + } +} +static void msi_set_mask_bit (unsigned int irq,int flag) {struct msi_desc *entry;
@@ -186,11 +209,13 @@ void write_msi_msg(unsigned int irq, strvoid mask_msi_irq (unsigned int irq) {msi_set_mask_bit (irq,1 ); +msix_flush_writes (irq); }void unmask_msi_irq (unsigned int irq) {msi_set_mask_bit (irq,0 ); +msix_flush_writes (irq); }static int msi_free_irq (struct pci_dev * dev,int irq);
Quoted 14 lines · Mitch Williams
This patch fixes a kernel bug which is triggered when using the irqbalance daemon with MSI-X hardware.
Because both MSI-X interrupt messages and MSI-X table writes are posted, it's possible for them to cross while in-flight. This results in interrupts being received long after the kernel thinks they're disabled, and in interrupts being sent to stale vectors after rebalancing.
This patch performs a read flush after writes to the MSI-X table for mask and unmask operations. Since the SMP affinity is set while the interrupt is masked, and since it's unmasked immediately after, no additional flushes are required in the various affinity setting routines.
Quoted 4 lines
This patch has been validated with (unreleased) network hardware which uses MSI-X.
Revised with input from Eric Biederman.
Quoted 51 lines
Signed-off-by Mitch Williams <[email protected]>diff -urpN -X dontdiff
linux-2.6.21-rc5-clean/drivers/pci/msi.clinux-2.6.21-rc5/drivers/pci/msi.cdrivers/pci/msi.c renamed from drivers/pci/msi.c 2007-03-28 10:05:24.000000000 -0700 +25
-0@@ -68,6 +68,29 @@ static void msix_set_enable(struct pci_d } } +static void msix_flush_writes (unsigned int irq) +{ +struct msi_desc *entry; + + entry =get_irq_msi (irq); +BUG_ON (!entry || !entry->dev); +switch (entry->msi_attrib.type) { +case PCI_CAP_ID_MSI : +/* nothing to do */ +break ; +case PCI_CAP_ID_MSIX : + { +int offset = entry->msi_attrib.entry_nr *PCI_MSIX_ENTRY_SIZE + +PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET ; +readl (entry->mask_base + offset); +break ; + } +default : +BUG (); +break ; + } +} +static void msi_set_mask_bit (unsigned int irq,int flag) {struct msi_desc *entry;@@ -186,11 +209,13 @@ void write_msi_msg(unsigned int irq, strvoid mask_msi_irq (unsigned int irq) {msi_set_mask_bit (irq,1 ); +msix_flush_writes (irq); }void unmask_msi_irq (unsigned int irq) {msi_set_mask_bit (irq,0 ); +msix_flush_writes (irq); }static int msi_free_irq (struct pci_dev * dev,int irq);
Quoted 16 lines · On Fri, 30 Mar 2007 13:04:02 -0600 [email protected] (Eric W. Biederman)…
Quoted 14 lines · Mitch Williams
This patch fixes a kernel bug which is triggered when using the irqbalance daemon with MSI-X hardware.
Because both MSI-X interrupt messages and MSI-X table writes are posted, it's possible for them to cross while in-flight. This results in interrupts being received long after the kernel thinks they're disabled, and in interrupts being sent to stale vectors after rebalancing.
This patch performs a read flush after writes to the MSI-X table for mask and unmask operations. Since the SMP affinity is set while the interrupt is masked, and since it's unmasked immediately after, no additional flushes are required in the various affinity setting routines.
Quoted 6 lines
Quoted 4 lines
This patch has been validated with (unreleased) network hardware which uses MSI-X.
Revised with input from Eric Biederman.
Did we end up deciding whether this is (needed*safe) enough for 2.6.21?
Quoted 19 lines · Andrew Morton
Quoted 16 lines · On Fri, 30 Mar 2007 13:04:02 -0600 [email protected] (Eric W. Biederman)…
Quoted 14 lines · Mitch Williams
This patch fixes a kernel bug which is triggered when using the irqbalance daemon with MSI-X hardware.
Because both MSI-X interrupt messages and MSI-X table writes are posted, it's possible for them to cross while in-flight. This results in interrupts being received long after the kernel thinks they're disabled, and in interrupts being sent to stale vectors after rebalancing.
This patch performs a read flush after writes to the MSI-X table for mask and unmask operations. Since the SMP affinity is set while the interrupt is masked, and since it's unmasked immediately after, no additional flushes are required in the various affinity setting routines.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
This patch has been validated with (unreleased) network hardware which uses MSI-X.
Revised with input from Eric Biederman.
Did we end up deciding whether this is (needed*safe) enough for 2.6.21?
I say no for now, I have seen no bug reports for any hardware that is not in a lab for this.
thanks,
greg k-h
Quoted 6 lines · On Fri, 30 Mar 2007 12:49:56 -0700 Greg KH <[email protected]> wrote:
Quoted 3 lines
Quoted 1 line
Did we end up deciding whether this is (needed*safe) enough for 2.6.21?
I say no for now, I have seen no bug reports for any hardware that is not in a lab for this.
Well. Presumably that hardware will be in the field during the lifetime of 2.6.21-based kernels.
Perhaps we should put this into 2.6.22 then backport it to 2.6.21.x once it seems safe to do so. If we decide to go this way, we'll need to ask Mitch to remind us to do the backport at the appropriate time, else we'll surely forget.
Quoted 10 lines · Andrew Morton
Quoted 6 lines · On Fri, 30 Mar 2007 12:49:56 -0700 Greg KH <[email protected]> wrote:
Quoted 3 lines
Quoted 1 line
Did we end up deciding whether this is (needed*safe) enough for 2.6.21?
I say no for now, I have seen no bug reports for any hardware that is not in a lab for this.
Quoted 7 lines
Well. Presumably that hardware will be in the field during the lifetime of 2.6.21-based kernels.
Perhaps we should put this into 2.6.22 then backport it to 2.6.21.x once it seems safe to do so. If we decide to go this way, we'll need to ask Mitch to remind us to do the backport at the appropriate time, else we'll surely forget.
Yes, that's what I just asked him to do a message ago :)
thanks,
greg k-h
Quoted 11 lines · Greg KH
Quoted 2 lines
Perhaps we should put this into 2.6.22 then backport it to
2.6.21.x once it
Quoted 1 line
seems safe to do so. If we decide to go this way, we'll
need to ask Mitch
Quoted 1 line
to remind us to do the backport at the appropriate time,
else we'll surely
Quoted 1 line
forget.
Yes, that's what I just asked him to do a message ago :)
Be happy to -- just give me a good definition of "appropriate time".
-Mitch
Quoted 6 lines · Mitch A
Quoted 5 lines · Greg KH
Quoted 2 lines
Perhaps we should put this into 2.6.22 then backport it to
2.6.21.x once it
Quoted 1 line
seems safe to do so. If we decide to go this way, we'll
need to ask Mitch
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
to remind us to do the backport at the appropriate time,
else we'll surely
Quoted 1 line
forget.
Yes, that's what I just asked him to do a message ago :)
Be happy to -- just give me a good definition of "appropriate time".
After it goes into Linus's tree, which will be usually the week after 2.6.21 is out.
thanks,
greg k-h
Quoted 5 lines · Greg KH
Quoted 1 line · Andrew Morton
Did we end up deciding whether this is (needed*safe) enough for 2.6.21?
I say no for now, I have seen no bug reports for any hardware that is not in a lab for this.
The bug report would be phrased as someone seeing "No irq for vector" on x86_64. Unless they are a skilled developer they are unlikely to trace it down to not flushing posted writes to a MSI bar during irq migration. It part it is a subtle hardware/software race.
I have seen some unresolved bug reports described this way on machines that have MSI-X capable hardware. The are against fedora core and I can't haven't been able to get the reporters to try 2.6.21-rcX and the other irq back ports don't work so well.
I currently count 7 drivers in 2.6.21 that are susceptible to the race this bug closes.
This version of the patch seems to meet all of the criteria for being obviously correct, and all it does is insert a stupid readl. Not very dangerous.
This fix removes a race between hardware and software that you likely need to migrate MSI-X irqs a lot to trigger. Which can be the kind of thing that bytes you after you have a month of uptime. This is a very nasty condition to track down from bug reports. I'd rather not have a known culprit out there.
Eric
Quoted 17 lines · Eric W. Biederman
The bug report would be phrased as someone seeing "No irq for vector" on
x86_64. Unless they are a skilled developer they are unlikely to trace it down to not flushing posted writes to a MSI bar during irq migration. It part it is a subtle hardware/software race.I have seen some unresolved bug reports described this way on machines that have MSI-X capable hardware. The are against fedora core and I can't haven't been able to get the reporters to try 2.6.21-rcX and the other irq back ports don't work so well.
I currently count 7 drivers in 2.6.21 that are susceptible to the race this bug closes.
This version of the patch seems to meet all of the criteria for being obviously correct, and all it does is insert a stupid readl. Not very dangerous.
Quoted 5 lines
This fix removes a race between hardware and software that you likely need to migrate MSI-X irqs a lot to trigger. Which can be the kind of thing that bytes you after you have a month of uptime. This is a very nasty condition to track down from bug reports. I'd rather not have a known culprit out there.
Agreed, this is a subtle bug, and was a real hairball to track down. Even so, I'm surprised that nobody else has dug into this, since it should affect anybody running MSI-X. I originally thought I was seeing a hardware bug, which is why I dug more deeply into the issue.
If Eric is seeing bug reports related to "no vector for IRQ" in the wild, then I have to change my stance and agree that this should be pushed to -stable. Every one of those messages indicates that we hit the race condition.
-Mitch
Quoted 6 lines · Mitch A
If Eric is seeing bug reports related to "no vector for IRQ" in the wild, then I have to change my stance and agree that this should be pushed to -stable. Every one of those messages indicates that we hit the race condition.
Your previous patch is in a Fedora test kernel. Hopefully I will have some feedback soon.
Quoted 9 lines · Mitch A
Agreed, this is a subtle bug, and was a real hairball to track down. Even so, I'm surprised that nobody else has dug into this, since it should affect anybody running MSI-X. I originally thought I was seeing a hardware bug, which is why I dug more deeply into the issue.
If Eric is seeing bug reports related to "no vector for IRQ" in the wild, then I have to change my stance and agree that this should be pushed to -stable. Every one of those messages indicates that we hit the race condition.
There is a non MSI-X cause as well with the ioapics that I believe I have now worked around. It was a lot harder because ioapics don't obey the pci ordering rules. Although the primary culprit was receiving a second instance of the an irq before we had acknowledged the first instance.
I had just about concluded that there was likely a second cause for the "no vector for IRQ" message but I had not been able to confirm that until I saw the first version of this patch.
In general the rule for -stable is that a fix has to go to -linus first. It makes it simple to see if the normal QA has been done on the patch before we push it towards stable. So this patch needs to hit 2.6.21 first.
Eric
PATCH 1/1 msi: Immediately mask and unmask msi-x irqs.
In reply to Williams, Mitch AThis is a simplified and actually more comprehensive form of a bug fix from Mitch Williams <[email protected]>.
When we mask or unmask a msi-x irqs the writes may be posted because we are writing to memory mapped region. This means the mask and unmask don't happen immediately but at some unspecified time in the future. Which is out of sync with how the mask/unmask logic work for ioapic irqs.
The practical result is that we get very subtle and hard to track down irq migration bugs.
This patch performs a read flush after writes to the MSI-X table for mask and unmask operations. Since the SMP affinity is set while the interrupt is masked, and since it's unmasked immediately after, no additional flushes are required in the various affinity setting routines.
The testing by Mitch Williams on his especially problematic system should still be valid as I have only simplified the code, not changed the functionality.
We currently have 7 drivers: cciss, mthca, cxgb3, forceth, s2io, pcie/portdrv_core, and qla2xxx in 2.6.21 that are affected by this problem when the hardware they driver is plugged into the right slot.
Given the difficulty of reproducing this bug and tracing it down to anything that even remotely resembles a cause, even if people are being affected we aren't likely to see many meaningful bug reports, and the people who see this bug aren't likely to be able to reproduce this bug in a timely fashion. So it is best to get this problem fixed as soon as we can so people don't have problems.
Then if people do have a kernel message stating "No irq for vector" we will know it is yet another novel cause that needs a complete new investigation.
So here is a one liner that will hopefully be a part of 2.6.21.
--- drivers/pci/msi.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/pci/msi.c +1 -0
@@ -94,6 +94,7 @@ static void msi_set_mask_bit(unsigned int irq, int flag)int offset = entry->msi_attrib.entry_nr *PCI_MSIX_ENTRY_SIZE +PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET ;writel (flag, entry->mask_base + offset); +readl (entry->mask_base + offset);break ; }default :
Signature · 1 line
1.5.0.4
RE: [PATCH] msi: Immediately mask and unmask msi-x irqs.
Quoted 2 lines
This is a simplified and actually more comprehensive form of a bug fix from Mitch Williams <[email protected]>.
[snip]
Quoted 31 lines
Then if people do have a kernel message stating "No irq for vector" we will know it is yet another novel cause that needs a complete new investigation.
So here is a one liner that will hopefully be a part of 2.6.21.
Cc Mitch Williams <[email protected]>Cc Greg KH <[email protected]>Cc Andrew Morton <[email protected]>Signed-off-by "Eric W. Biederman" <[email protected]>---
drivers/pci/msi.c| 1 +1 files changed, 1 insertions(+), 0 deletions(-)drivers/pci/msi.c +0
-0@@ -94,6 +94,7 @@ static void msi_set_mask_bit(unsigned int irq, int flag) int offset = entry->msi_attrib.entry_nr *
PCI_MSIX_ENTRY_SIZE+PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET; writel(flag, entry->mask_base + offset);drivers/pci/msi.c +1
-0+readl (entry->mask_base + offset);break; } default:Signature · 1 line
1.5.0.4
Re: [PATCH] msi: Immediately mask and unmask msi-x irqs.
In reply to Eric W. BiedermanQuoted 6 lines · Eric W. Biederman
When we mask or unmask a msi-x irqs the writes may be posted because we are writing to memory mapped region. This means the mask and unmask don't happen immediately but at some unspecified time in the future. Which is out of sync with how the mask/unmask logic work for ioapic irqs.
Quoted 7 lines
The practical result is that we get very subtle and hard to track down irq migration bugs.
This patch performs a read flush after writes to the MSI-X table for mask and unmask operations. Since the SMP affinity is set while the interrupt is masked, and since it's unmasked immediately after, no additional flushes are required in the various affinity setting routines.
set_msi_irq_affinity() is already doing read_msi_msg(). So the mask operation before this should atleast get flushed before we modify the irq destination information.
With this patch however, unmask happens immediately.
drivers/pci/msi.c static void msi_set_mask_bit(unsigned int irq, int flag) · line 97
Quoted 9 lines
drivers/pci/msi.c +1
-0@@ -94,6 +94,7 @@ static void msi_set_mask_bit(unsigned int irq, int flag)int offset = entry->msi_attrib.entry_nr *PCI_MSIX_ENTRY_SIZE +PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET ;writel (flag, entry->mask_base + offset); +readl (entry->mask_base + offset);
Don't we need the flush for the PCI_CAP_ID_MSI case aswell.
thanks,
suresh
Re: [PATCH] msi: Immediately mask and unmask msi-x irqs.
Quoted 3 lines · Suresh B
set_msi_irq_affinity()is already doingread_msi_msg(). So the mask operation before this should atleast get flushed before we modify the irq destination information.
We modify irq reception information in assign_irq_vector, before that. With my latest delayed free of irq reception information this is less of an issue.
Further now that we cache the msi message that read_msi_msg should go away and we should use the cached version from the msi_desc.
Quoted 1 line
With this patch however, unmask happens immediately.
Yes unmask happens immediately as well. Just as with Mitch Williams last patch, and just like we expect. Further mask/unmask should not be called that often so there is no point in micro optimizing them. At least not until some screams they are a performance problem.
Quoted 11 lines
drivers/pci/msi.c static void msi_set_mask_bit(unsigned int irq, int flag) · line 97
Quoted 9 lines
drivers/pci/msi.c +1
-0@@ -94,6 +94,7 @@ static void msi_set_mask_bit(unsigned int irq, int flag)int offset = entry->msi_attrib.entry_nr *PCI_MSIX_ENTRY_SIZE +PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET ;writel (flag, entry->mask_base + offset); +readl (entry->mask_base + offset);Don't we need the flush for the
PCI_CAP_ID_MSIcase aswell.
At least the 3.0 pci spec does not allow pci configuration access to be posted. So unless we find some hardware that actually does post pci configuration writes we should be ok.
If we do find that hardware that posts pci config writes I expect we will make pci config writes non posted in the generic linux pci config functions, and only an optimized set of pci accessors functions for drivers that really know what they are doing will export the posted behavior.
Eric
Re: [PATCH] msi: Immediately mask and unmask msi-x irqs.
Quoted 4 lines · Eric W. Biederman
"Siddha, Suresh B" <[email protected]> writes:
Further now that we cache the msi message that
read_msi_msgshould go away and we should use the cached version from themsi_desc.
Ok. Then this patch is a must.
Quoted 5 lines
Quoted 1 line
Don't we need the flush for the
PCI_CAP_ID_MSIcase aswell.At least the 3.0 pci spec does not allow pci configuration access to be posted. So unless we find some hardware that actually does post pci configuration writes we should be ok.
ok. Thanks.