[PATCH] Improvev netconsole support for RTL8139 NIC driver

[PATCH] Improvev netconsole support for RTL8139 NIC driver

[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 <yang.shi@windriver.com>
---
 b/drivers/net/8139too.c |    9 +++++++++
 1 file changed, 9 insertions(+)
---

--- a/drivers/net/8139too.c
+++ b/drivers/net/8139too.c
@@ -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? */
@@ -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

        DPRINTK ("%s: exiting interrupt, intr_status=%#4.4x.\n",
                 dev->name, RTL_R16 (IntrStatus));

Re: [PATCH] Improvev netconsole support for RTL8139 NIC driver

[Jeff Garzik]

yshi wrote:
> 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 <yang.shi@windriver.com>
> ---
> b/drivers/net/8139too.c |    9 +++++++++
> 1 file changed, 9 insertions(+)
> ---
> 
> --- a/drivers/net/8139too.c
> +++ b/drivers/net/8139too.c
> @@ -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? */
> @@ -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

Re: [PATCH] Improvev netconsole support for RTL8139 NIC driver

[yshi]

Hi Jeff,

Thanks for the comment. According your suggestion, I added 
local_irq_save()/local_irq_restore() in rtl8139_netpoll_controller().
Move the burden to netpoll layer. The following is the modified patch:

Replaced disable_irq()/enable_irq() with
local_irq_save()/local_irq_restore() to improve
netconsole/netpoll support on RTL8139 NIC driver.

Signed-off-by: Yang Shi <yang.shi@windriver.com>
---
 b/drivers/net/8139too.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
---

--- a/drivers/net/8139too.c
+++ b/drivers/net/8139too.c
@@ -2199,9 +2199,11 @@ static irqreturn_t rtl8139_interrupt (in
  */
 static void rtl8139_poll_controller(struct net_device *dev)
 {
-       disable_irq(dev->irq);
+       unsigned long flags;
+
+       local_irq_save(flags);
        rtl8139_interrupt(dev->irq, dev);
-       enable_irq(dev->irq);
+       local_irq_restore(flags);
 }
 #endif

Thanks.

Yang
Jeff Garzik 写道:
> yshi wrote:
>> 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 <yang.shi@windriver.com>
>> ---
>> b/drivers/net/8139too.c |    9 +++++++++
>> 1 file changed, 9 insertions(+)
>> ---
>>
>> --- a/drivers/net/8139too.c
>> +++ b/drivers/net/8139too.c
>> @@ -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? */
>> @@ -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
>
>
>
>

Re: [PATCH] Improvev netconsole support for RTL8139 NIC driver

[David Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Date: Tue, 25 Mar 2008 22:23:24 -0400

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

Re: [PATCH] Improvev netconsole support for RTL8139 NIC driver

[yshi]

David Miller 写道:
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Tue, 25 Mar 2008 22:23:24 -0400
>
>   
>> 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.
>   
Yes, I agree. Many NIC drivers do the same thing, like Gianfar, E1000, etc.

Re: [PATCH] Improvev netconsole support for RTL8139 NIC driver

[Jeff Garzik]

yshi wrote:
> David Miller 写道:
>> From: Jeff Garzik <jgarzik@pobox.com>
>> Date: Tue, 25 Mar 2008 22:23:24 -0400
>>
>>  
>>> 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.
>>   
> Yes, I agree. Many NIC drivers do the same thing, like Gianfar, E1000, etc.

No, I just looked.  Neither gianfar nor e1000 nor e1000e does this.

In fact, gfar_transmit() is precisely an example of what I'm talking 
about:  you only need to use spin_lock() there.

	Jeff

Re: [PATCH] Improvev netconsole support for RTL8139 NIC driver

[Jeff Garzik]

David Miller wrote:
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Tue, 25 Mar 2008 22:23:24 -0400
> 
>> 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.


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

The entire spin_lock() apparatus in the interrupt handler disappears 
nicely on uniprocessor machines.

Plus, you are not competing with any other interrupts other than your 
own, which is the only major class of problems where spin_lock_irqsave() 
in interrupt handler is really needed (PS/2 kbd + mouse is an example).

Or more simply, it's not needed, so nothing is gained by doing 
additional work in the hot path for the sake of consistency.

	Jeff

Re: [PATCH] Improvev netconsole support for RTL8139 NIC driver

[David Miller]

From: Jeff Garzik <jgarzik@pobox.com>
Date: Tue, 25 Mar 2008 23:14:03 -0400

> David Miller wrote:
> > From: Jeff Garzik <jgarzik@pobox.com>
> > Date: Tue, 25 Mar 2008 22:23:24 -0400
> > 
> >> 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.

Re: [PATCH] Improvev netconsole support for RTL8139 NIC driver

[Jeff Garzik]

David Miller wrote:
> From: Jeff Garzik <jgarzik@pobox.com>
> Date: Tue, 25 Mar 2008 23:14:03 -0400
> 
>> David Miller wrote:
>>> From: Jeff Garzik <jgarzik@pobox.com>
>>> Date: Tue, 25 Mar 2008 22:23:24 -0400
>>>
>>>> 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

Thus applying this consistency rule across N drivers needlessly 
increases the code size of N drivers.

Mainly I see such a change as a violation of a basic Linux principle: 
do what you must, and no more.

	Jeff

Re: [PATCH] Improvev netconsole support for RTL8139 NIC driver

[Jeff Garzik]

Jeff Garzik wrote:
> 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.

	Jeff

Re: [PATCH] Improvev netconsole support for RTL8139 NIC driver

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Tue, 25 Mar 2008 23:48:20 -0400

> Jeff Garzik wrote:
> > 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.

Secondly, this isn't a hot path.  The "hot path" is in the
->poll() handler which does all of the real packet RX work.
And that runs lockless, in software interrupt context.

The HW interrupt handler's cost is lower bound by the cost of doing a
PIO on the status register, it's impractical to perform any
micro-optimizations of any sort here.

Especially those that sacrifice consistency.

Re: [PATCH] Improvev netconsole support for RTL8139 NIC driver

[Jeff Garzik]

David Miller wrote:
> 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.

No, I hear you.

I'm not focusing on cycles, but list examples of the negative effects of 
doing needless work for the sake of consistency:

* eliminates ability to compile-out spinlocks on UP
* code size increases (even if miniscule)
* CPU instructions in a hot path increases (even if lost in the noise)
* stack usage increases (even if miniscule)

But those are just examples of the principle:  don't do work you don't 
need to do.

I also think spin_lock -> spin_lock_irqsave amounts to a slight loss of 
information, too:  Use of spin_lock() rather than spin_lock_irqsave() 
potentially gives the -rt folks some additional flexibility, by 
advertising a different set of acceptable irq-disablement states.

Is the effect huge in this specific case?  No.

Does that give us license to add needless code to drivers?  No, again, IMO.

	Jeff