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
