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

[Eddie Kohler]

Joonwoo, Egi,

Thank you for the patch, Joonwoo.

I think it is simpler to just recycle the skbuffs OUTSIDE of where the lock is
held.  There's no big problem with doing so, I think.  Here is my patch.
There is a difference here in that Joonwoo's patch was applied on top of
Joonwoo's existing patches, and mine is on top of mainline.  I will look at
the other patches soon.  Performance reports welcome.

Eddie



Joonwoo Park wrote:
> 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: todevice-recycle.patch
Type: text/x-patch
Size: 1727 bytes
Desc: not available
Url : https://pdos.csail.mit.edu/pipermail/click/attachments/20071208/6c622c40/todevice-recycle.bin