[Click] [RESEND] [PATCH] Multithreading bug.
Joonwoo Park
joonwpark81 at gmail.com
Sun Oct 14 20:57:18 EDT 2007
Hi Norbert,
I'm happy to hear that news.
Thanks.
Joonwoo Park (Jason Park)
2007/10/13, Egi, Norbert <n.egi at lancaster.ac.uk>:
> 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
>
>
>
> _______________________________________________
> click mailing list
> click at amsterdam.lcs.mit.edu
> https://amsterdam.lcs.mit.edu/mailman/listinfo/click
>
More information about the click
mailing list