[Click] Multithreading bug.

[Joonwoo Park]

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)