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

[Egi, Norbert]

Hi Joonwoo,
 
I finally managed to try out your patch, I have carried out numerous tests letting Linux assign the threads to cores as well as manually assigning them to cores on the same as well as on different dies and I never experienced any crash or instability with the system, and the performance of the machine is still the same as it was.
 
Thank you very much for it, it's definitely going to make my life easier,
Norbert

________________________________

From: click-bounces at pdos.csail.mit.edu on behalf of Joonwoo Park
Sent: Tue 10/2/2007 02:33
To: Click
Cc: Egi at pdos.csail.mit.edu
Subject: Re: [Click] [RESEND] [PATCH] Multithreading bug.



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)


_______________________________________________
click mailing list
click at amsterdam.lcs.mit.edu
https://amsterdam.lcs.mit.edu/mailman/listinfo/click