RE: e100 "Ferguson" release

RE: e100 "Ferguson" release

[Feldman, Scott]

New one:

	http://sf.net/projects/e1000, e100-3.0.0_dev12.tar.gz

> Comments:

Thanks Jeff!
 
> * (API) Does the out-of-tx-resources condition in 
> e100_xmit_frame ever really happen?  I am under the
> impression that returning non-zero in ->hard_start_xmit
> results in the packet sometimes being requeued and
> sometimes dropped.  I prefer to guarantee a more-steady 
> state, by simply dropping the packet unconditionally,
> when this uncommon condition occurs.  So, I would
> a) mark the failure condition with unlikely(), and
> b) if the condition occurs, simply drop the packet 
> (tx_dropped++, kfree 
> skb), and return zero.

Stop the queue also?

if(unlikely(e100_exec_cb(nic, skb, e100_xmit_prepare) == -ENOMEM)) {
        netif_stop_queue(netdev);
        nic->net_stats.tx_dropped++;
        dev_kfree_skb(skb);
        return 0;
}

Added some more likely/unlikely's in the perf paths.
 
> * (minor) for completeness, you should limit the PCI class in the 
> pci_device_id table to PCI_CLASS_NETWORK_ETHERNET.  There are 
> one-in-a-million cases where this matters, but it's usually a 
> BIOS bug. Still, it's there in pci_device_id table, and it's an easy 
> change, so might as well use it.

OK

> * (style) your struct config definition is terribly clever.  
> perhaps too clever, making it unreadable?  Not a specific complaint,
> mind you, just something that caught my eye.

Then the driver would be perfect.  We can't have that.  ;-)

> * (minor) in tg3, my own benchmarks and experiments showed it 
> helped to explictly use ____cacheline_aligned markers when
> defining certain sections of members in struct tg3
> (or struct nic, in e100's case).  You already clearly pay
> attention to member layout WRT cache effects, but if 
> you have a clear dividing line, or lines, in struct nic you can use 
> _____cacheline_aligned for even greater benefit.  At a 
> minimum test it with a cpu-usage-measuring benchmark like ttcp,
> though, of course :) 

OK

> * (extremely minor) some people (like me :)) consider dead reads like 
> the readb() call in e100_write_flush

OK
 
> * (major?) Aren't there some clunky e100 adapters that don't do MMIO? 
> Do we care?

Not that I'm aware of.  Current e100 doesn't support them if they're out
there.
 
> * I would love to see feedback from people testing this 
> driver on ppc64 and sparc64, particularly.

Me too.  Things seem to work on ppc (Mac) and ia64.

> * (style, minor) My eyes would prefer functions like e100_hw_reset to 
> have a few more blank lines in them, spreading code+comment 
> blocks out a  bit.

OK
 
> * (extremely minor) one wonders if you really need the write flush in 
> mdio_ctrl.  If the flush is removed, the same net effect 
> appears to occur.

Good catch.

> * (boring but needed) convert all the magic numbers in e100_configure 
> into constants, or at least add comments describing the magic 
> numbers. If the value is just one bit, you might simply append "/* 
> true */", for example.  The general idea is to make the "member name =

> value" list a little bit more readable to somebody who doesn't know
the 
> hardware, and struct config, intimately.

That _was_ boring.
 
> * IIRC Donald's MII phy scanning code scans MII phy ids like this: 
> 1..31,0.  Or maybe 1..31, and then 0 iff no MII phys were found.  In 
> general I would prefer to follow his eepro100.c probe order.  
> Some phys need this because they will report on both phy id #0 (which 
> is magical) and phy id #(non-zero).  Donald would know more than me,
here.

[kernel] eepro100 gets the ID from the eeprom, so no scanning there.
Current e100 goes 1, 0..31, which is what we've always done, IIRC.

> * Is it easy to support MII phy interrupts?  It would be nice 
> to get a callback that was handled immediately, on phys that 
> do support such interrupts.

I don't see those being passed through and handled by the MAC.

> * do we care about spinlocks around the update_stats and 
> get_stats code?

Not sure.  update_stats runs in a timer callback.  Can get_stats jump
in?

> * (bugs) in e100_up, you should undo mod_timer [major] and 
> netif_start_queue [minor], if request_irq fails.  And maybe stop the 
> receiver, too?

OK

