[Patch] speed up interrupt handling

[Patch] speed up interrupt handling

[Peter Chubb]

Hi David,
   The attached patch makes enable_irq() disappear from the profile.
From ~4000 cycles per call, it's gone down to around 260 cycles.
I've only tested on UP; however, the iosapic lock is held around all
the manipulations of the new field.

This also incidentally increases the performance of the gigabit
network driver: it drops fewer packets.

Patch against 2.6.3.

=== arch/ia64/kernel/iosapic.c 1.34 vs edited ==--- 1.34/arch/ia64/kernel/iosapic.c	Wed Jan 28 16:19:56 2004
+++ edited/arch/ia64/kernel/iosapic.c	Fri Feb 27 11:39:48 2004
@@ -103,6 +103,7 @@
 
 static struct iosapic_intr_info {
 	char		*addr;		/* base address of IOSAPIC */
+	u32		low32;		/* current value of low word of Redirection table entry */
 	unsigned int	gsi_base;	/* first GSI assigned to this IOSAPIC */
 	char		rte_index;	/* IOSAPIC RTE index (-1 => not an IOSAPIC interrupt) */
 	unsigned char	dmode	: 3;	/* delivery mode (see iosapic.h) */
@@ -213,6 +214,7 @@
 	writel(high32, addr + IOSAPIC_WINDOW);
 	writel(IOSAPIC_RTE_LOW(rte_index), addr + IOSAPIC_REG_SELECT);
 	writel(low32, addr + IOSAPIC_WINDOW);
+	iosapic_intr_info[vector].low32 = low32;
 }
 
 static void
@@ -239,9 +241,10 @@
 	spin_lock_irqsave(&iosapic_lock, flags);
 	{
 		writel(IOSAPIC_RTE_LOW(rte_index), addr + IOSAPIC_REG_SELECT);
-		low32 = readl(addr + IOSAPIC_WINDOW);
 
-		low32 |= (1 << IOSAPIC_MASK_SHIFT);    /* set only the mask bit */
+		/* set only the mask bit */
+		low32 = iosapic_intr_info[vec].low32 |= IOSAPIC_MASK;
+
 		writel(low32, addr + IOSAPIC_WINDOW);
 	}
 	spin_unlock_irqrestore(&iosapic_lock, flags);
@@ -264,9 +267,7 @@
 	spin_lock_irqsave(&iosapic_lock, flags);
 	{
 		writel(IOSAPIC_RTE_LOW(rte_index), addr + IOSAPIC_REG_SELECT);
-		low32 = readl(addr + IOSAPIC_WINDOW);
-
-		low32 &= ~(1 << IOSAPIC_MASK_SHIFT);    /* clear only the mask bit */
+		low32 = iosapic_intr_info[vec].low32 &= ~IOSAPIC_MASK;
 		writel(low32, addr + IOSAPIC_WINDOW);
 	}
 	spin_unlock_irqrestore(&iosapic_lock, flags);
