Improvev netconsole support for RTL8139 NIC driver

In current RTL8139 NIC driver, spin_lock()/spin_unlock() is used for irq handler. But for netconsole/netpoll, it prefers spin_lock_irqsave()/spin_unlcok_irqrestore(). So this patch fixed this problem to improve netconsole/netpoll support.

Signed-off-by Yang Shi <[email protected]>

---
b/drivers/net/8139too.c | 9 +++++++++

1 file changed, 9 insertions(+)

---

drivers/net/8139too.c +5 -0

@@ -2136,8 +2136,13 @@ static irqreturn_t rtl8139_interrupt (in
       u16 status, ackstat;
       int link_changed = 0; /* avoid bogus "uninit" warning */
       int handled = 0;
+#ifdef CONFIG_NET_POLL_CONTROLLER
+       unsigned long flags;
 
+       spin_lock_irqsave (&tp->lock, flags);
+#else
       spin_lock (&tp->lock);
+#endif
       status = RTL_R16 (IntrStatus);
 
       /* shared irq? */

Quoted 28 lines · yshi

In current RTL8139 NIC driver, spin_lock()/spin_unlock() is used for irq handler. But for netconsole/netpoll, it prefers spin_lock_irqsave()/spin_unlcok_irqrestore(). So this patch fixed this problem to improve netconsole/netpoll support.

Signed-off-by Yang Shi <[email protected]>

---
b/drivers/net/8139too.c | 9 +++++++++

1 file changed, 9 insertions(+)

---

drivers/net/8139too.c +5 -0

@@ -2136,8 +2136,13 @@ static irqreturn_t rtl8139_interrupt (in
       u16 status, ackstat;
       int link_changed = 0; /* avoid bogus "uninit" warning */
       int handled = 0;
+#ifdef CONFIG_NET_POLL_CONTROLLER
+       unsigned long flags;
 
+       spin_lock_irqsave (&tp->lock, flags);
+#else
       spin_lock (&tp->lock);
+#endif
       status = RTL_R16 (IntrStatus);
 
       /* shared irq? */

drivers/net/8139too.c static irqreturn_t rtl8139_interrupt (in · line 2197

Quoted 8 lines

drivers/net/8139too.c +4 -0

@@ -2185,7 +2190,11 @@ static irqreturn_t rtl8139_interrupt (in
                       RTL_W16 (IntrStatus, TxErr);
       }
 out:
+#ifdef CONFIG_NET_POLL_CONTROLLER
+       spin_unlock_irqrestore (&tp->lock, flags);
+#else
       spin_unlock (&tp->lock);
+#endif

This is bogus -- you should never need to slow down the hot path in such a way.

Add a local_irq_save()/restore() to rtl8139_poll_controller() or a similar solution, putting the burden on the netpoll/netconsole layer.

Jeff

This is bogus -- you should never need to slow down the hot path in such a way.

From: Jeff Garzik <[email protected]>
Date: Tue, 25 Mar 2008 22:23:24 -0400

Quoted 2 lines

This is bogus -- you should never need to slow down the hot path in such a way.

Slow down in what way? Even on x86 saving the flags is just about as expensive as a plain sti/cli.

I would in fact prefer to see drivers unconditionally use spin_lock_irqsave() et al. in the interrupt handler, for consistency.

Quoted 7 lines · David Miller

From: Jeff Garzik <[email protected]>
Date: Tue, 25 Mar 2008 22:23:24 -0400

Quoted 2 lines

This is bogus -- you should never need to slow down the hot path in such a way.

Quoted 7 lines

Slow down in what way? Even on x86 saving the flags is just about as expensive as a plain sti/cli.

I would in fact prefer to see drivers unconditionally use spin_lock_irqsave() et al. in the interrupt handler, for consistency.

Yes, I agree. Many NIC drivers do the same thing, like Gianfar, E1000, etc.

From: Jeff Garzik <[email protected]>
Date: Tue, 25 Mar 2008 22:23:24 -0400

Quoted 1 line

This is bogus -- you should never need to slow down the hot path in such a way.

Slow down in what way? Even on x86 saving the flags is just about as expensive as a plain sti/cli.

I would in fact prefer to see drivers unconditionally use spin_lock_irqsave() et al. in the interrupt handler, for consistency.

Quoted 6 lines · David Miller

From: Jeff Garzik <[email protected]>
Date: Tue, 25 Mar 2008 22:23:24 -0400

Quoted 2 lines

This is bogus -- you should never need to slow down the hot path in such a way.

Quoted 2 lines

Slow down in what way? Even on x86 saving the flags is just about as expensive as a plain sti/cli.

Replacing spin_lock() [current 8139too.c] with spin_lock_irqsave() results in a larger interrupt handler... more CPU instructions for the same result.

From: Jeff Garzik <[email protected]>
Date: Tue, 25 Mar 2008 23:14:03 -0400

Quoted 8 lines

Quoted 6 lines · David Miller

From: Jeff Garzik <[email protected]>
Date: Tue, 25 Mar 2008 22:23:24 -0400

Quoted 1 line

This is bogus -- you should never need to slow down the hot path in such a way.

Slow down in what way? Even on x86 saving the flags is just about as expensive as a plain sti/cli.

Replacing spin_lock() [current 8139too.c] with spin_lock_irqsave() results in a larger interrupt handler... more CPU instructions for the same result.

Jeff, please be realistic.

These interrupt handlers about to do a PIO on a status register, which will consume on the order of a few hundred cpu cycles.

Counting an I-cache line or two, or 18 cycles here or there, is immaterial by comparison.

I am being realistic... it's

* not needed
* increases code size
* increases number of CPU instructions executed
* not needed

Quoted 6 lines · Jeff Garzik

I am being realistic... it's

* not needed
* increases code size
* increases number of CPU instructions executed
* not needed

I also wonder if using spin_lock_irqsave() makes moving to a real-time kernel with interrupt threads more difficult for that driver.

And of course we're talking about a hot path here.

First, if you mention CPU instructions executed you're totally ignoring what I wrote. Which is that we're about to sit spinning on hundreds of cycles doing a PIO read on a status register.

Those hand full of cycles doing the irqsave/irqrestore don't matter, at all.

Again, you're arguing for 18 cycles or so out of say 800. It's peanuts, at best.