> * for all constants 0xffffffff (and others as well if you so choose), 
> prefer the C99 suffix to a cast.  This is particularly relevant in 
> pci_set_dma_mask calls, where one should be using 0xffffffffULL, but 
> applies to other constants as well.

I didn't see any other constant casts besides the pci_set_dma_mask call.
That one is fixed.

> * (potential races) in e100_probe, you want to call 
> register_netdev as basically the last operation that can
> fail, if possible.  Particularly, you need to move the
> PCI API operations above register_netdev. 
> Remember, register_netdev winds up calling /sbin/hotplug, 
> which in turn calls programs that will want to start using
> the interface.  So you need to have everything set up by
> that point, really.

OK (nice catch).
 
> * in e100_probe, "if(nic->csr == 0UL) {" should really just test for 
> NULL, because ioremap is defined to return a pointer...

OK
 
> * (minor) use a netif_msg_xxx wrapper/constant in 
> e100_init_module test?

Can't - don't have nic->msg_enable allocated yet.  :(

-scott

Re: e100 "Ferguson" release

[Jeff Garzik]

Feldman, Scott wrote:
>>* (API) Does the out-of-tx-resources condition in 
>>e100_xmit_frame ever really happen?  I am under the
>>impression that returning non-zero in ->hard_start_xmit
>>results in the packet sometimes being requeued and
>>sometimes dropped.  I prefer to guarantee a more-steady 
>>state, by simply dropping the packet unconditionally,
>>when this uncommon condition occurs.  So, I would
>>a) mark the failure condition with unlikely(), and
>>b) if the condition occurs, simply drop the packet 
>>(tx_dropped++, kfree 
>>skb), and return zero.
> 
> 
> Stop the queue also?
> 
> if(unlikely(e100_exec_cb(nic, skb, e100_xmit_prepare) == -ENOMEM)) {
>         netif_stop_queue(netdev);
>         nic->net_stats.tx_dropped++;
>         dev_kfree_skb(skb);
>         return 0;
> }

Yes.  I would also printk(KERN_ERR "we have a bug!") or somesuch, like 
several other drivers do, too.


>>* IIRC Donald's MII phy scanning code scans MII phy ids like this: 
>>1..31,0.  Or maybe 1..31, and then 0 iff no MII phys were found.  In 
>>general I would prefer to follow his eepro100.c probe order.  
>>Some phys need this because they will report on both phy id #0 (which 
>>is magical) and phy id #(non-zero).  Donald would know more than me,
> 
> here.
> 
> [kernel] eepro100 gets the ID from the eeprom, so no scanning there.
> Current e100 goes 1, 0..31, which is what we've always done, IIRC.

hmmm.  I prefer the phy scanning to checking eeprom, since it reduces 
the chance of eeprom screwups.  However, I still think there's some 
issue related to phy id #0.  Oh well, fine for now, I guess.


>>* do we care about spinlocks around the update_stats and 
>>get_stats code?
> 
> 
> Not sure.  update_stats runs in a timer callback.  Can get_stats jump
> in?

Well, the ->get_stats only returns a pointer to the stats, which are 
then accessed in an unlocked manner.  Since the net stats are unsigned 
longs, asynchronously reading and updating them isn't a big deal in 
practice.


>>* (minor) use a netif_msg_xxx wrapper/constant in 
>>e100_init_module test?
> 
> 
> Can't - don't have nic->msg_enable allocated yet.  :(

You could always use "(1 << debug) - 1"... :)  I dunno if it's worth 
worrying about.

	Jeff

Re: e100 "Ferguson" release

[David S. Miller]

On Mon, 4 Aug 2003 20:45:08 -0700
"Feldman, Scott" <scott.feldman@intel.com> wrote:

> > * I would love to see feedback from people testing this 
> > driver on ppc64 and sparc64, particularly.
> 
> Me too.  Things seem to work on ppc (Mac) and ia64.

This gets things building on sparc64, I'll stick an e100
into my workstation and use it for everything for a while
using this driver.

--- Makefile.~1~	2003-08-04 20:20:42.000000000 -0700
+++ Makefile	2003-08-05 00:12:29.000000000 -0700
@@ -96,10 +96,15 @@
 endif
 
 # pick a compiler
-ifneq (,$(findstring egcs-2.91.66, $(shell cat /proc/version)))
-  CC := kgcc gcc cc
+ARCH := $(shell uname -m | sed 's/i.86/i386/')
+ifeq ($(ARCH),sparc64)
+CC		:= $(shell if gcc -m64 -S -o /dev/null -xc /dev/null >/dev/null 2>&1; then echo gcc; else echo sparc64-linux-gcc; fi )
 else
