Re: [PATCH] pktgen: fix races between control/worker threads

2006-02-23 Thread David S. Miller
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

2006-02-22 Thread Robert Olsson

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

2006-02-17 Thread Robert Olsson

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

2006-02-16 Thread Arthur Kepner

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

2006-02-16 Thread Jesse Brandeburg
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

2006-02-16 Thread Arthur Kepner

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