Re: pktgen

Re: pktgen

[Robert Olsson]

Lennert Buytenhek writes:

 > Can you do me a favor and run the attached script on your box that can
 > do 870kpps and send me the output?


 OK! netdev is cc:ed as there are related discussions...
 sleep was 1 sec and I run SMP kernel. UP kernel give some extra kpps 
 acript data is attached as well as some systeminfo.

						--ro


 > #!/bin/sh
 > 
 > DEVICE=eth1
 > MAC=00:0E:0C:64:CC:A1
 > 
 > for size in `seq 60 4 1500`
 > do
 >         echo rem_device_all > /proc/net/pktgen/kpktgend_0
 >         echo add_device $DEVICE > /proc/net/pktgen/kpktgend_0
 > 
 >         echo min_pkt_size $size > /proc/net/pktgen/$DEVICE
 >         echo max_pkt_size $size > /proc/net/pktgen/$DEVICE
 >         echo count 1000000 > /proc/net/pktgen/$DEVICE
 >         echo clone_skb 1000000 > /proc/net/pktgen/$DEVICE
 >         echo dst_min 10.10.10.1 > /proc/net/pktgen/$DEVICE
 >         echo dst_max 10.10.10.1 > /proc/net/pktgen/$DEVICE
 >         echo src_min 10.10.10.2 > /proc/net/pktgen/$DEVICE
 >         echo src_max 10.10.10.2 > /proc/net/pktgen/$DEVICE
 >         echo src_mac $MAC > /proc/net/pktgen/$DEVICE
 >         echo dst_mac $MAC > /proc/net/pktgen/$DEVICE
 > 
 >         echo start > /proc/net/pktgen/pgctrl
 >         sleep 0.1
 >         echo -ne "$size\t"
 >         tail -1 /proc/net/pktgen/$DEVICE | awk '{print $1}' | sed s/pps//
 > done
 > 

60	825789
64	748975
68	729149
72	719721
76	720204
80	720127
84	702722
88	702799
92	705107
96	701711
100	703858
104	692120
108	696139
112	708936
116	697780
120	677887
124	678158
128	739290
132	737070
136	736894
140	737645
144	737816
148	682755
152	648547
156	646464
160	679270
164	663134
168	649420
172	637685
176	623475
180	612669
184	599538
188	588232
192	577422
196	566923
200	556811
204	547059
208	537637
212	528532
216	519271
220	511227
224	503027
228	495054
232	487353
236	479877
240	472637
244	465579
248	458754
252	452132
256	445681
260	438682
264	433324
268	427376
272	421577
276	416015
280	409920
284	405213
288	400012
292	394926
296	390010
300	385253
304	380546
308	375449
312	370965
316	366609
320	362369
324	358179
328	354149
332	350213
336	346290
340	342524
344	339244
348	335559
352	332027
356	328502
360	324697
364	321396
368	318073
372	315286
376	312123
380	308651
384	305657
388	302719
392	300121
396	296961
400	294140
404	291707
408	288638
412	286076
416	283756
420	280944
424	278409
428	276012
432	273548
436	271198
440	268858
444	266802
448	264349
452	262355
456	259928
460	257763
464	255713
468	253556
472	251542
476	249541
480	247356
484	245664
488	243680
492	241597
496	239978
500	238109
504	236376
508	234565
512	232851
516	231085
520	229229
524	227734
528	226098
532	224427
536	222851
540	221305
544	219546
548	218018
552	216502
556	215007
560	213722
564	212266
568	210867
572	209392
576	207854
580	206483
584	205175
588	203801
592	202445
596	201178
600	199913
604	198779
608	197514
612	196267
616	194893
620	193672
624	192649
628	191481
632	190136
636	189003
640	187870
644	186890
648	185617
652	184589
656	183608
660	182360
664	181342
668	180423
672	179220
676	178234
680	177220
684	176219
688	175234
692	174294
696	173307
700	172330
704	171407
708	170447
712	169538
716	168598
720	167698
724	166792
728	165934
732	165027
736	164170
740	163358
744	162496
748	161627
752	160808
756	160000
760	159041
764	158382
768	157593
772	156769
776	155901
780	155225
784	154484
788	153702
792	152950
796	152207
800	151460
804	150648
808	149907
812	149251
816	148500
820	147832
824	147132
828	146414
832	145748
836	145127
840	144374
844	143728
848	143176
852	142516
856	141817
860	141167
864	140465
868	139840
872	139221
876	138679
880	138089
884	137420
888	136833
892	136218
896	135668
900	135057
904	134445
908	133962
912	133329
916	132754
920	132196
924	131687
928	131121
932	130524
936	130013
940	129437
944	128925
948	128420
952	127892
956	127319
960	126800
964	126278
968	125854
972	125282
976	124800
980	124296
984	123770
988	123291
992	122888
996	122369
1000	121890
1004	121396
1008	120902
1012	120484
1016	120008
1020	119529
1024	119107
1028	118652
1032	118226
1036	117746
1040	117325
1044	116832
1048	116446
1052	115991
1056	115603
1060	115127
1064	114724
1068	114345
1072	113859
1076	113471
1080	113089
1084	112698
1088	112235
1092	111792
1096	111423
1100	111038
1104	110671
1108	110289
1112	109883
1116	109476
1120	109040
1124	108677
1128	108284
1132	107968
1136	107591
1140	107177
1144	106863
1148	106547
1152	106155
1156	105756
1160	105431
1164	105054
1168	104668
1172	104414
1176	104014
1180	103638
1184	103274
1188	103017
1192	102628
1196	102266
1200	101986
1204	101635
1208	101263
1212	100991
1216	100639
1220	100367
1224	100009
1228	99731
1232	99334
1236	99089
1240	98733
1244	98484
1248	98112
1252	97859
1256	97491
1260	97228
1264	96867
1268	96600
1272	96346
1276	95981
1280	95731
1284	95459
1288	95111
1292	94847
1296	94597
1300	94324
1304	93940
1308	93697
1312	93427
1316	93180
1320	92827
1324	92569
1328	92302
1332	92070
1336	91812
1340	91545
1344	91196
1348	90984
1352	90724
1356	90455
1360	90194
1364	89929
1368	89666
1372	89415
1376	89162
1380	88930
1384	88651
1388	88409
1392	88161
1396	87932
1400	87672
1404	87405
1408	87176
1412	86910
1416	86670
1420	86454
1424	86158
1428	85945
1432	85771
1436	85508
1440	85260
1444	85024
1448	84782
1452	84537
1456	84367
1460	84065
1464	83888
1468	83646
1472	83397
1476	83240
1480	83018
1484	82770
1488	82552
1492	82374
1496	82122
1500	81882

00:00.0 Host bridge: ServerWorks CNB20LE Host Bridge (rev 06)
00:00.1 Host bridge: ServerWorks CNB20LE Host Bridge (rev 06)
00:02.0 VGA compatible controller: S3 Inc. ViRGE/DX or /GX (rev 01)
00:03.0 Ethernet controller: Digital Equipment Corporation DECchip 21140 [FasterNet] (rev 22)
00:06.0 Ethernet controller: Intel Corp. 82557/8/9 [Ethernet Pro 100] (rev 08)
00:0f.0 ISA bridge: ServerWorks OSB4 South Bridge (rev 50)
00:0f.1 IDE interface: ServerWorks OSB4 IDE Controller
00:0f.2 USB Controller: ServerWorks OSB4/CSB5 OHCI USB Controller (rev 04)
01:01.0 Ethernet controller: Intel Corp. 82543GC Gigabit Ethernet Controller (Copper) (rev 02)
01:02.0 Ethernet controller: Intel Corp. 82543GC Gigabit Ethernet Controller (Copper) (rev 02)
01:03.0 SCSI storage controller: Adaptec AIC-7892P U160/m (rev 02)