-  CC := gcc cc
+  ifneq (,$(findstring egcs-2.91.66, $(shell cat /proc/version)))
+    CC := kgcc gcc cc
+  else
+    CC := gcc cc
+  endif
 endif
 test_cc = $(shell which $(cc) > /dev/null 2>&1 && echo $(cc))
 CC := $(foreach cc, $(CC), $(test_cc))
@@ -198,10 +203,30 @@
 
 # we need to know what platform the driver is being built on
 # some additional features are only built on Intel platforms
-ARCH := $(shell uname -m | sed 's/i.86/i386/')
 ifeq ($(ARCH),alpha)
   CFLAGS += -ffixed-8 -mno-fp-regs
 endif
+ifeq ($(ARCH),sparc64)
+  NEW_GCC := $(shell if $(CC) -m64 -mcmodel=medlow -S -o /dev/null -xc /dev/null >/dev/null 2>&1; then echo y; else echo n; fi; )
+  UNDECLARED_REGS := $(shell if $(CC) -c -x assembler /dev/null -Wa,--help | grep undeclared-regs > /dev/null; then echo y; else echo n; fi; )
+  INLINE_LIMIT := $(shell if $(CC) -m64 -finline-limit=100000 -S -o /dev/null -xc /dev/null >/dev/null 2>&1; then echo y; else echo n; fi; )
+  ifneq ($(UNDECLARED_REGS),y)
+    CC_UNDECL	=
+  else
+    CC_UNDECL	= -Wa,--undeclared-regs
+  endif
+  ifneq ($(NEW_GCC),y)
+    CFLAGS += -pipe -mno-fpu -mtune=ultrasparc -mmedlow \
+	      -ffixed-g4 -fcall-used-g5 -fcall-used-g7 -Wno-sign-compare
+  else
+    CFLAGS += -m64 -pipe -mno-fpu -mcpu=ultrasparc -mcmodel=medlow \
+	      -ffixed-g4 -fcall-used-g5 -fcall-used-g7 -Wno-sign-compare \
+	      $(CC_UNDECL)
+  endif
+  ifeq ($(INLINE_LIMIT),y)
+    CFLAGS := $(CFLAGS) -finline-limit=100000
+  endif
+endif
 
 # depmod version for rpm builds
 DEPVER := $(shell /sbin/depmod -V 2>/dev/null | awk 'BEGIN {FS="."} NR==1 {print $$2}')
--- e100.c.~1~	2003-08-04 20:20:42.000000000 -0700
+++ e100.c	2003-08-05 00:13:23.000000000 -0700
@@ -150,6 +150,7 @@
 #include <linux/ethtool.h>
 #include <asm/unaligned.h>
 #include <asm/uaccess.h>
+#include <asm/io.h>
 
 #include "kcompat.h"
 

RE: e100 "Ferguson" release

[Feldman, Scott]

> I've also noticed that the number of hard_start_xmit failures 
> in e1000 has increased significantly in version 5.1.13-k1. In 
> version 5.0.43-k1 the number of failures was much smaller.

Interesting.  Felix, would you undo the change[1] below in 5.1.13-k1 and
see what happens?  With the change below, 5.1.13 would be more
aggressive on Tx cleanup, so we'll be quicker waking the queue than
before. 

-scott

        for(i = 0; i < E1000_MAX_INTR; i++)
