Re: [PATCH] pcnet32 driver NAPI support

Re: [PATCH] pcnet32 driver NAPI support

[Don Fry]

On Wed, Jun 07, 2006 at 12:52:25PM -0400, Lennart Sorensen wrote:
> I have added NAPI support to the pcnet32 driver.  This has greatly
> improved the responsiveness on my systems (geode GX1 266MHz) when under
> heavy network load.  Without this change the system would become
> unresponsive due to interrupts when flooded with traffic, and eventually
> the watchdog would reboot the system due to the watchdog daemon being
> starved for cpu time.  With the patch the system is still useable on a
> serial console, although very slow.  Network throughput is also higher
> since more time is spend processing packets and getting them sent out,
> instead of only spending time acknowledging interrupts from incoming
> packets.
> 
> Now having never actually done a patch submission to the kernel before,
> I will try and see if I can do it right.
> 
> The patch adds a PCNET32_NAPI config option to drivers/net/Kconfig, and
> the appropriate code to support the option to drivers/net/pcnet32.c and
> has been tested on many of my systems (allthough they are allmost all
> identical, and require some extra patches to pcnet32 due to not having
> an EEPROM installed), and on an AT-2700TX.
> 
> I have made a diff against 2.6.16.20 and 2.6.17-rc6.
> 
> Comments would be very welcome.

I am also working on a NAPI version of the pcnet32 driver for many of
the same reasons, and will compare what you have with my own
implementation.  I probably won't be able to do much until Friday.  

Just a couple of comments.  I am adding netdev@vger.kernel.org to the cc
list, as most network driver discussion is done here rather than lkml.
linux-kernel (and linux-net) should be deleted in future replies.

The 2.6.17-rc6 would be the correct source to patch against.  Since this
is an enhancement it will not come out till 2.6.18.

I would not change the driver name from pcnet32 to pcnet32napi, but I
would changes the version from 1.32 to 1.33NAPI or something like that.

Some areas of concern that you may have addressed already, I have not
scanned your changes yet, are what happens if the ring size is changed
without bringing down the interface (via ethtool), or if the loopback
test is run in a similar fashion, or a tx timeout occurs.

The lp->lock MUST be held whenever accessing the csr or bcr registers as
this is a multi-step process, and has been the source of problems in the
past.  Even on UP systems.

> 
> Signed-off-by: Len Sorensen <lsorense@csclub.uwaterloo.ca>
> 
> Len Sorensen

-- 
Don Fry
brazilnut@us.ibm.com

Re: [PATCH] pcnet32 driver NAPI support

[Lennart Sorensen]

On Wed, Jun 07, 2006 at 11:20:40AM -0700, Don Fry wrote:
> I am also working on a NAPI version of the pcnet32 driver for many of
> the same reasons, and will compare what you have with my own
> implementation.  I probably won't be able to do much until Friday.  
> 
> Just a couple of comments.  I am adding netdev@vger.kernel.org to the cc
> list, as most network driver discussion is done here rather than lkml.
> linux-kernel (and linux-net) should be deleted in future replies.

I must have picked the wrong place to cc.

> The 2.6.17-rc6 would be the correct source to patch against.  Since this
> is an enhancement it will not come out till 2.6.18.

I thought so.  That is why I did it against both 2.6.17-rc6 and 2.6.16
(since I use it with 2.6.16).

> I would not change the driver name from pcnet32 to pcnet32napi, but I
> would changes the version from 1.32 to 1.33NAPI or something like that.

Hmm, perhaps.  I just wanted something that made it obvious in dmesg
which driver I was running.  I see tulip actually does put it in the
version instead.  I don't remember where I got the driver name change
idea from.

> Some areas of concern that you may have addressed already, I have not
> scanned your changes yet, are what happens if the ring size is changed
> without bringing down the interface (via ethtool), or if the loopback
> test is run in a similar fashion, or a tx timeout occurs.

The same thing as if it was done before enabling napi.  From a few
messages I have seen, it doesn't work right now, and it won't work any
better with my changes.  I have never tried changing the ring size on
the fly, so I don't know.

It appears that the port is stopped before the ring size change is done,
although I can't really tell how it handles things if the queue is not
empty when it stops the port.  Does it try to handle anything left in
the ring first or does it just toss those packets? (That I would
consider wrong).

> The lp->lock MUST be held whenever accessing the csr or bcr registers as
> this is a multi-step process, and has been the source of problems in the
> past.  Even on UP systems.

Hmm, I just followed what appeared to be in pcnet32_rx and how tulip and
a few other drivers had done their napi conversions.  It certainly works
for me the way I did it.  Haven't seen any lockups yet.  I do see that I
am not holding the lock when I acknowledge IRQs in pcnet32_poll, which
pcnet32_rx doesn't need to worry about since it is called from the
interrupt handler which already holds the lock.  That should be fixed
then.

So I can do:
        // Clear RX interrupts
        spin_lock(&lp->lock);
        lp->a.write_csr (ioaddr, 0, 0x1400);
        spin_unlock(&lp->lock);
