* pktgen (was Re: tests of kernel modules) [not found] ` <9fa31860611211236i104cb510mb5100ea056b657db@mail.gmail.com> @ 2006-11-21 21:22 ` Alexey Dobriyan 2006-11-28 23:33 ` pktgen David Miller 0 siblings, 1 reply; 23+ messages in thread From: Alexey Dobriyan @ 2006-11-21 21:22 UTC (permalink / raw) To: Pavol Gono; +Cc: netdev [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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2006-11-21 21:22 ` pktgen (was Re: tests of kernel modules) Alexey Dobriyan @ 2006-11-28 23:33 ` David Miller 2006-11-29 20:04 ` pktgen Alexey Dobriyan 0 siblings, 1 reply; 23+ messages in thread From: David Miller @ 2006-11-28 23:33 UTC (permalink / raw) To: adobriyan; +Cc: pavol.gono, netdev 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2006-11-28 23:33 ` pktgen David Miller @ 2006-11-29 20:04 ` Alexey Dobriyan 2006-11-30 1:49 ` pktgen David Miller 0 siblings, 1 reply; 23+ messages in thread From: Alexey Dobriyan @ 2006-11-29 20:04 UTC (permalink / raw) To: David Miller; +Cc: pavol.gono, netdev 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2006-11-29 20:04 ` pktgen Alexey Dobriyan @ 2006-11-30 1:49 ` David Miller 2006-11-30 7:30 ` pktgen Alexey Dobriyan 0 siblings, 1 reply; 23+ messages in thread From: David Miller @ 2006-11-30 1:49 UTC (permalink / raw) To: adobriyan; +Cc: pavol.gono, netdev 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! ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2006-11-30 1:49 ` pktgen David Miller @ 2006-11-30 7:30 ` Alexey Dobriyan 2006-11-30 8:45 ` pktgen Robert Olsson 0 siblings, 1 reply; 23+ messages in thread From: Alexey Dobriyan @ 2006-11-30 7:30 UTC (permalink / raw) To: David Miller; +Cc: pavol.gono, netdev 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2006-11-30 7:30 ` pktgen Alexey Dobriyan @ 2006-11-30 8:45 ` Robert Olsson 2006-11-30 17:33 ` pktgen Ben Greear 0 siblings, 1 reply; 23+ messages in thread From: Robert Olsson @ 2006-11-30 8:45 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: David Miller, pavol.gono, netdev 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: pktgen 2006-11-30 8:45 ` pktgen Robert Olsson @ 2006-11-30 17:33 ` Ben Greear 2006-12-01 4:14 ` pktgen David Miller 0 siblings, 1 reply; 23+ messages in thread From: Ben Greear @ 2006-11-30 17:33 UTC (permalink / raw) To: Robert Olsson; +Cc: Alexey Dobriyan, David Miller, pavol.gono, netdev 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2006-11-30 17:33 ` pktgen Ben Greear @ 2006-12-01 4:14 ` David Miller 2006-12-01 8:14 ` pktgen Robert Olsson 2006-12-01 8:22 ` pktgen Christoph Hellwig 0 siblings, 2 replies; 23+ messages in thread From: David Miller @ 2006-12-01 4:14 UTC (permalink / raw) To: greearb; +Cc: Robert.Olsson, adobriyan, pavol.gono, netdev 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2006-12-01 4:14 ` pktgen David Miller @ 2006-12-01 8:14 ` Robert Olsson 2006-12-01 9:51 ` pktgen Alexey Dobriyan 2007-01-02 4:53 ` pktgen David Miller 2006-12-01 8:22 ` pktgen Christoph Hellwig 1 sibling, 2 replies; 23+ messages in thread From: Robert Olsson @ 2006-12-01 8:14 UTC (permalink / raw) To: David Miller; +Cc: greearb, Robert.Olsson, adobriyan, pavol.gono, netdev 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; } ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: pktgen 2006-12-01 8:14 ` pktgen Robert Olsson @ 2006-12-01 9:51 ` Alexey Dobriyan 2006-12-01 17:18 ` pktgen Robert Olsson 2006-12-01 23:25 ` pktgen David Miller 2007-01-02 4:53 ` pktgen David Miller 1 sibling, 2 replies; 23+ messages in thread From: Alexey Dobriyan @ 2006-12-01 9:51 UTC (permalink / raw) To: Robert Olsson; +Cc: David Miller, greearb, pavol.gono, netdev 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; > } ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2006-12-01 9:51 ` pktgen Alexey Dobriyan @ 2006-12-01 17:18 ` Robert Olsson 2006-12-01 23:25 ` pktgen David Miller 1 sibling, 0 replies; 23+ messages in thread From: Robert Olsson @ 2006-12-01 17:18 UTC (permalink / raw) To: Alexey Dobriyan; +Cc: Robert Olsson, David Miller, greearb, pavol.gono, netdev 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2006-12-01 9:51 ` pktgen Alexey Dobriyan 2006-12-01 17:18 ` pktgen Robert Olsson @ 2006-12-01 23:25 ` David Miller 1 sibling, 0 replies; 23+ messages in thread From: David Miller @ 2006-12-01 23:25 UTC (permalink / raw) To: adobriyan; +Cc: Robert.Olsson, greearb, pavol.gono, netdev 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2006-12-01 8:14 ` pktgen Robert Olsson 2006-12-01 9:51 ` pktgen Alexey Dobriyan @ 2007-01-02 4:53 ` David Miller 1 sibling, 0 replies; 23+ messages in thread From: David Miller @ 2007-01-02 4:53 UTC (permalink / raw) To: Robert.Olsson; +Cc: greearb, adobriyan, pavol.gono, netdev 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(¤t->sighand->siglock); - tmpsig = current->blocked; - siginitsetinv(¤t->blocked, - sigmask(SIGKILL) | sigmask(SIGSTOP) | sigmask(SIGTERM)); - - recalc_sigpending(); - spin_unlock_irq(¤t->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 */ ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: pktgen 2006-12-01 4:14 ` pktgen David Miller 2006-12-01 8:14 ` pktgen Robert Olsson @ 2006-12-01 8:22 ` Christoph Hellwig 2006-12-01 23:17 ` pktgen David Miller 1 sibling, 1 reply; 23+ messages in thread From: Christoph Hellwig @ 2006-12-01 8:22 UTC (permalink / raw) To: David Miller; +Cc: greearb, Robert.Olsson, adobriyan, pavol.gono, netdev 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2006-12-01 8:22 ` pktgen Christoph Hellwig @ 2006-12-01 23:17 ` David Miller 0 siblings, 0 replies; 23+ messages in thread From: David Miller @ 2006-12-01 23:17 UTC (permalink / raw) To: hch; +Cc: greearb, Robert.Olsson, adobriyan, pavol.gono, netdev 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <20041110165318.GA19639@xi.wantstofly.org>]
[parent not found: <20041111233507.GA3202@xi.wantstofly.org>]
[parent not found: <20041124161848.GA18059@xi.wantstofly.org>]
[parent not found: <16804.48120.375307.718766@robur.slu.se>]
[parent not found: <20041124170948.GC18059@xi.wantstofly.org>]
[parent not found: <16804.60621.990421.525393@robur.slu.se>]
[parent not found: <20041125030450.GA24417@xi.wantstofly.org>]
[parent not found: <16805.40983.937641.670275@robur.slu.se>]
[parent not found: <20041127002841.GA17184@xi.wantstofly.org>]
[parent not found: <20041127004325.GA17401@xi.wantstofly.org>]
* Re: pktgen [not found] ` <20041127004325.GA17401@xi.wantstofly.org> @ 2004-11-27 12:04 ` Robert Olsson 2004-11-27 13:53 ` pktgen Lennert Buytenhek 0 siblings, 1 reply; 23+ messages in thread From: Robert Olsson @ 2004-11-27 12:04 UTC (permalink / raw) To: Lennert Buytenhek; +Cc: Robert Olsson, hadi, netdev 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2004-11-27 12:04 ` pktgen Robert Olsson @ 2004-11-27 13:53 ` Lennert Buytenhek 2004-11-27 14:39 ` pktgen Lennert Buytenhek 2004-11-29 15:27 ` pktgen Robert Olsson 0 siblings, 2 replies; 23+ messages in thread From: Lennert Buytenhek @ 2004-11-27 13:53 UTC (permalink / raw) To: Robert Olsson; +Cc: hadi, netdev 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2004-11-27 13:53 ` pktgen Lennert Buytenhek @ 2004-11-27 14:39 ` Lennert Buytenhek 2004-11-27 15:04 ` pktgen jamal 2004-11-29 15:27 ` pktgen Robert Olsson 1 sibling, 1 reply; 23+ messages in thread From: Lennert Buytenhek @ 2004-11-27 14:39 UTC (permalink / raw) To: Robert Olsson; +Cc: hadi, netdev 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2004-11-27 14:39 ` pktgen Lennert Buytenhek @ 2004-11-27 15:04 ` jamal 2004-11-28 18:31 ` pktgen Lennert Buytenhek 0 siblings, 1 reply; 23+ messages in thread From: jamal @ 2004-11-27 15:04 UTC (permalink / raw) To: Lennert Buytenhek; +Cc: Robert Olsson, netdev 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2004-11-27 15:04 ` pktgen jamal @ 2004-11-28 18:31 ` Lennert Buytenhek 2004-11-29 13:49 ` pktgen jamal 0 siblings, 1 reply; 23+ messages in thread From: Lennert Buytenhek @ 2004-11-28 18:31 UTC (permalink / raw) To: jamal; +Cc: Robert Olsson, netdev 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_ */ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2004-11-28 18:31 ` pktgen Lennert Buytenhek @ 2004-11-29 13:49 ` jamal 2004-11-29 16:57 ` pktgen Grant Grundler 0 siblings, 1 reply; 23+ messages in thread From: jamal @ 2004-11-29 13:49 UTC (permalink / raw) To: Lennert Buytenhek; +Cc: Robert Olsson, netdev, Grant Grundler 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2004-11-29 13:49 ` pktgen jamal @ 2004-11-29 16:57 ` Grant Grundler 0 siblings, 0 replies; 23+ messages in thread From: Grant Grundler @ 2004-11-29 16:57 UTC (permalink / raw) To: jamal; +Cc: Lennert Buytenhek, Robert Olsson, netdev, 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: pktgen 2004-11-27 13:53 ` pktgen Lennert Buytenhek 2004-11-27 14:39 ` pktgen Lennert Buytenhek @ 2004-11-29 15:27 ` Robert Olsson 1 sibling, 0 replies; 23+ messages in thread From: Robert Olsson @ 2004-11-29 15:27 UTC (permalink / raw) To: Lennert Buytenhek; +Cc: Robert Olsson, hadi, netdev 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2007-01-02 4:53 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20061121174737.GA5220@martell.zuzino.mipt.ru>
[not found] ` <9fa31860611211236i104cb510mb5100ea056b657db@mail.gmail.com>
2006-11-21 21:22 ` pktgen (was Re: tests of kernel modules) Alexey Dobriyan
2006-11-28 23:33 ` pktgen David Miller
2006-11-29 20:04 ` pktgen Alexey Dobriyan
2006-11-30 1:49 ` pktgen David Miller
2006-11-30 7:30 ` pktgen Alexey Dobriyan
2006-11-30 8:45 ` pktgen Robert Olsson
2006-11-30 17:33 ` pktgen Ben Greear
2006-12-01 4:14 ` pktgen David Miller
2006-12-01 8:14 ` pktgen Robert Olsson
2006-12-01 9:51 ` pktgen Alexey Dobriyan
2006-12-01 17:18 ` pktgen Robert Olsson
2006-12-01 23:25 ` pktgen David Miller
2007-01-02 4:53 ` pktgen David Miller
2006-12-01 8:22 ` pktgen Christoph Hellwig
2006-12-01 23:17 ` pktgen David Miller
[not found] <20041110165318.GA19639@xi.wantstofly.org>
[not found] ` <20041111233507.GA3202@xi.wantstofly.org>
[not found] ` <20041124161848.GA18059@xi.wantstofly.org>
[not found] ` <16804.48120.375307.718766@robur.slu.se>
[not found] ` <20041124170948.GC18059@xi.wantstofly.org>
[not found] ` <16804.60621.990421.525393@robur.slu.se>
[not found] ` <20041125030450.GA24417@xi.wantstofly.org>
[not found] ` <16805.40983.937641.670275@robur.slu.se>
[not found] ` <20041127002841.GA17184@xi.wantstofly.org>
[not found] ` <20041127004325.GA17401@xi.wantstofly.org>
2004-11-27 12:04 ` pktgen Robert Olsson
2004-11-27 13:53 ` pktgen Lennert Buytenhek
2004-11-27 14:39 ` pktgen Lennert Buytenhek
2004-11-27 15:04 ` pktgen jamal
2004-11-28 18:31 ` pktgen Lennert Buytenhek
2004-11-29 13:49 ` pktgen jamal
2004-11-29 16:57 ` pktgen Grant Grundler
2004-11-29 15:27 ` pktgen Robert Olsson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).