-               if(!e1000_clean_rx_irq(adapter) &&
+               if(!e1000_clean_rx_irq(adapter) &
                   !e1000_clean_tx_irq(adapter))
                        break;

[1] Something still bothers me about this new form where we're mixing a
bit-wise operator with logical operands.  Should this bother me?

Re: e100 "Ferguson" release

[Jeff Garzik]

On Tue, Aug 05, 2003 at 08:19:25AM -0700, Feldman, Scott wrote:
> > I've also noticed that the number of hard_start_xmit failures 
> > in e1000 has increased significantly in version 5.1.13-k1. In 
> > version 5.0.43-k1 the number of failures was much smaller.
> 
> Interesting.  Felix, would you undo the change[1] below in 5.1.13-k1 and
> see what happens?  With the change below, 5.1.13 would be more
> aggressive on Tx cleanup, so we'll be quicker waking the queue than
> before. 
> 
> -scott
> 
>         for(i = 0; i < E1000_MAX_INTR; i++)
> -               if(!e1000_clean_rx_irq(adapter) &&
> +               if(!e1000_clean_rx_irq(adapter) &
>                    !e1000_clean_tx_irq(adapter))
>                         break;
> 
> [1] Something still bothers me about this new form where we're mixing a
> bit-wise operator with logical operands.  Should this bother me?

It doesn't matter to the compiler if you make it explicit:

	unsigned int rx_work = e1000_clean_rx_irq();
	unsigned int tx_work = e1000_clean_tx_irq();
	if (!rx_work && !tx_work)
		break;

Re: e100 "Ferguson" release

[Felix Radensky]

[-- Attachment #1: Type: text/plain, Size: 1217 bytes --]

Hi, Jeff, Scott

Are you planning to fix this before 2.4.22-final ?

Thanks.

Felix.

Jeff Garzik wrote:

>On Tue, Aug 05, 2003 at 08:19:25AM -0700, Feldman, Scott wrote:
>  
>
>>>I've also noticed that the number of hard_start_xmit failures 
>>>in e1000 has increased significantly in version 5.1.13-k1. In 
>>>version 5.0.43-k1 the number of failures was much smaller.
>>>      
>>>
>>Interesting.  Felix, would you undo the change[1] below in 5.1.13-k1 and
>>see what happens?  With the change below, 5.1.13 would be more
>>aggressive on Tx cleanup, so we'll be quicker waking the queue than
>>before. 
>>
>>-scott
>>
>>        for(i = 0; i < E1000_MAX_INTR; i++)
>>-               if(!e1000_clean_rx_irq(adapter) &&
>>+               if(!e1000_clean_rx_irq(adapter) &
>>                   !e1000_clean_tx_irq(adapter))
>>                        break;
>>
>>[1] Something still bothers me about this new form where we're mixing a
>>bit-wise operator with logical operands.  Should this bother me?
>>    
>>
>
>It doesn't matter to the compiler if you make it explicit:
>
>	unsigned int rx_work = e1000_clean_rx_irq();
>	unsigned int tx_work = e1000_clean_tx_irq();
>	if (!rx_work && !tx_work)
>		break;
>
>
>  
>


[-- Attachment #2: Type: text/html, Size: 1605 bytes --]

Re: e100 "Ferguson" release

[Felix Radensky]

[-- Attachment #1: Type: text/plain, Size: 908 bytes --]

Hi, Scott

This change seems to fix the problem.
Thanks a lot !

Felix.

Feldman, Scott wrote:

>>I've also noticed that the number of hard_start_xmit failures 
>>in e1000 has increased significantly in version 5.1.13-k1. In 
>>version 5.0.43-k1 the number of failures was much smaller.
>>    
>>
>
>Interesting.  Felix, would you undo the change[1] below in 5.1.13-k1 and
>see what happens?  With the change below, 5.1.13 would be more
>aggressive on Tx cleanup, so we'll be quicker waking the queue than
>before. 
>
>-scott
>
>        for(i = 0; i < E1000_MAX_INTR; i++)
>-               if(!e1000_clean_rx_irq(adapter) &&
>+               if(!e1000_clean_rx_irq(adapter) &
>                   !e1000_clean_tx_irq(adapter))
>                        break;
>
>[1] Something still bothers me about this new form where we're mixing a
>bit-wise operator with logical operands.  Should this bother me?
>
>  
>


[-- Attachment #2: Type: text/html, Size: 1264 bytes --]

RE: e100 "Ferguson" release

[Feldman, Scott]

> > if(unlikely(e100_exec_cb(nic, skb, e100_xmit_prepare) == -ENOMEM)) {
> >         netif_stop_queue(netdev);
> >         nic->net_stats.tx_dropped++;
> >         dev_kfree_skb(skb);
> >         return 0;
> > }
> 
> Yes.  I would also printk(KERN_ERR "we have a bug!") or 
> somesuch, like several other drivers do, too.

It's there, sorry, was trying to keep the code snippet small.

> >>* (minor) use a netif_msg_xxx wrapper/constant in
> >>e100_init_module test?
> > 
> > 
> > Can't - don't have nic->msg_enable allocated yet.  :(
> 
> You could always use "(1 << debug) - 1"... :)  I dunno if it's worth 
> worrying about.

(1 << debug) - 1) & NETIF_MSG_DRV is what's there now.

-scott

e100 "Ferguson" release

[Feldman, Scott]

New development version:

	http://sf.net/projects/e1000, e100-3.0.0_dev11.tar.gz

Many thanks to JC [jchapman@katalix.com] for exploring the small packet
performance w/ and w/o NAPI.  This version includes one of his
optimization; others may follow, but I wanted to get this goodness out
now.

* added opportunistic fast loop (no udelays) in 
  e100_exec_cmd to wait for previous cmd to be
  accepted before queuing next cmd.  Boost 
  small packet performance.  [jchapman@katalix.com].
* Use correct versions of dev_kfree_skb for depending
  on possible contexts.  [jchapman@katalix.com].
* Added SET_NETDEV_DEV().

Looking for more testing on non-IA archs, power management, cardbus
nics, and WoL.

-scott

Re: e100 "Ferguson" release

[Jeff Garzik]

Comments:

* Given that e100 is only 10/100 hardware, I like the decision to not 
support rx/tx checksumming and zero-copy.

Particularly with some e100's, this eliminates various worries related 
to chip errata.  And as with any "do it in software" solution, you 
guarantee that the chip never screws up and "acks" a checksum 
incorrectly, thus passing corrupted data up into the net stack.

* (API) Does the out-of-tx-resources condition in e100_xmit_frame ever 
really happen?  I am under the impression that returning non-zero in 
->hard_start_xmit results in the packet sometimes being requeued and 
sometimes dropped.  I prefer to guarantee a more-steady state, by simply 
dropping the packet unconditionally, when this uncommon condition 
occurs.  So, I would
a) mark the failure condition with unlikely(), and
b) if the condition occurs, simply drop the packet (tx_dropped++, kfree 
skb), and return zero.

Though, ultimately, I wish the net stack would support some way to 
_guarantee_ that the skb is requeued for transmit.  Some packet 
schedulers in the kernel will drop the skb even if the ->hard_start_xmit 
return code indicates "requeue".  This makes sense from the rule of 
"skbs are lossy, and can be dropped"... but it really sucks on hardware 
where unexpected -- but temporary -- loss of TX resources occurs.  One 
can prevent 20-50% (or more) packet loss on certain classes of 
connections, simply by being able to tell the net stack "hey, if I could 
go back in time and issue a netif_stop_queue, before you called 
->hard_start_xmit, I would" :)

* (minor) for completeness, you should limit the PCI class in the 
pci_device_id table to PCI_CLASS_NETWORK_ETHERNET.  There are 
one-in-a-million cases where this matters, but it's usually a BIOS bug. 
  Still, it's there in pci_device_id table, and it's an easy change, so 
might as well use it.

This is a good janitor task for other PCI net drivers, too.

* (long term) I really like Ben H.'s work in drivers/net/sungem_phy.[ch] 
-- and similar benh code in ibm_emac -- and want to make his code 
generic for most MII phys.  Just something to read and keep in mind.

* (style) your struct config definition is terribly clever.  perhaps too 
clever, making it unreadable?  Not a specific complaint, mind you, just 
something that caught my eye.

* (minor) in tg3, my own benchmarks and experiments showed it helped to 
explictly use ____cacheline_aligned markers when defining certain 
sections of members in struct tg3 (or struct nic, in e100's case).  You 
already clearly pay attention to member layout WRT cache effects, but if 
  you have a clear dividing line, or lines, in struct nic you can use 
_____cacheline_aligned for even greater benefit.  At a minimum test it 
with a cpu-usage-measuring benchmark like ttcp, though, of course :) 
IIRC I divided tg3's struct into rx, tx, and "other" sections.

* (extremely minor) some people (like me :)) consider dead reads like 
the readb() call in e100_write_flush

* (major?) Aren't there some clunky e100 adapters that don't do MMIO? 
Do we care?

* I would love to see feedback from people testing this driver on ppc64 
and sparc64, particularly.

* (style, minor) My eyes would prefer functions like e100_hw_reset to 
have a few more blank lines in them, spreading code+comment blocks out a 
bit.

* (extremely minor) one wonders if you really need the write flush in 
mdio_ctrl.  If the flush is removed, the same net effect appears to occur.

* (boring but needed) convert all the magic numbers in e100_configure 
into constants, or at least add comments describing the magic numbers. 
If the value is just one bit, you might simply append "/* true */", for 
example.  The general idea is to make the "member name = value" list a 
little bit more readable to somebody who doesn't know the hardware, and 
struct config, intimately.

* IIRC Donald's MII phy scanning code scans MII phy ids like this: 
1..31,0.  Or maybe 1..31, and then 0 iff no MII phys were found.  In 
general I would prefer to follow his eepro100.c probe order.  Some phys 
need this because they will report on both phy id #0 (which is magical) 
and phy id #(non-zero).  Donald would know more than me, here.

* I like the e100_exec_cb stuff, with the callbacks.

* Is it easy to support MII phy interrupts?  It would be nice to get a 
callback that was handled immediately, on phys that do support such 
interrupts.

* do we care about spinlocks around the update_stats and get_stats code?

* (bugs) in e100_up, you should undo mod_timer [major] and 
netif_start_queue [minor], if request_irq fails.  And maybe stop the 
receiver, too?

* for all constants 0xffffffff (and others as well if you so choose), 
prefer the C99 suffix to a cast.  This is particularly relevant in 
pci_set_dma_mask calls, where one should be using 0xffffffffULL, but 
applies to other constants as well.

* (potential races) in e100_probe, you want to call register_netdev as 
basically the last operation that can fail, if possible.  Particularly, 
you need to move the PCI API operations above register_netdev. 
Remember, register_netdev winds up calling /sbin/hotplug, which in turn 
calls programs that will want to start using the interface.  So you need 
to have everything set up by that point, really.

* in e100_probe, "if(nic->csr == 0UL) {" should really just test for 
NULL, because ioremap is defined to return a pointer...

* (minor) use a netif_msg_xxx wrapper/constant in e100_init_module test?

Re: e100 "Ferguson" release

[Jeff Garzik]

Jeff Garzik wrote:
> * (extremely minor) some people (like me :)) consider dead reads like 
> the readb() call in e100_write_flush

er, that was a bit incomplete.

completing:

  ... needing to be marked explicitly with a "(void) " prefix, 
indicating it is intentionally a dead read.

Maintainer's call, ultimately, though...

Re: e100 "Ferguson" release

[Ben Greear]

Jeff Garzik wrote:
> Comments:

> * (API) Does the out-of-tx-resources condition in e100_xmit_frame ever 
> really happen?  I am under the impression that returning non-zero in 
> ->hard_start_xmit results in the packet sometimes being requeued and 
> sometimes dropped.  I prefer to guarantee a more-steady state, by simply 
> dropping the packet unconditionally, when this uncommon condition 
> occurs.  So, I would
> a) mark the failure condition with unlikely(), and
> b) if the condition occurs, simply drop the packet (tx_dropped++, kfree 
> skb), and return zero.
> 
> Though, ultimately, I wish the net stack would support some way to 
> _guarantee_ that the skb is requeued for transmit.  Some packet 
> schedulers in the kernel will drop the skb even if the ->hard_start_xmit 
> return code indicates "requeue".  This makes sense from the rule of 
> "skbs are lossy, and can be dropped"... but it really sucks on hardware 
> where unexpected -- but temporary -- loss of TX resources occurs.  One 
> can prevent 20-50% (or more) packet loss on certain classes of 
> connections, simply by being able to tell the net stack "hey, if I could 
> go back in time and issue a netif_stop_queue, before you called 
> ->hard_start_xmit, I would" :)

Although I have not tried this latest patch, the existing e100 and e1000 in
2.4.21 seldom seem to return true to this method:  netif_queue_stopped(odev),
even when the next hard_start_xmit() call fails.  For instance, this is the
code I use in pktgen.c:

                         if (!netif_queue_stopped(odev)) {
                                 if (odev->hard_start_xmit(next->skb, odev)) {
                                         if (net_ratelimit()) {
                                                 printk(KERN_INFO "Hard xmit error\n");
                                         }
                                         next->errors++;
                                         next->last_ok = 0;
                                         queue_stopped++;
                                 }
                                 else {
                                         queue_stopped = 0;
                                         next->last_ok = 1;
                                         next->sofar++;
                                         next->tx_bytes += (next->cur_pkt_size + 4); /* count csum */
                                 }

With e100 and e1000, I see the very large numbers of the hard_start_xmit failure
when running very high packets-per-second rates (small packets).
I see virtually no failures with tulip.  pktgen knows how to re-queue, but it's
curious it has to so often.  For code that does not requeue, this could be even
more of a bummer.

To point b), I think if the driver accepts the packet in hard_start_xmit, it should
be able to send the packet out, otherwise return the 'requeue' value and let the
calling code know.  It is very important to me, at least, to know if a packet has
really been sent or not.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: e100 "Ferguson" release

[David S. Miller]

> Although I have not tried this latest patch, the existing e100 and e1000 in
> 2.4.21 seldom seem to return true to this method:  netif_queue_stopped(odev),
> even when the next hard_start_xmit() call fails.

Returning an error from hard_start_xmit() from normal ethernet
drivers is, frankly, a driver bug and should never happen.

I don't know if there is somehow something special about the
e100, but even if there is hard_start_xmit() failures can
be avoided by properly doing netif_queue_{stop,wakeup}() in
the right places.

Re: e100 "Ferguson" release

[David Brownell]

David S. Miller wrote:
>>Although I have not tried this latest patch, the existing e100 and e1000 in
>>2.4.21 seldom seem to return true to this method:  netif_queue_stopped(odev),
>>even when the next hard_start_xmit() call fails.
> 
> 
> Returning an error from hard_start_xmit() from normal ethernet
> drivers is, frankly, a driver bug and should never happen.

What's "normal" mean?

With the current USB stack, network adapters tend to need
memory allocations there.  Those can fail, though it seems
that's not very common.  Doesn't seem like a bug, for all
that I'd rather see the those paths be zero-alloc in 2.7.

- Dave

Re: e100 "Ferguson" release

[David S. Miller]

On Sun, 03 Aug 2003 20:09:10 -0700
David Brownell <david-b@pacbell.net> wrote:

> David S. Miller wrote:
> >>Although I have not tried this latest patch, the existing e100 and e1000 in
> >>2.4.21 seldom seem to return true to this method:  netif_queue_stopped(odev),
> >>even when the next hard_start_xmit() call fails.
> > 
> > 
> > Returning an error from hard_start_xmit() from normal ethernet
> > drivers is, frankly, a driver bug and should never happen.
> 
> What's "normal" mean?

One that can manage it's own TX resources.

> With the current USB stack, network adapters tend to need
> memory allocations there.  Those can fail, though it seems
> that's not very common.  Doesn't seem like a bug, for all
> that I'd rather see the those paths be zero-alloc in 2.7.

Any particular reason why the SKB data itself can't be
mapped directly?  We created all of these DMA mapping
abstractions remember? :-)

