Re: [PATCH] pktgen: fix races between control/worker threads
From: Robert Olsson [EMAIL PROTECTED] Date: Wed, 22 Feb 2006 19:47:13 +0100 Jesse Brandeburg writes: I looked quickly at this on a couple different machines and wasn't able to reproduce, so don't let me block the patch. I think its a good patch FWIW OK! We ask Deve to apply it. Applied to net-2.6.17, thanks. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pktgen: fix races between control/worker threads
Jesse Brandeburg writes: I looked quickly at this on a couple different machines and wasn't able to reproduce, so don't let me block the patch. I think its a good patch FWIW OK! We ask Deve to apply it. Cheers. --ro - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pktgen: fix races between control/worker threads
Arthur Kepner writes: Tanks. These races should be cured now I've tested a some runs and it works but I didn't see any problems before either. We'll hear from Jesse if this cured his problems. Cheers. --ro Signed-off-by: Robert Olsson [EMAIL PROTECTED] New patch differs from the previous one only in the ordering of the two lines above. diff --git a/net/core/pktgen.c b/net/core/pktgen.c --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -153,7 +153,7 @@ #include asm/timex.h -#define VERSION pktgen v2.63: Packet Generator for packet performance testing.\n +#define VERSION pktgen v2.64: Packet Generator for packet performance testing.\n /* #define PG_DEBUG(a) a */ #define PG_DEBUG(a) @@ -176,7 +176,8 @@ #define T_TERMINATE (10) #define T_STOP(11) /* Stop run */ #define T_RUN (12) /* Start run */ -#define T_REMDEV (13) /* Remove all devs */ +#define T_REMDEVALL (13) /* Remove all devs */ +#define T_REMDEV (14) /* Remove one dev */ /* Locks */ #define thread_lock()down(pktgen_sem) @@ -218,6 +219,8 @@ struct pktgen_dev { * we will do a random selection from within the range. */ __u32 flags; +int removal_mark; /* non-zero = the device is marked for + * removal by worker thread */ int min_pkt_size;/* = ETH_ZLEN; */ int max_pkt_size;/* = ETH_ZLEN; */ @@ -481,7 +484,7 @@ static void pktgen_stop_all_threads_ifs( static int pktgen_stop_device(struct pktgen_dev *pkt_dev); static void pktgen_stop(struct pktgen_thread* t); static void pktgen_clear_counters(struct pktgen_dev *pkt_dev); -static struct pktgen_dev *pktgen_NN_threads(const char* dev_name, int remove); +static int pktgen_mark_device(const char* ifname); static unsigned int scan_ip6(const char *s,char ip[16]); static unsigned int fmt_ip6(char *s,const char ip[16]); @@ -1406,7 +1409,7 @@ static ssize_t pktgen_thread_write(struc if (!strcmp(name, rem_device_all)) { thread_lock(); -t-control |= T_REMDEV; +t-control |= T_REMDEVALL; thread_unlock(); schedule_timeout_interruptible(msecs_to_jiffies(125)); /* Propagate thread-control */ ret = count; @@ -1457,7 +1460,8 @@ static struct pktgen_dev *__pktgen_NN_th if (pkt_dev) { if(remove) { if_lock(t); -pktgen_remove_device(t, pkt_dev); +pkt_dev-removal_mark = 1; +t-control |= T_REMDEV; if_unlock(t); } break; @@ -1467,13 +1471,44 @@ static struct pktgen_dev *__pktgen_NN_th return pkt_dev; } -static struct pktgen_dev *pktgen_NN_threads(const char* ifname, int remove) +/* + * mark a device for removal + */ +static int pktgen_mark_device(const char* ifname) { struct pktgen_dev *pkt_dev = NULL; +const int max_tries = 10, msec_per_try = 125; +int i = 0; +int ret = 0; + thread_lock(); -pkt_dev = __pktgen_NN_threads(ifname, remove); -thread_unlock(); -return pkt_dev; +PG_DEBUG(printk(pktgen: pktgen_mark_device marking %s for removal\n, +ifname)); + +while(1) { + +pkt_dev = __pktgen_NN_threads(ifname, REMOVE); +if (pkt_dev == NULL) break; /* success */ + +thread_unlock(); +PG_DEBUG(printk(pktgen: pktgen_mark_device waiting for %s +to disappear\n, ifname)); +schedule_timeout_interruptible(msecs_to_jiffies(msec_per_try)); +thread_lock(); + +if (++i = max_tries) { +printk(pktgen_mark_device: timed out after waiting +%d msec for device %s to be removed\n, +msec_per_try*i, ifname); +ret = 1; +break; +} + +} + +thread_unlock(); + +return ret; } static int pktgen_device_event(struct notifier_block *unused, unsigned long event, void *ptr) @@ -1493,7 +1528,7 @@ static int pktgen_device_event(struct no break; case NETDEV_UNREGISTER: -pktgen_NN_threads(dev-name, REMOVE); +pktgen_mark_device(dev-name); break; }; @@ -2303,11 +2338,11 @@ static void pktgen_stop_all_threads_ifs( { struct pktgen_thread *t = pktgen_threads; -PG_DEBUG(printk(pktgen: entering
Re: [PATCH] pktgen: fix races between control/worker threads
On Thu, 16 Feb 2006, Robert Olsson wrote: . if(remove) { + t-control |= T_REMDEV; + pkt_dev-removal_mark = 1; } Guess you should mark before you set control to avoid the race above. Eventually T_REMDEV could be sent from the syncing loop you added too well maybe it's not needed. . OK. For now this is done under the if_lock, so it should be fine. But doing it the way you suggest will prevent breakage if the if_lock is removed. New patch differs from the previous one only in the ordering of the two lines above. diff --git a/net/core/pktgen.c b/net/core/pktgen.c --- a/net/core/pktgen.c +++ b/net/core/pktgen.c @@ -153,7 +153,7 @@ #include asm/timex.h -#define VERSION pktgen v2.63: Packet Generator for packet performance testing.\n +#define VERSION pktgen v2.64: Packet Generator for packet performance testing.\n /* #define PG_DEBUG(a) a */ #define PG_DEBUG(a) @@ -176,7 +176,8 @@ #define T_TERMINATE (10) #define T_STOP(11) /* Stop run */ #define T_RUN (12) /* Start run */ -#define T_REMDEV (13) /* Remove all devs */ +#define T_REMDEVALL (13) /* Remove all devs */ +#define T_REMDEV (14) /* Remove one dev */ /* Locks */ #define thread_lock()down(pktgen_sem) @@ -218,6 +219,8 @@ struct pktgen_dev { * we will do a random selection from within the range. */ __u32 flags; + int removal_mark; /* non-zero = the device is marked for +* removal by worker thread */ int min_pkt_size;/* = ETH_ZLEN; */ int max_pkt_size;/* = ETH_ZLEN; */ @@ -481,7 +484,7 @@ static void pktgen_stop_all_threads_ifs( static int pktgen_stop_device(struct pktgen_dev *pkt_dev); static void pktgen_stop(struct pktgen_thread* t); static void pktgen_clear_counters(struct pktgen_dev *pkt_dev); -static struct pktgen_dev *pktgen_NN_threads(const char* dev_name, int remove); +static int pktgen_mark_device(const char* ifname); static unsigned int scan_ip6(const char *s,char ip[16]); static unsigned int fmt_ip6(char *s,const char ip[16]); @@ -1406,7 +1409,7 @@ static ssize_t pktgen_thread_write(struc if (!strcmp(name, rem_device_all)) { thread_lock(); - t-control |= T_REMDEV; + t-control |= T_REMDEVALL; thread_unlock(); schedule_timeout_interruptible(msecs_to_jiffies(125)); /* Propagate thread-control */ ret = count; @@ -1457,7 +1460,8 @@ static struct pktgen_dev *__pktgen_NN_th if (pkt_dev) { if(remove) { if_lock(t); - pktgen_remove_device(t, pkt_dev); + pkt_dev-removal_mark = 1; + t-control |= T_REMDEV; if_unlock(t); } break; @@ -1467,13 +1471,44 @@ static struct pktgen_dev *__pktgen_NN_th return pkt_dev; } -static struct pktgen_dev *pktgen_NN_threads(const char* ifname, int remove) +/* + * mark a device for removal + */ +static int pktgen_mark_device(const char* ifname) { struct pktgen_dev *pkt_dev = NULL; + const int max_tries = 10, msec_per_try = 125; + int i = 0; + int ret = 0; + thread_lock(); - pkt_dev = __pktgen_NN_threads(ifname, remove); -thread_unlock(); - return pkt_dev; +PG_DEBUG(printk(pktgen: pktgen_mark_device marking %s for removal\n, + ifname)); + + while(1) { + + pkt_dev = __pktgen_NN_threads(ifname, REMOVE); + if (pkt_dev == NULL) break; /* success */ + + thread_unlock(); + PG_DEBUG(printk(pktgen: pktgen_mark_device waiting for %s + to disappear\n, ifname)); + schedule_timeout_interruptible(msecs_to_jiffies(msec_per_try)); + thread_lock(); + + if (++i = max_tries) { + printk(pktgen_mark_device: timed out after waiting + %d msec for device %s to be removed\n, + msec_per_try*i, ifname); + ret = 1; + break; + } + + } + + thread_unlock(); + + return ret; } static int pktgen_device_event(struct notifier_block *unused, unsigned long event, void *ptr) @@ -1493,7 +1528,7 @@ static int pktgen_device_event(struct no break; case NETDEV_UNREGISTER: -pktgen_NN_threads(dev-name, REMOVE); +pktgen_mark_device(dev-name); break; };
Re: [PATCH] pktgen: fix races between control/worker threads
On 2/15/06, Arthur Kepner [EMAIL PROTECTED] wrote: Let's try this again. How does this look, Robert? There's a race in pktgen which can lead to a double free of a pktgen_dev's skb. If a worker thread is in the midst of doing fill_packet(), and the controlling thread gets a stop message, the already freed skb can be freed once again in pktgen_stop_device(). This patch gives all responsibility for cleaning up a pktgen_dev's skb to the associated worker thread. Signed-off-by: Arthur Kepner [EMAIL PROTECTED] should i test this patch to see if it fixes the pktgen panic when removing a network driver module (or renegotiating link, i don't remember where i saw the failure) in the middle of a pktgen run? - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] pktgen: fix races between control/worker threads
On Thu, 16 Feb 2006, Jesse Brandeburg wrote: should i test this patch to see if it fixes the pktgen panic when removing a network driver module (or renegotiating link, i don't remember where i saw the failure) in the middle of a pktgen run? Yes, please. Removing the network driver module during a pktgen run was one of the tests I used, and it works for me. -- Arthur - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html