mask/unmasking while servicing MSI(-X) unnecessary?

[Loic Prylli <loic@myri.com>]

While looking into using MSI-X for our myri10ge driver, we have seen
that the msi(x) code (driver/pci/msi.c) masks the MSI(-X) vector while
servicing an interrupt. We are not sure why this masking is needed (for
instance no such thing is done for "edge IOAPIC" interrupts). There
seems to be already several mechanisms each individually protecting
against "loosing an interrupt" without masking:
- the "x86" architecture is able to queue 2 interrupt messages. That
guarantees an interrupt handler will always start after the last MSI
received (even in the case of a big burst of MSI messages).
- Even if there wasn't that interrupt queuing, ack_APIC_irq() could be
moved in the ack() method. Then things would work without masking even
on a hypothetical platform where a new interrupt is completely ignored
(with no IRR-like register) while servicing the same vector (anyway
since this "msi" code is already tied to "x86" architecture
specificities, that hypothetical platform might not be relevant).
- Almost every driver/device have their own way of acknowledging
interrupts anyway, which also by itself makes the masking/unmasking
unnecessary.
- The masking by itself is racy unless the driver interrupt handler
starts by making sure the masking request has reached the device with
some kind of MMIO-read. Such a MMIO-read is normally the kind of costly
requests you are happy to get rid of in the MSI model.

So if it is not useful, it might be better to avoid that systematic
mask/unmask pair. This masking has a small but measurable performance
impact for our device/driver combo.

Would you agree to suppress that masking (sample patch following)? Or
otherwise, is there is a possibility of making it optional on a
per-device basis.

Thanks for any comment!


Loic Prylli
Myricom, Inc.

[PATCH] Don't mask the interrupt vector while servicing a MSI interrupt.


Signed-off-by: Loic Prylli <loic@myri.com>


---

 drivers/pci/msi.c |   19 ++++++-------------
 1 files changed, 6 insertions(+), 13 deletions(-)

98e9a27091c3ccdc8a8a72cea9ab9086fb258af3
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a83c1f5..af446e3 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -200,19 +200,12 @@ static void shutdown_msi_irq(unsigned in
 	spin_unlock_irqrestore(&msi_lock, flags);
 }
 
-static void end_msi_irq_wo_maskbit(unsigned int vector)
+static void end_msi_irq(unsigned int vector)
 {
 	move_native_irq(vector);
 	ack_APIC_irq();
 }
 
-static void end_msi_irq_w_maskbit(unsigned int vector)
-{
-	move_native_irq(vector);
-	unmask_MSI_irq(vector);
-	ack_APIC_irq();
-}
-
 static void do_nothing(unsigned int vector)
 {
 }
@@ -227,8 +220,8 @@ static struct hw_interrupt_type msix_irq
 	.shutdown	= shutdown_msi_irq,
 	.enable		= unmask_MSI_irq,
 	.disable	= mask_MSI_irq,
-	.ack		= mask_MSI_irq,
-	.end		= end_msi_irq_w_maskbit,
+	.ack		= do_nothing,
+	.end		= end_msi_irq,
 	.set_affinity	= set_msi_affinity
 };
 
@@ -243,8 +236,8 @@ static struct hw_interrupt_type msi_irq_
 	.shutdown	= shutdown_msi_irq,
 	.enable		= unmask_MSI_irq,
 	.disable	= mask_MSI_irq,
-	.ack		= mask_MSI_irq,
-	.end		= end_msi_irq_w_maskbit,
+	.ack		= do_nothing,
+	.end		= end_msi_irq,
 	.set_affinity	= set_msi_affinity
 };
 
@@ -260,7 +253,7 @@ static struct hw_interrupt_type msi_irq_
 	.enable		= do_nothing,
 	.disable	= do_nothing,
 	.ack		= do_nothing,
-	.end		= end_msi_irq_wo_maskbit,
+	.end		= end_msi_irq,
 	.set_affinity	= set_msi_affinity
 };
 
-- 
1.3.2




-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/