Another option is to pre-allocate, such that while the TX
queue is awake we know we have enough resources to send any
given packet.  Then in ->hard_start_xmit() after using a buffer
we allocate a replacement buffer, if this fails in such a way
that a subsequent ->hard_start_xmit() could possibly fail, we
do netif_stop_queue().

Look to tg3 to see what I'm talking about in general.
netif_stop_queue() is done at the moment at which it may be possible
that we cannot accept the queueing of a TX packet.  This means that
when TX entries available <= MAX_SKB_FRAGS + 1, we stop the queue.
This guarentees that we will always be able to handle any packet given
to us via ->hard_start_xmit().

Re: e100 "Ferguson" release

[David Brownell]

>>>>Although I have not tried this latest patch, the existing e100 and e1000 in
>>>>2.4.21 seldom seem to return true to this method:  netif_queue_stopped(odev),
>>>>even when the next hard_start_xmit() call fails.
>>>
>>>
>>>Returning an error from hard_start_xmit() from normal ethernet
>>>drivers is, frankly, a driver bug and should never happen.
>>
>>What's "normal" mean?
> 
> 
> One that can manage it's own TX resources.

Which for the moment, would seem to exclude USB.


>>With the current USB stack, network adapters tend to need
>>memory allocations there.  Those can fail, though it seems
>>that's not very common.  Doesn't seem like a bug, for all
>>that I'd rather see the those paths be zero-alloc in 2.7.
> 
> 
> Any particular reason why the SKB data itself can't be
> mapped directly?  We created all of these DMA mapping
> abstractions remember? :-)