processor	: 0
vendor_id	: GenuineIntel
cpu family	: 6
model		: 8
model name	: Pentium III (Coppermine)
stepping	: 6
cpu MHz		: 866.711
cache size	: 256 KB
fdiv_bug	: no
hlt_bug		: no
f00f_bug	: no
coma_bug	: no
fpu		: yes
fpu_exception	: yes
cpuid level	: 2
wp		: yes
flags		: fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 mmx fxsr sse
bogomips	: 1712.12

processor	: 1
vendor_id	: GenuineIntel
cpu family	: 6
model		: 8
model name	: Pentium III (Coppermine)
stepping	: 6
cpu MHz		: 866.711
cache size	: 256 KB
fdiv_bug	: no
hlt_bug		: no
f00f_bug	: no
coma_bug	: no
fpu		: yes
fpu_exception	: yes
cpuid level	: 2
wp		: yes
flags		: fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 mmx fxsr sse
bogomips	: 1728.51


Linux version 2.6.9-rc2-bk11 (root@Sourcemage) (gcc version 3.3.2) #232 SMP Wed Oct 20 18:31:15 CEST 2004

Re: pktgen

[Lennert Buytenhek]

On Sat, Nov 27, 2004 at 01:04:53PM +0100, Robert Olsson wrote:

> 60	825789
> 64	748975
> 68	729149
> 72	719721
> 76	720204
> 80	720127
> 84	702722
> 88	702799
> 92	705107
> 96	701711
> 100	703858
> 104	692120
> 108	696139
> 112	708936
> 116	697780
> 120	677887
> 124	678158
> 128	739290
> 132	737070
> 136	736894
> 140	737645
> 144	737816
> 148	682755
> 152	648547
> 156	646464

This part is strange.  In my case the pps rate for 132-byte packets is
100kpps lower than for 60-byte packets, in your case the curve is rather
flat.  Perhaps you're CPU-bound here (or hitting some NIC limit.)

Your data is a bit noisy -- can you rerun the test for the 60-200 byte
packet range but using 10M packets per run instead of 1M?


> 160	679270

>From this point onwards your HW can saturate the pipe, this is the
boring part of the graph.


thanks,
Lennert

Re: pktgen

[Lennert Buytenhek]

On Sat, Nov 27, 2004 at 02:53:54PM +0100, Lennert Buytenhek wrote:

> > 160	679270
> 
> From this point onwards your HW can saturate the pipe, this is the
> boring part of the graph.

Look at it this way.  Assume that the cost of transmitting a single
packet consists of a packet-size-dependent part (call it 'bandwidth')
and a packet-size-independent part (call that one 'latency').

The higher the latter part is, the bigger packets you need to saturate
the (GigE) pipe.  

Your 64/133 setup saturates GigE with 160B packets, my 32/66 setup needs
350B packets even though there is ample bandwidth in both cases.

Hope I'm making some sense here.


--L

Re: pktgen

[jamal]

On Sat, 2004-11-27 at 09:39, Lennert Buytenhek wrote:

> Look at it this way.  Assume that the cost of transmitting a single
> packet consists of a packet-size-dependent part (call it 'bandwidth')
> and a packet-size-independent part (call that one 'latency').
> 
> The higher the latter part is, the bigger packets you need to saturate
> the (GigE) pipe.  
> 
> Your 64/133 setup saturates GigE with 160B packets, my 32/66 setup needs
> 350B packets even though there is ample bandwidth in both cases.
> 
> Hope I'm making some sense here.

Yes, you are.

Note the constant part of the equation though is not exactly "constant"
even if uyou picked constant hardware. It is per machine (chipset,
topology layout of the bus), per machine setup (how much latency does
your RAM have) and worse: load dependent (two IO endpoints contending
for a PCI-X bridge or the CPU being very busy at the moment with a lot
compute vs RAM-bound execution).
It would be interesting to see a study in this area though.

cheers,
jamal

Re: pktgen

[Lennert Buytenhek]

On Sat, Nov 27, 2004 at 10:04:22AM -0500, jamal wrote:

> Note the constant part of the equation though is not exactly "constant"
> even if uyou picked constant hardware. It is per machine (chipset,
> topology layout of the bus), per machine setup (how much latency does
> your RAM have) and worse: load dependent (two IO endpoints contending
> for a PCI-X bridge or the CPU being very busy at the moment with a lot
> compute vs RAM-bound execution).

Yup.


> It would be interesting to see a study in this area though.

Indeed.  Right now it feels like I'm just poking around in the dark.  I'm
really interested by now in finding out exactly what part of packet TX is
taking how long and where all my cycles are going.

I don't have an Itanic but it's still possible to instrument the driver
and do some stuff Grant talks about in his OLS paper, something like the
attached.  (Exports # of MMIO reads/writes/flushes in the RX frame/
TX carrier/collision stats field.  Beware, flushes are double-counted
as reads.  Produces lots of output.)

During a 10Mpkt pktgen session (~16 seconds), I'm seeing:
- 131757 interrupts, ~8k ints/sec, ~76 pkts/int
- 131789 pure MMIO reads (i.e. not counting MMIO reads intended as write
	flushes), which is E1000_READ_REG(icr) in the irq handler
- 10263536 MMIO writes (which would be 1 per packet plus 2 per interrupt)
- 131757 MMIO write flushes (readl() of the e1000 status register after
	re-enabling IRQs in dev->poll())

Pretty consistent with what Grant was seeing.

MMIO reads from the e1000 are somewhere between 2000 and 3000 cycles a
pop on my hardware.  2400MHz CPU -> ~1us/each.  (Reading netdevice stats
does ~50 of those in a row.)


cheers,
Lennert



diff -urN e1000.orig/e1000_hw.h e1000/e1000_hw.h
--- e1000.orig/e1000_hw.h	2004-11-24 15:35:24.000000000 +0100
+++ e1000/e1000_hw.h	2004-11-28 14:27:25.953933398 +0100
@@ -1038,6 +1038,9 @@
     boolean_t adaptive_ifs;
     boolean_t ifs_params_forced;
     boolean_t in_ifs_mode;
+    uint32_t mmio_reads;
+    uint32_t mmio_writes;
+    uint32_t mmio_write_flushes;
 };
 
 
diff -urN e1000.orig/e1000_main.c e1000/e1000_main.c
--- e1000.orig/e1000_main.c	2004-11-24 15:35:23.000000000 +0100
+++ e1000/e1000_main.c	2004-11-28 15:52:13.127944083 +0100
@@ -491,7 +491,7 @@
 	}
 
 #ifdef NETIF_F_TSO
