[Click] [RESEND] [PATCH] Multithreading bug.

[Joonwoo Park]

Aha.....
There is a big fault in last patch.
The xmit_lock() should call netif_tx_lock_bh() not netif_tx_lock().
Please forgive me.
I'm resending whole patch once again. sorry again.

Signed-off-by: Joonwoo Park <joonwpark81 at gmail.com>
---
 elements/linuxmodule/todevice.cc |   46 +++++++++++++++++++++++++++++--------
 elements/linuxmodule/todevice.hh |    2 +
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/elements/linuxmodule/todevice.cc b/elements/linuxmodule/todevice.cc
index 35328ad..c2877c7 100644
--- a/elements/linuxmodule/todevice.cc
+++ b/elements/linuxmodule/todevice.cc
@@ -267,8 +267,14 @@ ToDevice::run_task(Task *)
 	    GET_STATS_RESET(low00, low10, time_now, 
 			    _perfcnt1_clean, _perfcnt2_clean, _time_clean);
 # endif
-	if (skbs)
+	if (skbs) {
+	    // The dev_watchdog acquires dev->xmit_lock by timer interrupt.
+	    // To avoid dead lock, we need to release dev->xmit_lock
+	    // before calling skbmgr_recycle_skbs() - Joonwoo Park
+	    xmit_unlock();
 	    skbmgr_recycle_skbs(skbs);
+	    xmit_lock();
+	}
 # if CLICK_DEVICE_STATS
 	if (_activations > 0 && skbs)
 	    GET_STATS_RESET(low00, low10, time_now, 
@@ -345,15 +351,7 @@ ToDevice::run_task(Task *)
     }
 #endif
 
-#if LINUX_VERSION_CODE >= 0x020400
-# if HAVE_NETIF_TX_LOCK
-    netif_tx_unlock_bh(_dev);
-# else
-    _dev->xmit_lock_owner = -1;
-    spin_unlock(&_dev->xmit_lock);
-    local_bh_enable();
-# endif
-#endif
+    xmit_unlock();
 
     // If we're polling, never go to sleep! We're relying on ToDevice to clean
     // the transmit ring.
@@ -558,5 +556,33 @@ ToDevice::add_handlers()
     add_task_handlers(&_task);
 }
 
+inline void
+ToDevice::xmit_lock()
+{
+#if LINUX_VERSION_CODE >= 0x020400
+# if HAVE_NETIF_TX_LOCK
+    netif_tx_lock_bh(_dev);
+# else
+    local_bh_disable();
+    spin_lock(&_dev->xmit_lock);
+    _dev->xmit_lock_owner = smp_processor_id();
+# endif
+#endif
+}
+
+inline void
+ToDevice::xmit_unlock()
+{
+#if LINUX_VERSION_CODE >= 0x020400
+# if HAVE_NETIF_TX_LOCK
+    netif_tx_unlock_bh(_dev);
+# else
+    _dev->xmit_lock_owner = -1;
+    spin_unlock(&_dev->xmit_lock);
+    local_bh_enable();
+# endif
+#endif
+}
+
 ELEMENT_REQUIRES(AnyDevice linuxmodule)
 EXPORT_ELEMENT(ToDevice)
diff --git a/elements/linuxmodule/todevice.hh b/elements/linuxmodule/todevice.hh
index 01aa264..0f53d95 100644
--- a/elements/linuxmodule/todevice.hh
+++ b/elements/linuxmodule/todevice.hh
@@ -159,6 +159,8 @@ class ToDevice : public AnyTaskDevice { public:
   bool _no_pad;
   
   int queue_packet(Packet *p);
+  inline void xmit_lock();
+  inline void xmit_unlock();
   
 };
---

Joonwoo Park (Jason Park)

2007-10-01 (월), 14:07 +0900, Joonwoo Park wrote:
> ThSigned-off-by: Joonwoo Park <joonwpark81 at gmail.com>e gmail demangled white spaces of the patch.
> I'm resending a patch with another mail client.
> Sorry.
> 
> Signed-off-by: Joonwoo Park <joonwpark81 at gmail.com>
> ---
>  elements/linuxmodule/todevice.cc |   46 +++++++++++++++++++++++++++++--------
>  elements/linuxmodule/todevice.hh |    2 +
>  2 files changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/elements/linuxmodule/todevice.cc b/elements/linuxmodule/todevice.cc
> index 35328ad..5ff53b5 100644
> --- a/elements/linuxmodule/todevice.cc
> +++ b/elements/linuxmodule/todevice.cc
> @@ -267,8 +267,14 @@ ToDevice::run_task(Task *)
>  	    GET_STATS_RESET(low00, low10, time_now, 
>  			    _perfcnt1_clean, _perfcnt2_clean, _time_clean);
>  # endif
> -	if (skbs)
> +	if (skbs) {
> +	    // The dev_watchdog acquires dev->xmit_lock by timer interrupt.
> +	    // To avoid dead lock, we need to release dev->xmit_lock
> +	    // before calling skbmgr_recycle_skbs() - Joonwoo Park
> +	    xmit_unlock();
>  	    skbmgr_recycle_skbs(skbs);
> +	    xmit_lock();
> +	}
>  # if CLICK_DEVICE_STATS
>  	if (_activations > 0 && skbs)
>  	    GET_STATS_RESET(low00, low10, time_now, 
> @@ -345,15 +351,7 @@ ToDevice::run_task(Task *)
>      }
>  #endif
>  
> -#if LINUX_VERSION_CODE >= 0x020400
> -# if HAVE_NETIF_TX_LOCK
> -    netif_tx_unlock_bh(_dev);
> -# else
> -    _dev->xmit_lock_owner = -1;
> -    spin_unlock(&_dev->xmit_lock);
> -    local_bh_enable();
> -# endif
> -#endif
> +    xmit_unlock();
>  
>      // If we're polling, never go to sleep! We're relying on ToDevice to clean
>      // the transmit ring.
> @@ -558,5 +556,33 @@ ToDevice::add_handlers()
>      add_task_handlers(&_task);
>  }
>  
> +inline void
> +ToDevice::xmit_lock()
> +{
> +#if LINUX_VERSION_CODE >= 0x020400
> +# if HAVE_NETIF_TX_LOCK
> +    netif_tx_lock(_dev);
> +# else
> +    local_bh_disable();
> +    spin_lock(&_dev->xmit_lock);
> +    _dev->xmit_lock_owner = smp_processor_id();
> +# endif
> +#endif
> +}
> +
> +inline void
> +ToDevice::xmit_unlock()
> +{
> +#if LINUX_VERSION_CODE >= 0x020400
> +# if HAVE_NETIF_TX_LOCK
> +    netif_tx_unlock_bh(_dev);
> +# else
> +    _dev->xmit_lock_owner = -1;
> +    spin_unlock(&_dev->xmit_lock);
> +    local_bh_enable();
> +# endif
> +#endif
> +}
> +
>  ELEMENT_REQUIRES(AnyDevice linuxmodule)
>  EXPORT_ELEMENT(ToDevice)
> diff --git a/elements/linuxmodule/todevice.hh b/elements/linuxmodule/todevice.hh
> index 01aa264..0f53d95 100644
> --- a/elements/linuxmodule/todevice.hh
> +++ b/elements/linuxmodule/todevice.hh
> @@ -159,6 +159,8 @@ class ToDevice : public AnyTaskDevice { public:
>    bool _no_pad;
>    
>    int queue_packet(Packet *p);
> +  inline void xmit_lock();
> +  inline void xmit_unlock();
>    
>  };
> ---
> 
> Joonwoo Park (Jason Park)