Yes, but the network drivers weren't the ones that needed them!

Some older drivers do copy the buffer out of (or for rx, into)
the skb, but newer ones just pass the skb data, avoiding a copy.
In either case, the buffer was always DMA mapped.  Nowadays,
some drivers will even set NETIF_F_HIGHDMA if they're going out
through a host controller that allows it!  (Intel boxes only,
AFAIK.)


> Another option is to pre-allocate, such that while the TX
> queue is awake we know we have enough resources to send any
> given packet.  Then in ->hard_start_xmit() after using a buffer
> we allocate a replacement buffer, if this fails in such a way
> that a subsequent ->hard_start_xmit() could possibly fail, we
> do netif_stop_queue().

Pre-allocation can be done for the URBs that wrap the data
buffers, yes.  Not often done today; but it could be.

What can't be pre-allocated in a reliable way is the resources
used by the host controller drivers, specifically the transfer
descriptors.  EHCI and OHCI usually need one per URB, unless
MTU is over 4 KB.  UHCI normally needs quite a few.

The API that works inside USB "gadgets' does allow pre-allocation
at all those levels, mostly because it's factored to make the
submission and completion paths fast.  So that "stop if can't
pre-allocate" scheme would work, given an "ether.c" patch!  :)

- Dave

Re: e100 "Ferguson" release

