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

[Joonwoo Park]

The 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)

2007-09-27 (목), 16:58 +0900, Joonwoo Park wrote: 
> Hi,
> Norbert reported previous patch didn't work.
> And I found a problem on ToDevice's::run_task().
> 
> A new patch :
> 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().
> (disable && enable local_irq at RecycledSkbPool::lock() &&
> RecycledSkbPool::unlock() can be a alternative solution for this
> problem)
> 
> Norbert, please let me know whether it works.
> 
> -
> 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..5cb5d98 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
> +    spin_lock_bh(&_dev->_xmit_lock);
> +# 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)
> _______________________________________________
> click mailing list
> click at amsterdam.lcs.mit.edu
> https://amsterdam.lcs.mit.edu/mailman/listinfo/click