[PATCH] pktgen handle netdev device getting full.

[PATCH] pktgen handle netdev device getting full.

[Stephen Hemminger]

I was trying out pktgen on a NIC with an undersized ring, so 
hard_start_xmit would always return non-zero when full.  This caused a slew
of console messages.  Better to just have pktgen retry in this case.

Also, can use do_div to do the 64 bit divide rather than doing long string
of if statements when computing results.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

diff -Nru a/net/core/pktgen.c b/net/core/pktgen.c
--- a/net/core/pktgen.c	2004-09-16 14:42:25 -07:00
+++ b/net/core/pktgen.c	2004-09-16 14:42:25 -07:00
@@ -589,7 +589,7 @@
 {
 	struct net_device *odev = NULL;
 	struct sk_buff *skb = NULL;
-	__u64 total = 0;
+	__u32 total = 0;
 	__u64 idle = 0;
 	__u64 lcount = 0;
 	int nr_frags = 0;
@@ -636,28 +636,20 @@
 		   
 		if (!(odev->features & NETIF_F_LLTX))
 			spin_lock_bh(&odev->xmit_lock);
-		if (!netif_queue_stopped(odev)) {
 
+		last_ok = 0;
+		if (!netif_queue_stopped(odev)) {
 			atomic_inc(&skb->users);
 
-			if (odev->hard_start_xmit(skb, odev)) {
-
+			if (odev->hard_start_xmit(skb, odev) == NETDEV_TX_OK) {
+				last_ok = 1;	
+				info->sofar++;
+				info->seq_num++;
+			} else {
+				/* Device kicked us out :(
+				   so retry again */
 				atomic_dec(&skb->users);
-				if (net_ratelimit()) {
-				   printk(KERN_INFO "Hard xmit error\n");
-				}
-				info->errors++;
-				last_ok = 0;
 			}
-			else {
-			   last_ok = 1;	
-			   info->sofar++;
-			   info->seq_num++;
-			}
-		}
-		else {
-			/* Re-try it next time */
-			last_ok = 0;
 		}
 		
 		if (!(odev->features & NETIF_F_LLTX))
@@ -733,15 +725,9 @@
 	{
 		char *p = info->result;
 		__u64 bps, pps = 0;
-
-		if (total > 1000)
-			pps = (__u32)(info->sofar * 1000) / ((__u32)(total) / 1000);
-		else if(total > 100)
-			pps = (__u32)(info->sofar * 10000) / ((__u32)(total) / 100);
-		else if(total > 10)
-			pps = (__u32)(info->sofar * 100000) / ((__u32)(total) / 10);
-		else if(total > 1)
-			pps = (__u32)(info->sofar * 1000000) / (__u32)total;
+		
+		pps = info->sofar * 1000000;
+		do_div(pps, total);
 
 		bps = pps * 8 * (info->pkt_size + 4); /* take 32bit ethernet CRC into account */
 		p += sprintf(p, "OK: %llu(c%llu+d%llu) usec, %llu (%dbyte,%dfrags) %llupps %lluMb/sec (%llubps)  errors: %llu",

Re: [PATCH] pktgen handle netdev device getting full.

[Ben Greear]

Stephen Hemminger wrote:
> I was trying out pktgen on a NIC with an undersized ring, so 
> hard_start_xmit would always return non-zero when full.  This caused a slew
> of console messages.  Better to just have pktgen retry in this case.

My understanding is that if the queue is not stopped, then you
should not get the hard xmit errors.  So, in a proper driver,
you should not see these printks.

By the way, is there any interest in adding my patch that also allows
pktgen to receive packets (and count the statistics, etc)?  I know
DaveM objected to the hook in the skb-receive logic some time back,
but maybe he has a different opinion now?

Ben

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

Re: [PATCH] pktgen handle netdev device getting full.

[David S. Miller]

On Thu, 16 Sep 2004 15:59:30 -0700
Ben Greear <greearb@candelatech.com> wrote:

> Stephen Hemminger wrote:
> > I was trying out pktgen on a NIC with an undersized ring, so 
> > hard_start_xmit would always return non-zero when full.  This caused a slew
> > of console messages.  Better to just have pktgen retry in this case.
> 
> My understanding is that if the queue is not stopped, then you
> should not get the hard xmit errors.  So, in a proper driver,
> you should not see these printks.

That is absolutely correct, that is why Stephen's patch is
not correct (aside from the do_div() part which I'll happily
apply if submitted by itself).

> By the way, is there any interest in adding my patch that also allows
> pktgen to receive packets (and count the statistics, etc)?  I know
> DaveM objected to the hook in the skb-receive logic some time back,
> but maybe he has a different opinion now?

I still object to this, it will be abused.

Re: [PATCH] pktgen handle netdev device getting full.

[Stephen Hemminger]

On Thu, 16 Sep 2004 15:59:13 -0700
"David S. Miller" <davem@davemloft.net> wrote:

> On Thu, 16 Sep 2004 15:59:30 -0700
> Ben Greear <greearb@candelatech.com> wrote:
> 
> > Stephen Hemminger wrote:
> > > I was trying out pktgen on a NIC with an undersized ring, so 
> > > hard_start_xmit would always return non-zero when full.  This caused a slew
> > > of console messages.  Better to just have pktgen retry in this case.
> > 
> > My understanding is that if the queue is not stopped, then you
> > should not get the hard xmit errors.  So, in a proper driver,
> > you should not see these printks.

Well the E1000 fill's it Tx ring and returns TX_BUSY, and on
SMP can also get TX_LOCKED now.  If that happens, the message shows up.
Several other drivers don't set stopped when ring is almost full, but
rely on scheduler to do it for them.


> That is absolutely correct, that is why Stephen's patch is
> not correct (aside from the do_div() part which I'll happily
> apply if submitted by itself).
> 
> > By the way, is there any interest in adding my patch that also allows
> > pktgen to receive packets (and count the statistics, etc)?  I know
> > DaveM objected to the hook in the skb-receive logic some time back,
> > but maybe he has a different opinion now?
> 
> I still object to this, it will be abused.

Robert also has a new version of pktgen but he wanted to hold off till
2.7, but now that 2.7 is delayed maybe it should come sooner.

Re: [PATCH] pktgen handle netdev device getting full.

[David S. Miller]

On Thu, 16 Sep 2004 16:20:51 -0700
Stephen Hemminger <shemminger@osdl.org> wrote:

> Well the E1000 fill's it Tx ring and returns TX_BUSY, and on
> SMP can also get TX_LOCKED now.  If that happens, the message shows up.
> Several other drivers don't set stopped when ring is almost full, but
> rely on scheduler to do it for them.

TX_LOCKED is special, and actually this points out that pktgen.c
needs to change how it interprets hard_start_xmit() return
values now that TX_LOCKED is possible too.

I'm sorry, is that what your patch was doing?  Then it was
correct, please resend.

Re: [PATCH] pktgen handle netdev device getting full.

[Ben Greear]

David S. Miller wrote:
> On Thu, 16 Sep 2004 15:59:30 -0700
> Ben Greear <greearb@candelatech.com> wrote:
> 
> 
>>Stephen Hemminger wrote:
>>
>>>I was trying out pktgen on a NIC with an undersized ring, so 
>>>hard_start_xmit would always return non-zero when full.  This caused a slew
>>>of console messages.  Better to just have pktgen retry in this case.
>>
>>My understanding is that if the queue is not stopped, then you
>>should not get the hard xmit errors.  So, in a proper driver,
>>you should not see these printks.
> 
> 
> That is absolutely correct, that is why Stephen's patch is
> not correct (aside from the do_div() part which I'll happily
> apply if submitted by itself).

Well, in his defense, the e1000 and probably other drivers
will cause this printk to happen (or, at least earlier versions
of the e1000 in the 2.4.25 kernel will).

>>By the way, is there any interest in adding my patch that also allows
>>pktgen to receive packets (and count the statistics, etc)?  I know
>>DaveM objected to the hook in the skb-receive logic some time back,
>>but maybe he has a different opinion now?
> 
> 
> I still object to this, it will be abused.

Even if the hook is exported GPL?  (The bridging hook is almost
identical in abusability, and it is not even exported GPL, just plain
exported....)

Just for grins, here is the /proc output from one of my tests.  This
is a pair of GigE nics running back to back on the same machine.  Since
it is the same clock, we can get very exact latency numbers, as well as
packet drops, etc....


VERSION-1
Params: count 0  min_pkt_size: 60  max_pkt_size: 60  cur_pkt_size 60
      frags: 0  ipg: 11478  multiskb: 0  ifname: eth2
      dst_min: 172.2.2.3  dst_max: 172.2.2.3
      src_min: 172.2.2.2  src_max: 172.2.2.2
      src_mac: 00:07:E9:1F:97:C9  dst_mac: 00:07:E9:1F:97:C8
      udp_src_min: 9  udp_src_max: 9  udp_dst_min: 9  udp_dst_max: 9
      src_mac_count: 0  dst_mac_count: 0  peer_multiskb: 0
      Flags:
Current:
      pkts-sofar: 3488264  errors: 0
      started: 1095376671654330us  elapsed: 43664674us
      idle: 42349478538ns  next_tx: 211092296137443(24804)ns
      seq_num: 3488265  cur_dst_mac_offset: 0  cur_src_mac_offset: 0
      cur_saddr: 0x20202ac  cur_daddr: 0x30202ac  cur_udp_dst: 9  cur_udp_src: 9
      pkts_rcvd: 3488254  bytes_rcvd: 174412700  last_seq_rcvd: 3488254  ooo_rcvd: 0
      dup_rcvd: 0  seq_gap_rcvd(dropped): 0  non_pg_rcvd: 0
      avg_latency: 148us  min_lat: 6us  max_lat: 2259us  pkts_in_sample: 3488254
       Buckets(us) [ 0  0  0  14416  55106  138901  266024  523649  1040910  1382427  66662  139  20  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  0  ]




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