[David S. Miller]

On Sun, 03 Aug 2003 20:45:01 -0700
David Brownell <david-b@pacbell.net> wrote:

> What can't be pre-allocated in a reliable way is the resources
> used by the host controller drivers, specifically the transfer
> descriptors.  EHCI and OHCI usually need one per URB, unless
> MTU is over 4 KB.  UHCI normally needs quite a few.

Ok, that's interesting.

Is there a callback that tells the USB driver that some host
controller "resources" have become available?  I mean, these host
controllers either have to queue requests when out of resources or
provide a callback so that the drivers can resubmit.

Right?

Re: e100 "Ferguson" release

[David Brownell]

David S. Miller wrote:
> On Sun, 03 Aug 2003 20:45:01 -0700
> David Brownell <david-b@pacbell.net> wrote:
> 
> 
>>What can't be pre-allocated in a reliable way is the resources
>>used by the host controller drivers, specifically the transfer
>>descriptors.  EHCI and OHCI usually need one per URB, unless
>>MTU is over 4 KB.  UHCI normally needs quite a few.
> 
> 
> Ok, that's interesting.

All TDs get allocated in usb_submit_urb(), which is the first
time the "real" core of USB connects an urb with an I/O queue.
That's host-side, not device-side.

> Is there a callback that tells the USB driver that some host
> controller "resources" have become available?  I mean, these host
> controllers either have to queue requests when out of resources or
> provide a callback so that the drivers can resubmit.

