[PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[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.

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

Revised with input from Eric Biederman.

Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>

diff -urpN -X dontdiff linux-2.6.21-rc5-clean/drivers/pci/msi.c linux-2.6.21-rc5/drivers/pci/msi.c
--- linux-2.6.21-rc5-clean/drivers/pci/msi.c	2007-03-28 10:05:24.000000000 -0700
+++ linux-2.6.21-rc5/drivers/pci/msi.c	2007-03-28 09:21:34.000000000 -0700
@@ -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, str
 void 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);

Re: [PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[Eric W. Biederman]

Mitch Williams <mitch.a.williams@intel.com> writes:

> 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.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

>
> Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
>
> diff -urpN -X dontdiff linux-2.6.21-rc5-clean/drivers/pci/msi.c
> linux-2.6.21-rc5/drivers/pci/msi.c
> --- linux-2.6.21-rc5-clean/drivers/pci/msi.c 2007-03-28 10:05:24.000000000 -0700
> +++ linux-2.6.21-rc5/drivers/pci/msi.c	2007-03-28 09:21:34.000000000 -0700
> @@ -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, str
>  void 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);

Re: [PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[Andrew Morton]

On Fri, 30 Mar 2007 13:04:02 -0600
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Mitch Williams <mitch.a.williams@intel.com> writes:
> 
> > 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.
> 
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Did we end up deciding whether this is (needed*safe) enough for 2.6.21?

Re: [PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[Greg KH]

On Fri, Mar 30, 2007 at 12:47:47PM -0700, Andrew Morton wrote:
> On Fri, 30 Mar 2007 13:04:02 -0600
> ebiederm@xmission.com (Eric W. Biederman) wrote:
> 
> > Mitch Williams <mitch.a.williams@intel.com> writes:
> > 
> > > 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.
> > 
> > Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> 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

Re: [PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[Andrew Morton]

On Fri, 30 Mar 2007 12:49:56 -0700
Greg KH <gregkh@suse.de> wrote:

> > > Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > 
> > 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.

Re: [PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[Greg KH]

On Fri, Mar 30, 2007 at 01:00:22PM -0700, Andrew Morton wrote:
> On Fri, 30 Mar 2007 12:49:56 -0700
> Greg KH <gregkh@suse.de> wrote:
> 
> > > > Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > > 
> > > 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.

Yes, that's what I just asked him to do a message ago :)

thanks,

greg k-h

RE: [PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[Williams, Mitch A]

Greg KH wrote: 
> 
>> 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 :)
>
Be happy to -- just give me a good definition of "appropriate
time".

-Mitch

Re: [PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[Greg KH]

On Fri, Mar 30, 2007 at 01:21:03PM -0700, Williams, Mitch A wrote:
> Greg KH wrote: 
> > 
> >> 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 :)
> >
> 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

Re: [PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> On Fri, Mar 30, 2007 at 12:47:47PM -0700, Andrew Morton wrote:
>> 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

RE: [PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[Williams, Mitch A]

Eric W. Biederman wrote:

>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.

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

Re: [PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[Chuck Ebbert]

Williams, Mitch A wrote:
> 
> 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.

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225399

Re: [PATCH 2.6.21-rc5] Flush MSI-X table writes (rev 3)

[Eric W. Biederman]

"Williams, Mitch A" <mitch.a.williams@intel.com> writes:

> 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] msi: Immediately mask and unmask msi-x irqs.

[Eric W. Biederman]

This is a simplified and actually more comprehensive form of a bug
fix from Mitch Williams <mitch.a.williams@intel.com>.

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.

Cc: Mitch Williams <mitch.a.williams@intel.com>
Cc: Greg KH <greg@kroah.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 drivers/pci/msi.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index ad33e01..435c195 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -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:
-- 
1.5.0.4

RE: [PATCH] msi: Immediately mask and unmask msi-x irqs.

[Williams, Mitch A]

Acked-by: Mitch Williams <mitch.a.williams@intel.com>

>This is a simplified and actually more comprehensive form of a bug
>fix from Mitch Williams <mitch.a.williams@intel.com>.
[snip]
>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 <mitch.a.williams@intel.com>
>Cc: Greg KH <greg@kroah.com>
>Cc: Andrew Morton <akpm@linux-foundation.org>
>Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>---
> drivers/pci/msi.c |    1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
>diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>index ad33e01..435c195 100644
>--- a/drivers/pci/msi.c
>+++ b/drivers/pci/msi.c
>@@ -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:
>-- 
>1.5.0.4
>

Re: [PATCH] msi: Immediately mask and unmask msi-x irqs.

[Siddha, Suresh B]

On Tue, Apr 03, 2007 at 01:41:49AM -0600, Eric W. Biederman wrote:
> 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.

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.

> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index ad33e01..435c195 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -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.

[Eric W. Biederman]

"Siddha, Suresh B" <suresh.b.siddha@intel.com> writes:

> 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.

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.

> 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.

>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index ad33e01..435c195 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
>> @@ -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.

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.

[Siddha, Suresh B]

On Tue, Apr 03, 2007 at 01:39:43PM -0600, Eric W. Biederman wrote:
> "Siddha, Suresh B" <suresh.b.siddha@intel.com> writes:
> 
> 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.

Ok. Then this patch is a must.

> > Don't we need the flush for the PCI_CAP_ID_MSI case 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.