[PATCH] Improvev netconsole support for RTL8139 NIC driver - Kernel

This is a discussion on [PATCH] Improvev netconsole support for RTL8139 NIC driver - Kernel ; 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 --- b/drivers/net/8139too.c | 9 +++++++++ 1 file changed, 9 insertions(+) ...

+ Reply to Thread
Results 1 to 12 of 12

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

  1. [PATCH] 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
    ---
    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));

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

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

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



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

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

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


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

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

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

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

    David Miller 写道:
    > From: Jeff Garzik
    > 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.

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

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

    David Miller wrote:
    > From: Jeff Garzik
    > 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


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

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

    yshi wrote:
    > David Miller 写道:
    >> From: Jeff Garzik
    >> 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



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

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

    From: Jeff Garzik
    Date: Tue, 25 Mar 2008 23:14:03 -0400

    > David Miller wrote:
    > > From: Jeff Garzik
    > > 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.
    --
    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/

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

    David Miller wrote:
    > From: Jeff Garzik
    > Date: Tue, 25 Mar 2008 23:14:03 -0400
    >
    >> David Miller wrote:
    >>> From: Jeff Garzik
    >>> 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



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

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

    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


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

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

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

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

    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


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

+ Reply to Thread