No such callback.  If no resources, they fail -ENOMEM and the
caller must recover.  Which is why hard_start_xmit() needs to
do something.

- Dave

Re: e100 "Ferguson" release

[David S. Miller]

On Sun, 03 Aug 2003 21:08:26 -0700
David Brownell <david-b@pacbell.net> wrote:

> No such callback.  If no resources, they fail -ENOMEM and the
> caller must recover.  Which is why hard_start_xmit() needs to
> do something.

I would suggest something different :-)

For example, what do USB block device drivers do when -ENOMEM comes
back?  Do they just drop the request on the floor?  No, rather they
resubmit the request later without the scsi/block layer knowing
anything about what happened, right?

How do the USB block device drivers know when "later" is?  This is why
I can't believe there is not some kind of "some USB resources have
been freed" event of some sort which USB drivers can use to deal with
this.  :-)

Re: e100 "Ferguson" release

[David Brownell]

David S. Miller wrote:
>
> For example, what do USB block device drivers do when -ENOMEM comes
> back?  Do they just drop the request on the floor?  No, rather they
> resubmit the request later without the scsi/block layer knowing
> anything about what happened, right?

I didn't notice any code to retry, but I did see some that morphed
ENOMEM into a generic scsi "error".  Scsi presumably does something
more or less intelligent then.

The network layer on the other hand _does_ have hooks for retrying,
not that they're used much.

- Dave

Re: e100 "Ferguson" release

[Felix Radensky]

I've also noticed that the number of hard_start_xmit failures in e1000
has increased significantly in version 5.1.13-k1. In version 5.0.43-k1 the
number of failures was much smaller.

Felix.

Ben Greear wrote:

>
>
>
> With e100 and e1000, I see the very large numbers of the 
> hard_start_xmit failure
> when running very high packets-per-second rates (small packets).
> I see virtually no failures with tulip.  pktgen knows how to re-queue, 
> but it's
> curious it has to so often.  For code that does not requeue, this 
> could be even
> more of a bummer.
>
>
>