-	/* Disbaled for now until root-cause is found for
+	/* Disabled for now until root cause is found for
 	 * hangs reported against non-IA archs.  TSO can be
 	 * enabled using ethtool -K eth<x> tso on */
 	if((adapter->hw.mac_type >= e1000_82544) &&
@@ -585,6 +585,21 @@
 	if(eeprom_data & E1000_EEPROM_APME)
 		adapter->wol |= E1000_WUFC_MAG;
 
+	/* print bus type/speed/width info */
+	printk(KERN_INFO "%s: e1000 (PCI%s:%s:%s) ", netdev->name,
+		((adapter->hw.bus_type == e1000_bus_type_pcix) ? "X" : ""),
+		((adapter->hw.bus_speed == e1000_bus_speed_133) ? "133MHz" :
+		 (adapter->hw.bus_speed == e1000_bus_speed_120) ? "120MHz" :
+		 (adapter->hw.bus_speed == e1000_bus_speed_100) ? "100MHz" :
+		 (adapter->hw.bus_speed == e1000_bus_speed_66) ? "66MHz" :
+		  "33MHz"),
+		((adapter->hw.bus_width == e1000_bus_width_64) ? "64-bit" :
+		  "32-bit"));
+
+	for (i = 0; i < 6; i++)
+		printk("%2.2x%c", netdev->dev_addr[i],
+			i == 5 ? '\n' : ':');
+
 	/* reset the hardware with the new settings */
 	e1000_reset(adapter);
 
@@ -1971,6 +1986,7 @@
 	 * be written while holding adapter->stats_lock
 	 */
 
+#if 0
 	adapter->stats.crcerrs += E1000_READ_REG(hw, CRCERRS);
 	adapter->stats.gprc += E1000_READ_REG(hw, GPRC);
 	adapter->stats.gorcl += E1000_READ_REG(hw, GORCL);
@@ -2035,6 +2051,7 @@
 		adapter->stats.tsctc += E1000_READ_REG(hw, TSCTC);
 		adapter->stats.tsctfc += E1000_READ_REG(hw, TSCTFC);
 	}
+#endif
 
 	/* Fill out the OS statistics structure */
 
@@ -2043,7 +2060,7 @@
 	adapter->net_stats.rx_bytes = adapter->stats.gorcl;
 	adapter->net_stats.tx_bytes = adapter->stats.gotcl;
 	adapter->net_stats.multicast = adapter->stats.mprc;
-	adapter->net_stats.collisions = adapter->stats.colc;
+	adapter->net_stats.collisions = hw->mmio_write_flushes;
 
 	/* Rx Errors */
 
@@ -2054,7 +2071,7 @@
 	adapter->net_stats.rx_dropped = adapter->stats.rnbc;
 	adapter->net_stats.rx_length_errors = adapter->stats.rlec;
 	adapter->net_stats.rx_crc_errors = adapter->stats.crcerrs;
-	adapter->net_stats.rx_frame_errors = adapter->stats.algnerrc;
+	adapter->net_stats.rx_frame_errors = hw->mmio_reads;
 	adapter->net_stats.rx_fifo_errors = adapter->stats.mpc;
 	adapter->net_stats.rx_missed_errors = adapter->stats.mpc;
 
@@ -2064,12 +2081,13 @@
 	                               adapter->stats.latecol;
 	adapter->net_stats.tx_aborted_errors = adapter->stats.ecol;
 	adapter->net_stats.tx_window_errors = adapter->stats.latecol;
-	adapter->net_stats.tx_carrier_errors = adapter->stats.tncrs;
+	adapter->net_stats.tx_carrier_errors = hw->mmio_writes;
 
 	/* Tx Dropped needs to be maintained elsewhere */
 
 	/* Phy Stats */
 
+#if 0
 	if(hw->media_type == e1000_media_type_copper) {
 		if((adapter->link_speed == SPEED_1000) &&
 		   (!e1000_read_phy_reg(hw, PHY_1000T_STATUS, &phy_tmp))) {
@@ -2082,6 +2100,7 @@
 		   !e1000_read_phy_reg(hw, M88E1000_RX_ERR_CNTR, &phy_tmp))
 			adapter->phy_stats.receive_errors += phy_tmp;
 	}
+#endif
 
 	spin_unlock_irqrestore(&adapter->stats_lock, flags);
 }
diff -urN e1000.orig/e1000_osdep.h e1000/e1000_osdep.h
--- e1000.orig/e1000_osdep.h	2004-11-24 15:35:23.000000000 +0100
+++ e1000/e1000_osdep.h	2004-11-28 16:05:50.063341317 +0100
@@ -78,23 +78,40 @@
 
 
 #define E1000_WRITE_REG(a, reg, value) ( \
