MSI: read-flush MSI-X table

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 enable/disable and rebalancing operations. Because this is an expensive operation, we do not perform the read flush after mask/unmask operations. Hardware which supports MSI-X typically also supports some sort of interrupt moderation, so a read-flush is not necessary for mask/unmask operations.

This patch has been validated with (unreleased) network hardware which uses MSI-X.

How well does this play with the MSI core changes that Michael Ellerman has proposed on the linux-pci mailing list?

Quoted 15 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 enable/disable and rebalancing operations. Because this is an expensive operation, we do not perform the read flush after mask/unmask operations. Hardware which supports MSI-X typically also supports some sort of interrupt moderation, so a read-flush is not necessary for mask/unmask operations.

Quoted 2 lines

This patch has been validated with (unreleased) network hardware which uses MSI-X.

How well does this play with the MSI core changes that Michael Ellerman has proposed on the linux-pci mailing list?

I guess I should add that I'm not certain that the code is exactly correct there are weird differences between enable/disable and mask.

Where generally
the mask/unmask methods do the work and enable/disable do some weird software thing. Having them different and enable/disable not doing some software thing concerns me a little. I think mask/unmask may been overoptimized in this case.

So I expect someone will wind up refactor this code at some point.

However the code is clearly better than what we have now, and it can't affect anything else.

Eric

Quoted 2 lines · Eric W. Biederman

I guess I should add that I'm not certain that the code is exactly correct there are weird differences between enable/disable and mask.

My understanding was "enable" would clear (or ignore) pending interrupts and "unmask" would deliver pending interrupts. Disable and mask could in many implementations be the same thing as long as the enable/unmask difference was supported.

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 enable/disable and rebalancing operations.

diff -urpN -X dontdiff linux-2.6.21-rc4-clean/drivers/pci/msi.c linux-2.6.21-rc4/drivers/pci/msi.c

drivers/pci/msi.c +12 -0

@@ -68,6 +68,29 @@ static void msix_set_enable(struct pci_d
 	}
 }
 
+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:
+	{

Why wouldn't MSI have the same problems as MSI-X?

Quoted 2 lines · Grant Grundler

Why wouldn't MSI have the same problems as MSI-X?

Because enabling and disabling the MSI interrupt is done through config space, and config space writes are not posted. So we won't see the problem that we do with MSI-X.

Quoted 4 lines

Because enabling and disabling the MSI interrupt is done through config space, and config space writes are not posted. So we won't see the problem that we do with MSI-X.

Normally this is true. However when we have memory mapped pci config space the writes could very easily be posted. Even if there aren't any Intel platforms that do it today other architectures might.

From what I read in the spec, configuration writes are not ever posted.

The more I think about this the more I think we should make mask and unmask do the read and set enable/disable to mask/unmask in the msi case.

I think being a little more paranoid will result in slightly simpler more maintainable code, and not measurably affect performance. I don't expect you are migrating irqs in a fast path.

Quoted 5 lines · Eric W. Biederman

Quoted 4 lines

Because enabling and disabling the MSI interrupt is done through config space, and config space writes are not posted. So we won't see the problem that we do with MSI-X.

Quoted 5 lines

Normally this is true. However when we have memory mapped pci config space the writes could very easily be posted. Even if there aren't any Intel platforms that do it today other architectures might.

From what I read in the spec, configuration writes are not ever posted.

They are a completely separate transaction type from memory writes, and only memory writes are posted. So there's no need to read-flush these.

Quoted 7 lines

The more I think about this the more I think we should make mask and unmask do the read and set enable/disable to mask/unmask in the msi case.

I think being a little more paranoid will result in slightly simpler more maintainable code, and not measurably affect performance. I don't expect you are migrating irqs in a fast path.

Migrating IRQs happens in the fast path, but infrequently (every 10 seconds or so).

However the mask function is called at EVERY interrupt, so this change would be VERY expensive.

If the driver really needs to disable the interrupt, then it can call irq_disable().

Otherwise, mask (as-is) should be fine -- it masks
the interrupt at the APIC, and the device's interrupt moderation should take care of keeping it from generating interrupts right away.

Quoted 2 lines

However the mask function is called at EVERY interrupt, so this change would be VERY expensive.

If true I think that would be bad. However I don't see it. Where in handle_edge_irq do we mask the interrupt? The only place I see us calling ->mask is from move_native_irq and that only if IRQ_MOVE_PENDING is true.

All I can see is us routinely calling is ack_APIC_edge.

Doh! I was reading the code wrong. We only mask if we're still handling a previous interrupt on the same vector. My bad.

However, I can't really see where mask() is used outside of that instance. Which then leads us back to the question: do we need a read flush on mask/unmask or just enable/disable?

The practical question in my book is do we set the enable/disable methods to the same functions as the mask/unmask methods or do we let them default to the crazy delayed disable scenario.

Given that we do have a tiny race where we need to ensure the MSI is disabled before we unregister it, we don't know of any MSI implementation problems that will result in a screaming IRQ. I would say set enable/disable to the mask/unmask methods.