That part seems simple enough to protect.

Is this safe without holding the lock?
    } while(lp->a.read_csr (ioaddr, 0) & 0x1400);
Not sure how to wrap a lock around that one without holding the lock for
way too long.

perhaps:
        spin_lock(&lp->lock);
	state=lp->a.read_csr (ioaddr, 0) & 0x1400;
        spin_unlock(&lp->lock);
    } while(state);
Does that seem more reasonable?

Len Sorensen

Re: [PATCH] pcnet32 driver NAPI support

[Don Fry]

On Wed, Jun 07, 2006 at 03:34:56PM -0400, Lennart Sorensen wrote:
> On Wed, Jun 07, 2006 at 11:20:40AM -0700, Don Fry wrote:
> 
> > Some areas of concern that you may have addressed already, I have not
> > scanned your changes yet, are what happens if the ring size is changed
> > without bringing down the interface (via ethtool), or if the loopback
> > test is run in a similar fashion, or a tx timeout occurs.
> 
> The same thing as if it was done before enabling napi.  From a few
> messages I have seen, it doesn't work right now, and it won't work any
> better with my changes.  I have never tried changing the ring size on
> the fly, so I don't know.

Today without your NAPI changes the device can be up and operational,
and change the ring size without hanging or causing a panic or causing
problems with the driver.  The same can be said for running the loopback
test, or when a tx timeout occurs.

It does not look like the same can be said for your NAPI changes, yet.
Try changing the ring size a dozen times during a receive storm and see
what happens.

> 
> It appears that the port is stopped before the ring size change is done,
> although I can't really tell how it handles things if the queue is not
> empty when it stops the port.  Does it try to handle anything left in
> the ring first or does it just toss those packets? (That I would
> consider wrong).
> 
> > The lp->lock MUST be held whenever accessing the csr or bcr registers as
> > this is a multi-step process, and has been the source of problems in the
> > past.  Even on UP systems.
> 
> Hmm, I just followed what appeared to be in pcnet32_rx and how tulip and
> a few other drivers had done their napi conversions.  It certainly works
> for me the way I did it.  Haven't seen any lockups yet.  I do see that I
> am not holding the lock when I acknowledge IRQs in pcnet32_poll, which
> pcnet32_rx doesn't need to worry about since it is called from the
> interrupt handler which already holds the lock.  That should be fixed
> then.

Yes, that is the minimum that needs to be changed.
There have been a lot of changes made to the driver, by myself included,
that seem to work just fine, but later the timing changes and you lose
the race, resulting in problems.  Changing one part of the driver
without understanding the whole thing is very risky.

> 
> So I can do:
>         // Clear RX interrupts
>         spin_lock(&lp->lock);
>         lp->a.write_csr (ioaddr, 0, 0x1400);
>         spin_unlock(&lp->lock);
> That part seems simple enough to protect.
> 
> Is this safe without holding the lock?
>     } while(lp->a.read_csr (ioaddr, 0) & 0x1400);

No, see below.

> Not sure how to wrap a lock around that one without holding the lock for
> way too long.
> 
> perhaps:
>         spin_lock(&lp->lock);
> 	state=lp->a.read_csr (ioaddr, 0) & 0x1400;
>         spin_unlock(&lp->lock);
>     } while(state);
> Does that seem more reasonable?

The lock must be held during ANY read or write to a csr or bcr.

The problem is that accessing a csr/bcr takes two steps.  One is to
write the address register indicating which register to read or write,
and second, read or write the register.  The problem is that without the
lock, entity A writes to the rap to access register X, then before it
can access that register entity B writes the rap to access register Y,
At that point entity A will read, or worse write, the contents of the
wrong register.

This problem is even worse when reading/writing a PHY register because
it entails writing bcr 33 and reading/writing bcr 34, without being
interrupted by something that might change bcr 33.  Not likely, but not
something that can be ignored.  It may not happen often, but it will
happen eventually.

> 
> Len Sorensen

-- 
Don Fry
brazilnut@us.ibm.com

Re: [PATCH] pcnet32 driver NAPI support

[Don Fry]

One other problem I ran into.  I applied the patch but it will not
compile because rl_active is never defined.  I have worked around it but
...


-- 
Don Fry
brazilnut@us.ibm.com

Re: [PATCH] pcnet32 driver NAPI support

[Lennart Sorensen]

On Wed, Jun 07, 2006 at 03:32:45PM -0700, Don Fry wrote:
> One other problem I ran into.  I applied the patch but it will not
> compile because rl_active is never defined.  I have worked around it but

Doh!  I thought I cleaned up all my "weird code" from my own version.
Because of the platform I work with having 4 pcnet32 ports, and a slow
poke 266MHz geode, we can't handle full traffic load, so to keep the
system responsive to pause processing receives when we pass a certain
number of packets per second.  rl_active is part of that.  I meant to
remove all of it, apparently I didn't read every line of my patch
carefully enough. :(

Well at least this ought to clean up my work a bit.

Len Sorensen