+    printk(KERN_INFO "e1000: MMIO write\n"), \
+    (a)->mmio_writes++, \
     writel((value), ((a)->hw_addr + \
         (((a)->mac_type >= e1000_82543) ? E1000_##reg : E1000_82542_##reg))))
 
-#define E1000_READ_REG(a, reg) ( \
-    readl((a)->hw_addr + \
-        (((a)->mac_type >= e1000_82543) ? E1000_##reg : E1000_82542_##reg)))
+#define E1000_READ_REG(a, reg) ({ \
+    unsigned long s, e, d, v; \
+\
+    (a)->mmio_reads++; \
+    rdtsc(s, d); \
+    v = readl((a)->hw_addr + \
+        (((a)->mac_type >= e1000_82543) ? E1000_##reg : E1000_82542_##reg)); \
+    rdtsc(e, d); \
+    e -= s; \
+    printk(KERN_INFO "e1000: MMIO read took %ld clocks\n", e); \
+    printk(KERN_INFO "e1000: in process %d(%s)\n", current->pid, current->comm); \
+    dump_stack(); \
+    v; \
+})
 
 #define E1000_WRITE_REG_ARRAY(a, reg, offset, value) ( \
+    (a)->mmio_writes++, \
     writel((value), ((a)->hw_addr + \
         (((a)->mac_type >= e1000_82543) ? E1000_##reg : E1000_82542_##reg) + \
         ((offset) << 2))))
 
 #define E1000_READ_REG_ARRAY(a, reg, offset) ( \
+    (a)->mmio_reads++, \
     readl((a)->hw_addr + \
         (((a)->mac_type >= e1000_82543) ? E1000_##reg : E1000_82542_##reg) + \
         ((offset) << 2)))
 
-#define E1000_WRITE_FLUSH(a) E1000_READ_REG(a, STATUS)
+#define E1000_WRITE_FLUSH(a) ( \
+    (a)->mmio_write_flushes++, \
+    E1000_READ_REG(a, STATUS))
 
 #endif /* _E1000_OSDEP_H_ */

Re: pktgen

[jamal]

On Sun, 2004-11-28 at 13:31, Lennert Buytenhek wrote:
> On Sat, Nov 27, 2004 at 10:04:22AM -0500, jamal wrote:

> 
> > It would be interesting to see a study in this area though.
> 
> Indeed.  Right now it feels like I'm just poking around in the dark.  I'm
> really interested by now in finding out exactly what part of packet TX is
> taking how long and where all my cycles are going.
> 
> I don't have an Itanic but it's still possible to instrument the driver
> and do some stuff Grant talks about in his OLS paper, something like the
> attached.  (Exports # of MMIO reads/writes/flushes in the RX frame/
> TX carrier/collision stats field.  Beware, flushes are double-counted
> as reads.  Produces lots of output.)
> 
> During a 10Mpkt pktgen session (~16 seconds), I'm seeing:
> - 131757 interrupts, ~8k ints/sec, ~76 pkts/int
> - 131789 pure MMIO reads (i.e. not counting MMIO reads intended as write
> 	flushes), which is E1000_READ_REG(icr) in the irq handler
> - 10263536 MMIO writes (which would be 1 per packet plus 2 per interrupt)
> - 131757 MMIO write flushes (readl() of the e1000 status register after
> 	re-enabling IRQs in dev->poll())
> 
> Pretty consistent with what Grant was seeing.
> 
> MMIO reads from the e1000 are somewhere between 2000 and 3000 cycles a
> pop on my hardware.  2400MHz CPU -> ~1us/each.  (Reading netdevice stats
> does ~50 of those in a row.)
> 

Reads are known to be expensive. Good to see how much they are reduced. 
Not sure if this applies to MMIO reads though. Grant?

cheers,
jamal

Re: pktgen

[Robert Olsson]

Lennert Buytenhek writes:
 > Your data is a bit noisy -- can you rerun the test for the 60-200 byte
 > packet range but using 10M packets per run instead of 1M?

 OK!

60	825976
64	746730
68	730215
72	720942
76	721090
80	721772
84	704472
88	704489
92	705184
96	714750
100	694178
104	700853
108	703332
112	715865
116	692223
120	677893
124	678306
128	736908
132	712328
136	722271
140	706784
144	716770
148	694497
152	647868
156	647693
160	678071
164	663694
168	649894
172	636478
176	623761
180	611580
184	599835
188	588540
192	577549
196	567044
200	556967

						--ro

Re: pktgen

[Grant Grundler]

On Mon, Nov 29, 2004 at 08:49:52AM -0500, jamal wrote:
> On Sun, 2004-11-28 at 13:31, Lennert Buytenhek wrote:
> > Indeed.  Right now it feels like I'm just poking around in the dark.  I'm
> > really interested by now in finding out exactly what part of packet TX is
> > taking how long and where all my cycles are going.

ia64 PMU can measure exactly where/why the CPU is stalling.
MMIO reads are by far the worst offenders - but not the only ones.
"bubbles" in the pipeline can be caused by lots of other
stalls and will affect CPU utilization as well.
A very nice description of CPU stalls caused by memory subsystem is here:

	http://www.gelato.org/pdf/mysql_itanium2_perf.pdf

Gelato.org, sgi.com, intel.com, hp.com have more white papers
on ia64 performance tools and tuning.

> > I don't have an Itanic but it's still possible to instrument the driver
> > and do some stuff Grant talks about in his OLS paper, something like the
> > attached.  (Exports # of MMIO reads/writes/flushes in the RX frame/
> > TX carrier/collision stats field.  Beware, flushes are double-counted
> > as reads.  Produces lots of output.)

I'd be happy to give you access to an IA64 machine to poke at.
If you can send me:
o preferred login
o public ssh key
o work telephone #

BTW, Jamal, I'm expecting we'll be able to get Robur an RX2600
to play with this quarter. I need to ask about that again.

> > During a 10Mpkt pktgen session (~16 seconds), I'm seeing:
> > - 131757 interrupts, ~8k ints/sec, ~76 pkts/int
> > - 131789 pure MMIO reads (i.e. not counting MMIO reads intended as write
> > 	flushes), which is E1000_READ_REG(icr) in the irq handler
> > - 10263536 MMIO writes (which would be 1 per packet plus 2 per interrupt)
> > - 131757 MMIO write flushes (readl() of the e1000 status register after
> > 	re-enabling IRQs in dev->poll())
> > 
> > Pretty consistent with what Grant was seeing.

yup.

> > 
> > MMIO reads from the e1000 are somewhere between 2000 and 3000 cycles a
> > pop on my hardware.  2400MHz CPU -> ~1us/each.  (Reading netdevice stats
> > does ~50 of those in a row.)
> > 
> 
> Reads are known to be expensive. Good to see how much they are reduced. 
> Not sure if this applies to MMIO reads though. Grant?

I don't differentiate between "pure" MMIO reads and posted MMIO write
flushes. They cost the same AFAIK. If one can tweak the algorithm so
either is not needed, it's a win.

But I didn't see any opportunity to do that in e1000 driver.
There is such an opportunity in tg3 though. I just won't have
a chance to pursue it. :^(

The absolute cost in CPU cycles of an MMIO read will depend on chipset,
CPU speed, and number of bridges the transaction has to cross.
On an idle 1Ghz system, I've measured ~1000-1200 cycles.
When measured in time (not CPU cycles), the cost hasn't changed that
much in the past 6-8 years (mostly 66Mhz PCI busses).
Adding or removing a PCI-PCI bridge is the biggest variable in
absolute time.

thanks,
grant

pktgen (was Re: tests of kernel modules)

[Alexey Dobriyan]

[CCing netdev, bug in pktgen]

	[build modular pktgen]
	while true; do modprobe pktgen && rmmod pktgen; done

	BUG: warning at fs/proc/generic.c:732/remove_proc_entry()
	 [<c016a7ad>] remove_proc_entry+0x161/0x1ca
	 [<e19969ab>] pg_cleanup+0xd5/0xdc [pktgen]
         [<c011fa3e>] autoremove_wake_function+0x0/0x35
	 [<c01280bd>] sys_delete_module+0x162/0x189
	 [<c0136500>] remove_vma+0x31/0x36
	 [<c0136df7>] do_munmap+0x193/0x1ac
	 [<c0102829>] sysenter_past_esp+0x56/0x79
	 [<c02d007b>] fn_hash_delete+0x4f/0x1c7

On Tue, Nov 21, 2006 at 09:36:46PM +0100, Pavol Gono wrote:
> I am going to add two more:
> for i in 1 2 3 4 5 ; do modprobe pktgen ; rmmod pktgen ; done

Looks like it creates /proc/net/pktgen/kpktgen_%i but forgets to remove
them.

Re: pktgen

[David Miller]

From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Wed, 22 Nov 2006 00:22:51 +0300

> [CCing netdev, bug in pktgen]
> 
> 	[build modular pktgen]
> 	while true; do modprobe pktgen && rmmod pktgen; done
> 
> 	BUG: warning at fs/proc/generic.c:732/remove_proc_entry()
> 	 [<c016a7ad>] remove_proc_entry+0x161/0x1ca
> 	 [<e19969ab>] pg_cleanup+0xd5/0xdc [pktgen]
>          [<c011fa3e>] autoremove_wake_function+0x0/0x35
> 	 [<c01280bd>] sys_delete_module+0x162/0x189
> 	 [<c0136500>] remove_vma+0x31/0x36
> 	 [<c0136df7>] do_munmap+0x193/0x1ac
> 	 [<c0102829>] sysenter_past_esp+0x56/0x79
> 	 [<c02d007b>] fn_hash_delete+0x4f/0x1c7
> 
> On Tue, Nov 21, 2006 at 09:36:46PM +0100, Pavol Gono wrote:
> > I am going to add two more:
> > for i in 1 2 3 4 5 ; do modprobe pktgen ; rmmod pktgen ; done
> 
> Looks like it creates /proc/net/pktgen/kpktgen_%i but forgets to remove
> them.

It's pretty careful to delete all of the entries under
/proc/net/pktgen/.

When the module is brought down, it walks the list of threads
and brings them down by setting T_TERMINATE in t->control.
This makes the thread break out of it's loop and run:

	pktgen_stop(t);
	pktgen_rem_all_ifs(t);
	pktgen_rem_thread(t);

pktgen_rem_all_ifs() will delete all device entries in
/proc/net/pktgen/

Next, pktgen_rem_thread() will kill off the entry for
/proc/net/pktgen/kpkgen_%i

Finally, the top-level module remove code will delete the
control file right before it tries to kill off the directory
via:

	/* Clean up proc file system */
	remove_proc_entry(PGCTRL, pg_proc_dir);
	proc_net_remove(PG_PROC_DIR);

So I don't see any bugs that could cause this.

One possibility is that the thread's t->name is being
corrupted somehow, that would make the remove_proc_entry()
fail and cause the behavior you see.  But I can't see anything
obvious that might do that either.

Re: pktgen

[Alexey Dobriyan]

On Tue, Nov 28, 2006 at 03:33:25PM -0800, David Miller wrote:
> From: Alexey Dobriyan <adobriyan@gmail.com>
> Date: Wed, 22 Nov 2006 00:22:51 +0300
> 
> > [CCing netdev, bug in pktgen]
> > 
> > 	[build modular pktgen]
> > 	while true; do modprobe pktgen && rmmod pktgen; done
> > 
> > 	BUG: warning at fs/proc/generic.c:732/remove_proc_entry()
> > 	 [<c016a7ad>] remove_proc_entry+0x161/0x1ca
> > 	 [<e19969ab>] pg_cleanup+0xd5/0xdc [pktgen]
> >          [<c011fa3e>] autoremove_wake_function+0x0/0x35
> > 	 [<c01280bd>] sys_delete_module+0x162/0x189
> > 	 [<c0136500>] remove_vma+0x31/0x36
> > 	 [<c0136df7>] do_munmap+0x193/0x1ac
> > 	 [<c0102829>] sysenter_past_esp+0x56/0x79
> > 	 [<c02d007b>] fn_hash_delete+0x4f/0x1c7
> > 
> > On Tue, Nov 21, 2006 at 09:36:46PM +0100, Pavol Gono wrote:
> > > I am going to add two more:
> > > for i in 1 2 3 4 5 ; do modprobe pktgen ; rmmod pktgen ; done
> > 
> > Looks like it creates /proc/net/pktgen/kpktgen_%i but forgets to remove
> > them.
>
> It's pretty careful to delete all of the entries under
> /proc/net/pktgen/.
>
> When the module is brought down, it walks the list of threads
> and brings them down by setting T_TERMINATE in t->control.

Looks like worker thread strategically clears it if scheduled at wrong
moment.

--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -3292,7 +3292,6 @@ static void pktgen_thread_worker(struct
 
 	init_waitqueue_head(&t->queue);
 
-	t->control &= ~(T_TERMINATE);
 	t->control &= ~(T_RUN);
 	t->control &= ~(T_STOP);
 	t->control &= ~(T_REMDEVALL);

> This makes the thread break out of it's loop and run:
>
> 	pktgen_stop(t);
> 	pktgen_rem_all_ifs(t);
> 	pktgen_rem_thread(t);

Kernel seeems to survive, but when I hit Ctrl+C after half
a minute backtrace is back being the very last dmesg lines.

Re: pktgen

[David Miller]

From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Wed, 29 Nov 2006 23:04:37 +0300

> Looks like worker thread strategically clears it if scheduled at wrong
> moment.
> 
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -3292,7 +3292,6 @@ static void pktgen_thread_worker(struct
>  
>  	init_waitqueue_head(&t->queue);
>  
> -	t->control &= ~(T_TERMINATE);
>  	t->control &= ~(T_RUN);
>  	t->control &= ~(T_STOP);
>  	t->control &= ~(T_REMDEVALL);

Good catch Alexey.  Did you rerun the load/unload test with
this fix applied?  If it fixes things, I'll merge it.

Thanks!

Re: pktgen

[Alexey Dobriyan]

On 11/30/06, David Miller <davem@davemloft.net> wrote:
> From: Alexey Dobriyan <adobriyan@gmail.com>
> Date: Wed, 29 Nov 2006 23:04:37 +0300
>
> > Looks like worker thread strategically clears it if scheduled at wrong
> > moment.
> >
> > --- a/net/core/pktgen.c
> > +++ b/net/core/pktgen.c
> > @@ -3292,7 +3292,6 @@ static void pktgen_thread_worker(struct
> >
> >  	init_waitqueue_head(&t->queue);
> >
> > -	t->control &= ~(T_TERMINATE);
> >  	t->control &= ~(T_RUN);
> >  	t->control &= ~(T_STOP);
> >  	t->control &= ~(T_REMDEVALL);
>
> Good catch Alexey.  Did you rerun the load/unload test with
> this fix applied?  If it fixes things, I'll merge it.

Well, yes, it fixes things, except Ctrl+C getting you out of
modprobe/rmmod loop will spit
backtrace again. And other flags: T_RUN, T_STOP. Clearance is not
needed due to kZalloc and
create bugs as demostrated.

Give me some time.

Re: pktgen

[Robert Olsson]

Hello!

Seems you found a race when rmmod is done before it's fully started

Try:

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 733d86d..ac0b4b1 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -160,7 +160,7 @@
 #include <asm/div64.h>		/* do_div */
 #include <asm/timex.h>
 
-#define VERSION  "pktgen v2.68: Packet Generator for packet performance testing.\n"
+#define VERSION  "pktgen v2.69: Packet Generator for packet performance testing.\n"
 
 /* #define PG_DEBUG(a) a */
 #define PG_DEBUG(a)
@@ -3673,6 +3673,8 @@ static void __exit pg_cleanup(void)
 	struct list_head *q, *n;
 	wait_queue_head_t queue;
 	init_waitqueue_head(&queue);
+	
+	schedule_timeout_interruptible(msecs_to_jiffies(125));
 
 	/* Stop all interfaces & threads */
 


for i in 1 2 3 4 5 ; do modprobe pktgen ; rmmod pktgen ; done

In dmesg
pktgen v2.69: Packet Generator for packet performance testing.
pktgen v2.69: Packet Generator for packet performance testing.
pktgen v2.69: Packet Generator for packet performance testing.
pktgen v2.69: Packet Generator for packet performance testing.
pktgen v2.69: Packet Generator for packet performance testing.

Cheers.
					--ro



Alexey Dobriyan writes:
 > On 11/30/06, David Miller <davem@davemloft.net> wrote:
 > > From: Alexey Dobriyan <adobriyan@gmail.com>
 > > Date: Wed, 29 Nov 2006 23:04:37 +0300
 > >
 > > > Looks like worker thread strategically clears it if scheduled at wrong
 > > > moment.
 > > >
 > > > --- a/net/core/pktgen.c
 > > > +++ b/net/core/pktgen.c
 > > > @@ -3292,7 +3292,6 @@ static void pktgen_thread_worker(struct
 > > >
 > > >  	init_waitqueue_head(&t->queue);
 > > >
 > > > -	t->control &= ~(T_TERMINATE);
 > > >  	t->control &= ~(T_RUN);
 > > >  	t->control &= ~(T_STOP);
 > > >  	t->control &= ~(T_REMDEVALL);
 > >
 > > Good catch Alexey.  Did you rerun the load/unload test with
 > > this fix applied?  If it fixes things, I'll merge it.
 > 
 > Well, yes, it fixes things, except Ctrl+C getting you out of
 > modprobe/rmmod loop will spit
 > backtrace again. And other flags: T_RUN, T_STOP. Clearance is not
 > needed due to kZalloc and
 > create bugs as demostrated.
 > 
 > Give me some time.
 > -
 > To unsubscribe from this list: send the line "unsubscribe netdev" in
 > the body of a message to majordomo@vger.kernel.org
 > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: pktgen

[Ben Greear]

Robert Olsson wrote:
> Hello!
>
> Seems you found a race when rmmod is done before it's fully started
>
> Try:
>
> diff --git a/net/core/pktgen.c b/net/core/pktgen.c
> index 733d86d..ac0b4b1 100644
> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -160,7 +160,7 @@
>  #include <asm/div64.h>		/* do_div */
>  #include <asm/timex.h>
>  
> -#define VERSION  "pktgen v2.68: Packet Generator for packet performance testing.\n"
> +#define VERSION  "pktgen v2.69: Packet Generator for packet performance testing.\n"
>  
>  /* #define PG_DEBUG(a) a */
>  #define PG_DEBUG(a)
> @@ -3673,6 +3673,8 @@ static void __exit pg_cleanup(void)
>  	struct list_head *q, *n;
>  	wait_queue_head_t queue;
>  	init_waitqueue_head(&queue);
> +	
> +	schedule_timeout_interruptible(msecs_to_jiffies(125));
>  
>  	/* Stop all interfaces & threads */
>  
>   
That strikes me as a hack..surely there is a better method than just adding
a sleep??

Ben

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

Re: pktgen

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Thu, 30 Nov 2006 09:33:43 -0800

> Robert Olsson wrote:
> > @@ -3673,6 +3673,8 @@ static void __exit pg_cleanup(void)
> >  	struct list_head *q, *n;
> >  	wait_queue_head_t queue;
> >  	init_waitqueue_head(&queue);
> > +	
> > +	schedule_timeout_interruptible(msecs_to_jiffies(125));
> >  
> >  	/* Stop all interfaces & threads */
> >  
>
> That strikes me as a hack..surely there is a better method than just adding
> a sleep??

Agreed.

Robert, please fix this by using a completion so that we can
wait for the threads to start up, something like this:

...
#include <linux/completion.h>
...

struct pktgen_thread_info {
       struct pktgen_thread *t;
       struct completion *c;
};

static void pktgen_thread_worker(struct pktgen_thread_info *info)
{
	struct pktgen_thread *t = info->t;
 ...
	__set_current_state(TASK_INTERRUPTIBLE);
	mb();

	complete(info->c);
 ...
}

static int __init pktgen_create_thread(const char *name, int cpu)
{
 ...
	struct pktgen_thread_info info;
	struct complation started;
 ...
        init_completion(&started);

	info.t = t;
	info.c = &started;

	err = kernel_thread((void *)pktgen_thread_worker, (void *)t,
			  CLONE_FS | CLONE_FILES | CLONE_SIGHAND);
	if (err < 0) {
		printk("pktgen: kernel_thread() failed for cpu %d\n", t->cpu);
		remove_proc_entry(t->name, pg_proc_dir);
		list_del(&t->th_list);
		kfree(t);
		return err;
	}

	wait_for_completion(&started);
	return 0;
}

We can horse around with fixing the initial t->control bit flipping
in pktgen_thread_worker() in a future changeset if we want.

Re: pktgen

[Robert Olsson]

David Miller writes:

 > Agreed.
 > 
 > Robert, please fix this by using a completion so that we can
 > wait for the threads to start up, something like this:

Included. It passes my test but Alexey and others test.
Cheers.
				--ro

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 733d86d..a630a73 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -147,6 +147,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/wait.h>
+#include <linux/completion.h>
 #include <linux/etherdevice.h>
 #include <net/checksum.h>
 #include <net/ipv6.h>
@@ -160,7 +161,7 @@
 #include <asm/div64.h>		/* do_div */
 #include <asm/timex.h>
 
-#define VERSION  "pktgen v2.68: Packet Generator for packet performance testing.\n"
+#define VERSION  "pktgen v2.69: Packet Generator for packet performance testing.\n"
 
 /* #define PG_DEBUG(a) a */
 #define PG_DEBUG(a)
@@ -206,6 +207,11 @@ static struct proc_dir_entry *pg_proc_di
 #define VLAN_TAG_SIZE(x) ((x)->vlan_id == 0xffff ? 0 : 4)
 #define SVLAN_TAG_SIZE(x) ((x)->svlan_id == 0xffff ? 0 : 4)
 
+struct pktgen_thread_info {
+       struct pktgen_thread *t;
+       struct completion *c;
+};
+
 struct flow_state {
 	__u32 cur_daddr;
 	int count;
@@ -3264,10 +3270,11 @@ out:;
  * Main loop of the thread goes here
  */
 
-static void pktgen_thread_worker(struct pktgen_thread *t)
+static void pktgen_thread_worker(struct pktgen_thread_info *info)
 {
 	DEFINE_WAIT(wait);
 	struct pktgen_dev *pkt_dev = NULL;
+	struct pktgen_thread *t = info->t;
 	int cpu = t->cpu;
 	sigset_t tmpsig;
 	u32 max_before_softirq;
@@ -3307,6 +3314,8 @@ static void pktgen_thread_worker(struct 
 	__set_current_state(TASK_INTERRUPTIBLE);
 	mb();
 
+        complete(info->c);
+
 	while (1) {
 
 		__set_current_state(TASK_RUNNING);
@@ -3518,6 +3527,8 @@ static struct pktgen_thread *__init pktg
 static int __init pktgen_create_thread(const char *name, int cpu)
 {
 	int err;
+	struct pktgen_thread_info info;
+        struct completion started;
 	struct pktgen_thread *t = NULL;
 	struct proc_dir_entry *pe;
 
@@ -3558,7 +3569,11 @@ static int __init pktgen_create_thread(c
 
 	t->removed = 0;
 
-	err = kernel_thread((void *)pktgen_thread_worker, (void *)t,
+	init_completion(&started);
+        info.t = t;
+        info.c = &started;
+
+	err = kernel_thread((void *)pktgen_thread_worker, (void *)&info,
 			  CLONE_FS | CLONE_FILES | CLONE_SIGHAND);
 	if (err < 0) {
 		printk("pktgen: kernel_thread() failed for cpu %d\n", t->cpu);
@@ -3568,6 +3583,7 @@ static int __init pktgen_create_thread(c
 		return err;
 	}
 
+	wait_for_completion(&started);
 	return 0;
 }
 

Re: pktgen

[Christoph Hellwig]

On Thu, Nov 30, 2006 at 08:14:23PM -0800, David Miller wrote:
> Agreed.
> 
> Robert, please fix this by using a completion so that we can
> wait for the threads to start up, something like this:

No, that's wrong aswell :)  Please use the kthread_ API that takes
care of all this.  kernel_thread is going away mid-term, so you'd
have to do this work anyway.

Re: pktgen

[Alexey Dobriyan]

On 12/1/06, Robert Olsson <Robert.Olsson@data.slu.se> wrote:
> David Miller writes:
>  > Agreed.
>  >
>  > Robert, please fix this by using a completion so that we can
>  > wait for the threads to start up, something like this:
>
> Included. It passes my test but Alexey and others test.

Confused now. Is my "t->control &= ~(T_TERMINATE);" fix deprecated by
completions?

> --- a/net/core/pktgen.c
> +++ b/net/core/pktgen.c
> @@ -147,6 +147,7 @@
>  #include <linux/proc_fs.h>
>  #include <linux/seq_file.h>
>  #include <linux/wait.h>
> +#include <linux/completion.h>
>  #include <linux/etherdevice.h>
>  #include <net/checksum.h>
>  #include <net/ipv6.h>
> @@ -160,7 +161,7 @@
>  #include <asm/div64.h>		/* do_div */
>  #include <asm/timex.h>
>
> -#define VERSION  "pktgen v2.68: Packet Generator for packet performance
> testing.\n"
> +#define VERSION  "pktgen v2.69: Packet Generator for packet performance
> testing.\n"
>
>  /* #define PG_DEBUG(a) a */
>  #define PG_DEBUG(a)
> @@ -206,6 +207,11 @@ static struct proc_dir_entry *pg_proc_di
>  #define VLAN_TAG_SIZE(x) ((x)->vlan_id == 0xffff ? 0 : 4)
>  #define SVLAN_TAG_SIZE(x) ((x)->svlan_id == 0xffff ? 0 : 4)
>
> +struct pktgen_thread_info {
> +       struct pktgen_thread *t;
> +       struct completion *c;
> +};
> +
>  struct flow_state {
>  	__u32 cur_daddr;
>  	int count;
> @@ -3264,10 +3270,11 @@ out:;
>   * Main loop of the thread goes here
>   */
>
> -static void pktgen_thread_worker(struct pktgen_thread *t)
> +static void pktgen_thread_worker(struct pktgen_thread_info *info)
>  {
>  	DEFINE_WAIT(wait);
>  	struct pktgen_dev *pkt_dev = NULL;
> +	struct pktgen_thread *t = info->t;
>  	int cpu = t->cpu;
>  	sigset_t tmpsig;
>  	u32 max_before_softirq;
> @@ -3307,6 +3314,8 @@ static void pktgen_thread_worker(struct
>  	__set_current_state(TASK_INTERRUPTIBLE);
>  	mb();
>
> +        complete(info->c);
> +
>  	while (1) {
>
>  		__set_current_state(TASK_RUNNING);
> @@ -3518,6 +3527,8 @@ static struct pktgen_thread *__init pktg
>  static int __init pktgen_create_thread(const char *name, int cpu)
>  {
>  	int err;
> +	struct pktgen_thread_info info;
> +        struct completion started;
>  	struct pktgen_thread *t = NULL;
>  	struct proc_dir_entry *pe;
>
> @@ -3558,7 +3569,11 @@ static int __init pktgen_create_thread(c
>
>  	t->removed = 0;
>
> -	err = kernel_thread((void *)pktgen_thread_worker, (void *)t,
> +	init_completion(&started);
> +        info.t = t;
> +        info.c = &started;
> +
> +	err = kernel_thread((void *)pktgen_thread_worker, (void *)&info,
>  			  CLONE_FS | CLONE_FILES | CLONE_SIGHAND);
>  	if (err < 0) {
>  		printk("pktgen: kernel_thread() failed for cpu %d\n", t->cpu);
> @@ -3568,6 +3583,7 @@ static int __init pktgen_create_thread(c
>  		return err;
>  	}
>
> +	wait_for_completion(&started);
>  	return 0;
>  }

Re: pktgen

[Robert Olsson]

Alexey Dobriyan writes:

 > 
 > Confused now. Is my "t->control &= ~(T_TERMINATE);" fix deprecated by
 > completions?
 
 It's not needed with completion patch as this does the job a bit more
 mainstream. The T_TERMINATE seems to work well I've tested on machine
 with CPU:s. Only once I noticed that only 2 of 4 threads got started 
 but it could be something else...

 And the race is hardly seen with any real use of pktgen.. .Let's hear
 what others... and completions was out-of-date.

 Cheers.
					--ro
 

Re: pktgen

[David Miller]

From: Christoph Hellwig <hch@infradead.org>
Date: Fri, 1 Dec 2006 08:22:25 +0000

> On Thu, Nov 30, 2006 at 08:14:23PM -0800, David Miller wrote:
> > Agreed.
> > 
> > Robert, please fix this by using a completion so that we can
> > wait for the threads to start up, something like this:
> 
> No, that's wrong aswell :)  Please use the kthread_ API that takes
> care of all this.  kernel_thread is going away mid-term, so you'd
> have to do this work anyway.

I was going to suggest this Christophe, but I wanted a change small
and easy enough to verify in order to merge the fix into the -stable
branch.  Converting the thing over to kthread would make the change
more risky in such a context.

Re: pktgen

[David Miller]

From: "Alexey Dobriyan" <adobriyan@gmail.com>
Date: Fri, 1 Dec 2006 12:51:53 +0300

> On 12/1/06, Robert Olsson <Robert.Olsson@data.slu.se> wrote:
> > David Miller writes:
> >  > Agreed.
> >  >
> >  > Robert, please fix this by using a completion so that we can
> >  > wait for the threads to start up, something like this:
> >
> > Included. It passes my test but Alexey and others test.
> 
> Confused now. Is my "t->control &= ~(T_TERMINATE);" fix deprecated by
> completions?

The completions solve the bug completely.

But later on we should integate a change that eliminates
these spurious t->control bit clears at the start of the
pktgen thread execution, it just isn't needed to fix the
bug so we can make it later.

Re: pktgen

[David Miller]

From: Robert Olsson <Robert.Olsson@data.slu.se>
Date: Fri, 1 Dec 2006 09:14:01 +0100

> 
> David Miller writes:
> 
>  > Agreed.
>  > 
>  > Robert, please fix this by using a completion so that we can
>  > wait for the threads to start up, something like this:
> 
> Included. It passes my test but Alexey and others test.

Ok, I'm going to put Robert's version of the fix into 2.6.19
and previous -stable branches.

But for 2.6.20 I'd like to do the following, based upon
Christoph's suggestion to convert to kthread.

Can folks please give this a spin?  I've tested that the
module loads and unloads properly, the threads startup
and shutdown, but that's it.

Thanks!

commit b3010a665cc33596fbf4be3fc6c3c5c80aeefb65
Author: David S. Miller <davem@sunset.davemloft.net>
Date:   Mon Jan 1 20:51:53 2007 -0800

    [PKTGEN]: Convert to kthread API.
    
    Based upon a suggestion from Christoph Hellwig.
    
    This fixes various races in module load/unload handling
    too.
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/core/pktgen.c b/net/core/pktgen.c
index 1897a3a..04d4b93 100644
--- a/net/core/pktgen.c
+++ b/net/core/pktgen.c
@@ -148,6 +148,7 @@
 #include <linux/seq_file.h>
 #include <linux/wait.h>
 #include <linux/etherdevice.h>
+#include <linux/kthread.h>
 #include <net/checksum.h>
 #include <net/ipv6.h>
 #include <net/addrconf.h>
@@ -360,8 +361,7 @@ struct pktgen_thread {
 	spinlock_t if_lock;
 	struct list_head if_list;	/* All device here */
 	struct list_head th_list;
-	int removed;
-	char name[32];
+	struct task_struct *tsk;
 	char result[512];
 	u32 max_before_softirq;	/* We'll call do_softirq to prevent starvation. */
 
@@ -1689,7 +1689,7 @@ static int pktgen_thread_show(struct seq_file *seq, void *v)
 	BUG_ON(!t);
 
 	seq_printf(seq, "Name: %s  max_before_softirq: %d\n",
-		   t->name, t->max_before_softirq);
+		   t->tsk->comm, t->max_before_softirq);
 
 	seq_printf(seq, "Running: ");
 
@@ -3112,7 +3112,7 @@ static void pktgen_rem_thread(struct pktgen_thread *t)
 {
 	/* Remove from the thread list */
 
-	remove_proc_entry(t->name, pg_proc_dir);
+	remove_proc_entry(t->tsk->comm, pg_proc_dir);
 
 	mutex_lock(&pktgen_thread_lock);
 
@@ -3260,58 +3260,40 @@ out:;
  * Main loop of the thread goes here
  */
 
-static void pktgen_thread_worker(struct pktgen_thread *t)
+static int pktgen_thread_worker(void *arg)
 {
 	DEFINE_WAIT(wait);
+	struct pktgen_thread *t = arg;
 	struct pktgen_dev *pkt_dev = NULL;
 	int cpu = t->cpu;
-	sigset_t tmpsig;
 	u32 max_before_softirq;
 	u32 tx_since_softirq = 0;
 
-	daemonize("pktgen/%d", cpu);
-
-	/* Block all signals except SIGKILL, SIGSTOP and SIGTERM */
-
-	spin_lock_irq(&current->sighand->siglock);
-	tmpsig = current->blocked;
-	siginitsetinv(&current->blocked,
-		      sigmask(SIGKILL) | sigmask(SIGSTOP) | sigmask(SIGTERM));
-
-	recalc_sigpending();
-	spin_unlock_irq(&current->sighand->siglock);
-
-	/* Migrate to the right CPU */
-	set_cpus_allowed(current, cpumask_of_cpu(cpu));
-	if (smp_processor_id() != cpu)
-		BUG();
+	BUG_ON(smp_processor_id() != cpu);
 
 	init_waitqueue_head(&t->queue);
 
-	t->control &= ~(T_TERMINATE);
-	t->control &= ~(T_RUN);
-	t->control &= ~(T_STOP);
-	t->control &= ~(T_REMDEVALL);
-	t->control &= ~(T_REMDEV);
-
 	t->pid = current->pid;
 
 	PG_DEBUG(printk("pktgen: starting pktgen/%d:  pid=%d\n", cpu, current->pid));
 
 	max_before_softirq = t->max_before_softirq;
 
-	__set_current_state(TASK_INTERRUPTIBLE);
-	mb();
+	set_current_state(TASK_INTERRUPTIBLE);
 
-	while (1) {
-
-		__set_current_state(TASK_RUNNING);
+	while (!kthread_should_stop()) {
+		pkt_dev = next_to_run(t);
 
-		/*
-		 * Get next dev to xmit -- if any.
-		 */
+		if (!pkt_dev &&
+		    (t->control & (T_STOP | T_RUN | T_REMDEVALL | T_REMDEV))
+		    == 0) {
+			prepare_to_wait(&(t->queue), &wait,
+					TASK_INTERRUPTIBLE);
+			schedule_timeout(HZ / 10);
+			finish_wait(&(t->queue), &wait);
+		}
 
-		pkt_dev = next_to_run(t);
+		__set_current_state(TASK_RUNNING);
 
 		if (pkt_dev) {
 
@@ -3329,21 +3311,8 @@ static void pktgen_thread_worker(struct pktgen_thread *t)
 					do_softirq();
 				tx_since_softirq = 0;
 			}
-		} else {
-			prepare_to_wait(&(t->queue), &wait, TASK_INTERRUPTIBLE);
-			schedule_timeout(HZ / 10);
-			finish_wait(&(t->queue), &wait);
 		}
 
-		/*
-		 * Back from sleep, either due to the timeout or signal.
-		 * We check if we have any "posted" work for us.
-		 */
-
-		if (t->control & T_TERMINATE || signal_pending(current))
-			/* we received a request to terminate ourself */
-			break;
-
 		if (t->control & T_STOP) {
 			pktgen_stop(t);
 			t->control &= ~(T_STOP);
@@ -3364,20 +3333,19 @@ static void pktgen_thread_worker(struct pktgen_thread *t)
 			t->control &= ~(T_REMDEV);
 		}
 
-		if (need_resched())
-			schedule();
+		set_current_state(TASK_INTERRUPTIBLE);
 	}
 
-	PG_DEBUG(printk("pktgen: %s stopping all device\n", t->name));
+	PG_DEBUG(printk("pktgen: %s stopping all device\n", t->tsk->comm));
 	pktgen_stop(t);
 
-	PG_DEBUG(printk("pktgen: %s removing all device\n", t->name));
+	PG_DEBUG(printk("pktgen: %s removing all device\n", t->tsk->comm));
 	pktgen_rem_all_ifs(t);
 
-	PG_DEBUG(printk("pktgen: %s removing thread.\n", t->name));
+	PG_DEBUG(printk("pktgen: %s removing thread.\n", t->tsk->comm));
 	pktgen_rem_thread(t);
 
-	t->removed = 1;
+	return 0;
 }
 
 static struct pktgen_dev *pktgen_find_dev(struct pktgen_thread *t,
@@ -3495,37 +3463,11 @@ static int pktgen_add_device(struct pktgen_thread *t, const char *ifname)
 	return add_dev_to_thread(t, pkt_dev);
 }
 
-static struct pktgen_thread *__init pktgen_find_thread(const char *name)
+static int __init pktgen_create_thread(int cpu)
 {
 	struct pktgen_thread *t;
-
-	mutex_lock(&pktgen_thread_lock);
-
-	list_for_each_entry(t, &pktgen_threads, th_list)
-		if (strcmp(t->name, name) == 0) {
-			mutex_unlock(&pktgen_thread_lock);
-			return t;
-		}
-
-	mutex_unlock(&pktgen_thread_lock);
-	return NULL;
-}
-
-static int __init pktgen_create_thread(const char *name, int cpu)
-{
-	int err;
-	struct pktgen_thread *t = NULL;
 	struct proc_dir_entry *pe;
-
-	if (strlen(name) > 31) {
-		printk("pktgen: ERROR:  Thread name cannot be more than 31 characters.\n");
-		return -EINVAL;
-	}
-
-	if (pktgen_find_thread(name)) {
-		printk("pktgen: ERROR: thread: %s already exists\n", name);
-		return -EINVAL;
-	}
+	struct task_struct *p;
 
 	t = kzalloc(sizeof(struct pktgen_thread), GFP_KERNEL);
 	if (!t) {
@@ -3533,14 +3475,29 @@ static int __init pktgen_create_thread(const char *name, int cpu)
 		return -ENOMEM;
 	}
 
-	strcpy(t->name, name);
 	spin_lock_init(&t->if_lock);
 	t->cpu = cpu;
 
-	pe = create_proc_entry(t->name, 0600, pg_proc_dir);
+	INIT_LIST_HEAD(&t->if_list);
+
+	list_add_tail(&t->th_list, &pktgen_threads);
+
+	p = kthread_create(pktgen_thread_worker, t, "kpktgend_%d", cpu);
+	if (IS_ERR(p)) {
+		printk("pktgen: kernel_thread() failed for cpu %d\n", t->cpu);
+		list_del(&t->th_list);
+		kfree(t);
+		return PTR_ERR(p);
+	}
+	kthread_bind(p, cpu);
+	t->tsk = p;
+
+	pe = create_proc_entry(t->tsk->comm, 0600, pg_proc_dir);
 	if (!pe) {
 		printk("pktgen: cannot create %s/%s procfs entry.\n",
-		       PG_PROC_DIR, t->name);
+		       PG_PROC_DIR, t->tsk->comm);
+		kthread_stop(p);
+		list_del(&t->th_list);
 		kfree(t);
 		return -EINVAL;
 	}
@@ -3548,21 +3505,7 @@ static int __init pktgen_create_thread(const char *name, int cpu)
 	pe->proc_fops = &pktgen_thread_fops;
 	pe->data = t;
 
-	INIT_LIST_HEAD(&t->if_list);
-
-	list_add_tail(&t->th_list, &pktgen_threads);
-
-	t->removed = 0;
-
-	err = kernel_thread((void *)pktgen_thread_worker, (void *)t,
-			  CLONE_FS | CLONE_FILES | CLONE_SIGHAND);
-	if (err < 0) {
-		printk("pktgen: kernel_thread() failed for cpu %d\n", t->cpu);
-		remove_proc_entry(t->name, pg_proc_dir);
-		list_del(&t->th_list);
-		kfree(t);
-		return err;
-	}
+	wake_up_process(p);
 
 	return 0;
 }
@@ -3643,10 +3586,8 @@ static int __init pg_init(void)
 
 	for_each_online_cpu(cpu) {
 		int err;
-		char buf[30];
 
-		sprintf(buf, "kpktgend_%i", cpu);
-		err = pktgen_create_thread(buf, cpu);
+		err = pktgen_create_thread(cpu);
 		if (err)
 			printk("pktgen: WARNING: Cannot create thread for cpu %d (%d)\n",
 					cpu, err);
@@ -3674,9 +3615,8 @@ static void __exit pg_cleanup(void)
 
 	list_for_each_safe(q, n, &pktgen_threads) {
 		t = list_entry(q, struct pktgen_thread, th_list);
-		t->control |= (T_TERMINATE);
-
-		wait_event_interruptible_timeout(queue, (t->removed == 1), HZ);
+		kthread_stop(t->tsk);
+		kfree(t);
 	}
 
 	/* Un-register us from receiving netdevice events */