@@ -307,9 +308,7 @@
 	{
 		/* get current delivery mode by reading the low32 */
 		writel(IOSAPIC_RTE_LOW(rte_index), addr + IOSAPIC_REG_SELECT);
-		low32 = readl(addr + IOSAPIC_WINDOW);
-
-		low32 &= ~(7 << IOSAPIC_DELIVERY_SHIFT);
+		low32 = iosapic_intr_info[vec].low32 & ~(7 << IOSAPIC_DELIVERY_SHIFT);
 		if (redir)
 		        /* change delivery mode to lowest priority */
 			low32 |= (IOSAPIC_LOWEST_PRIORITY << IOSAPIC_DELIVERY_SHIFT);
@@ -317,6 +316,7 @@
 		        /* change delivery mode to fixed */
 			low32 |= (IOSAPIC_FIXED << IOSAPIC_DELIVERY_SHIFT);
 
+		iosapic_intr_info[vec].low32 = low32;
 		writel(IOSAPIC_RTE_HIGH(rte_index), addr + IOSAPIC_REG_SELECT);
 		writel(high32, addr + IOSAPIC_WINDOW);
 		writel(IOSAPIC_RTE_LOW(rte_index), addr + IOSAPIC_REG_SELECT);
 
=== include/asm-ia64/iosapic.h 1.12 vs edited ==--- 1.12/include/asm-ia64/iosapic.h	Tue Jan 13 12:30:32 2004
+++ edited/include/asm-ia64/iosapic.h	Fri Feb 27 11:39:17 2004
@@ -45,9 +45,9 @@
 /*
  * Mask bit
  */
+
 #define	IOSAPIC_MASK_SHIFT		16
-#define	IOSAPIC_UNMASK			0
-#define	IOSAPIC_MSAK			1
+#define	IOSAPIC_MASK			(1<<IOSAPIC_MASK_SHIFT)
 
 #ifndef __ASSEMBLY__
 

Re: [Patch] speed up interrupt handling

[Grant Grundler]

On Fri, Feb 27, 2004 at 01:18:56PM +1100, Peter Chubb wrote:
> 
> Hi David,
>    The attached patch makes enable_irq() disappear from the profile.
> From ~4000 cycles per call, it's gone down to around 260 cycles.

wow! cool!

> I've only tested on UP; however, the iosapic lock is held around all
> the manipulations of the new field.

I think that's a great tradeoff.

> This also incidentally increases the performance of the gigabit
> network driver: it drops fewer packets.

Is this related to NAPI?

I'll assume "the gigabit" card is e1000:
static void e1000_netpoll (struct net_device *dev)
{
	struct e1000_adapter *adapter = dev->priv;
	disable_irq(adapter->pdev->irq);
	e1000_intr (adapter->pdev->irq, dev, NULL);
	enable_irq(adapter->pdev->irq);
}

Ok - this will work great on ia64 with this patch.
pktgen should measurably perform better too.
I'll have to try that.

e1000_intr() will need to handle being called twice for
the same device in case it gets invoked on something
other than the same CPU which normally handles it's IRQ.

Not sure how the net stack schedules the poll call but
it's probably worth trying to make sure only one
CPU touches instance data for a NIC card.
Anyone know how to enforce or verify that?
(or want to pursue the question on linux-net mailing list)

Any comments on how efficient disable_irq/enable_irq()
are on other architectures?
I'm just wondering if e1000_netpoll should be the reccomended method
to support NAPI.

thanks,
grant

RE: [Patch] speed up interrupt handling

[Feldman, Scott]

> Is this related to NAPI?
> 
> I'll assume "the gigabit" card is e1000:
> static void e1000_netpoll (struct net_device *dev) {
> 	struct e1000_adapter *adapter = dev->priv;
> 	disable_irq(adapter->pdev->irq);
> 	e1000_intr (adapter->pdev->irq, dev, NULL);
> 	enable_irq(adapter->pdev->irq);
> }

e1000_netpoll() is for netdump (netconsole) support, not NAPI.

NAPI uses dev->poll = e1000_clean.

Unfortunate similar naming for unrelated things. :-(

-scott

RE: [Patch] speed up interrupt handling

[Peter Chubb]

>>>>> "Scott" = Scott Feldman <Feldman> writes:

>> Is this related to NAPI?
>> 
>> I'll assume "the gigabit" card is e1000: 

Nope, DP83820.  I'm benchmarking a driver that runs in userspace
against the in-kernel driver; both benefit from the interrupt speedup.

--
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
The technical we do immediately,  the political takes *forever*

RE: [Patch] speed up interrupt handling

[David Mosberger]

>>>>> On Fri, 27 Feb 2004 18:44:38 +1100, Peter Chubb <peter@chubb.wattle.id.au> said:

>>>>> "Scott" = Scott Feldman <Feldman> writes:

  >>> Is this related to NAPI?

  >>> I'll assume "the gigabit" card is e1000:

  Peter> Nope, DP83820.  I'm benchmarking a driver that runs in
  Peter> userspace against the in-kernel driver; both benefit from the
  Peter> interrupt speedup.

So why is irq enabling/disabling on the critical path for this driver?

	--david

RE: [Patch] speed up interrupt handling

[Peter Chubb]

>>>>> "David" = David Mosberger <davidm@napali.hpl.hp.com> writes:

>>>>> On Fri, 27 Feb 2004 18:44:38 +1100, Peter Chubb <peter@chubb.wattle.id.au> said:
>>>>> "Scott" = Scott Feldman <Feldman> writes:

>>>> Is this related to NAPI?

>>>> I'll assume "the gigabit" card is e1000:

Peter> Nope, DP83820.  I'm benchmarking a driver that runs in
Peter> userspace against the in-kernel driver; both benefit from the
Peter> interrupt speedup.

David> So why is irq enabling/disabling on the critical path for this
David> driver?

Because IRQs are disabled for every hardware interrupt, and enabled after each
interrupt returns.  So the latency between hardware interrupt and the
driver seeing the interrupt *was* (prepatch) around 5 microseconds;
minimum latency between interrupts was around 10 microseconds.
This is time on a busy system to run out of receive buffers (you can
get a new packet every 640ns with minimum-sized packets, and it takes
time to process each packet)

With the patch minimum time between interrupts should drop to around 2us,
but I need to measure it again.

--
Dr Peter Chubb  http://www.gelato.unsw.edu.au  peterc AT gelato.unsw.edu.au
The technical we do immediately,  the political takes *forever*

RE: [Patch] speed up interrupt handling

[David Mosberger]

>>>>> On Sat, 28 Feb 2004 08:27:40 +1100, Peter Chubb <peter@chubb.wattle.id.au> said:

>>>>> "David" = David Mosberger <davidm@napali.hpl.hp.com> writes:

  David> So why is irq enabling/disabling on the critical path for
  David> this driver?

  Peter> Because IRQs are disabled for every hardware interrupt, and
  Peter> enabled after each interrupt returns.

OK, it's been really long since I worked on this, but AFAIR, for
level-triggered interrupts, there should be no masking/unmasking
during interrupt delivery.  What am I missing?

	--david

RE: [Patch] speed up interrupt handling

[Peter Chubb]

>>>>> "David" = David Mosberger <davidm@napali.hpl.hp.com> writes:

>>>>> On Sat, 28 Feb 2004 08:27:40 +1100, Peter Chubb <peter@chubb.wattle.id.au> said:
>>>>> "David" = David Mosberger <davidm@napali.hpl.hp.com> writes:

Peter> Because IRQs are disabled for every hardware interrupt, and
Peter> enabled after each interrupt returns.

David> OK, it's been really long since I worked on this, but AFAIR,
David> for level-triggered interrupts, there should be no
David> masking/unmasking during interrupt delivery.  What am I
David> missing?

Sorry, I should have been more clear.  For the user-mode driver, we
need to disable the interrupt so that it can be acknowledged to the
device in the user-level code after the in-kernel generic  interrupt
handler has returned.

So the sequence goes:
  Kernel					Userspace handler
						read(irqfd)
						  enable_irq()
						  down(semaphore)	

   do_IRQ
      desc->handler->ack()
      desc->action() (= stub_handler())		
	disable_irq				
	up(semaphore)
	return IRQ_HANDLED
      desc->handler->end()
						return from read()
						Read device status
							register
						Handle interrupt

I'm sure I also saw enable_irq() in the profile for the normal
in-kernel driver, but I don't know where it comes from, and can't see
it now.

Peter C

RE: [Patch] speed up interrupt handling

[David Mosberger]

>>>>> On Sat, 28 Feb 2004 15:22:41 +1100, Peter Chubb <peter@chubb.wattle.id.au> said:

  Peter> Sorry, I should have been more clear.  For the user-mode driver, we
  Peter> need to disable the interrupt so that it can be acknowledged to the
  Peter> device in the user-level code after the in-kernel generic  interrupt
  Peter> handler has returned.

Ah, now it makes sense.  OK, I don't think it will have much of a
practical effect on the normal kernel, but the patch is obvious and
simple enough that I don't see a reason not to do it.

	--david

RE: [Patch] speed up interrupt handling

[Mallick, Asit K]

The patch is fine but I will be concerned about using disable/enable
(masking and unmasking at IOAPIC) especially for edge triggered
interrupts. There is a possibility of loosing interrupts. The user
handler handles the device interrupt with this interrupt masked. A new
interrupt could be generated by the device while the interrupt is still
masked. This new interrupt if edge triggered will be lost since IOAPIC
does not keep pending interrupts. Level triggered interrupts should be
ok.
Thanks,
Asit


>-----Original Message-----
>From: linux-ia64-owner@vger.kernel.org 
>[mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of David Mosberger
>Sent: Friday, February 27, 2004 9:48 PM
>To: Peter Chubb
>Cc: davidm@hpl.hp.com; Feldman, Scott; Grant Grundler; Peter 
>Chubb; linux-ia64@vger.kernel.org; gelato@gelato.unsw.edu.au
>Subject: RE: [Patch] speed up interrupt handling
>
>>>>>> On Sat, 28 Feb 2004 15:22:41 +1100, Peter Chubb 
><peter@chubb.wattle.id.au> said:
>
>  Peter> Sorry, I should have been more clear.  For the 
>user-mode driver, we
>  Peter> need to disable the interrupt so that it can be 
>acknowledged to the
>  Peter> device in the user-level code after the in-kernel 
>generic  interrupt
>  Peter> handler has returned.
>
>Ah, now it makes sense.  OK, I don't think it will have much of a
>practical effect on the normal kernel, but the patch is obvious and
>simple enough that I don't see a reason not to do it.
>
>	--david
>-
>To unsubscribe from this list: send the line "unsubscribe 
>linux-ia64" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

RE: [Patch] speed up interrupt handling

[Peter Chubb]

>>>>> "Asit" = Asit K Mallick <Mallick> writes:

Asit> The patch is fine but I will be concerned about using
Asit> disable/enable (masking and unmasking at IOAPIC) especially for
Asit> edge triggered interrupts. There is a possibility of loosing
Asit> interrupts. The user handler handles the device interrupt with
Asit> this interrupt masked. A new interrupt could be generated by the
Asit> device while the interrupt is still masked. This new interrupt
Asit> if edge triggered will be lost since IOAPIC does not keep
Asit> pending interrupts. Level triggered interrupts should be ok.
Asit> Thanks, Asit

The in-kernel handling for edge-triggered interrupts already has this
problem, as far as I can tell.  However, for the more modern IA64
platforms as far as I know there *are* no edge-triggered interrupts,
so it shouldn't be a problem.

PeterC

RE: [Patch] speed up interrupt handling

[David Mosberger]

>>>>> On Sat, 28 Feb 2004 21:21:00 +1100, Peter Chubb <peter@chubb.wattle.id.au> said:

  Peter> The in-kernel handling for edge-triggered interrupts already
  Peter> has this problem, as far as I can tell.  However, for the
  Peter> more modern IA64 platforms as far as I know there *are* no
  Peter> edge-triggered interrupts, so it shouldn't be a problem.

On a zx2000, I see:

$ cat /proc/interrupts |fgrep -i edge
 49:      14555   IO-SAPIC-edge  serial

I doubt you care about user-level serial-line drivers, though. ;-)

	--david