RFC: bsd_tcpip patch on synch.c

RFC: bsd_tcpip patch on synch.c

[Lambrecht Jürgen]

Hello,

The patch below we already use since 2009. But I did not code the patch, 
so I do not know what problem it solves. Here are our CVS logs about the 
patch:

- To avoid deadlock on mutex 'splx_mutex', I changed 'cyg_mutex_lock' 
into 'cyg_mutex_trylock'.
- Add 'trylock' and 'cyg_thread_delay' to spl_any() in order to handle 
the deadlock issue on the mutex 'splx_mutex'

I am applying the patches from Bernd Edlinger 
(http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001656)(to fix an 
anoying problem "packet loss every 20 minutes when ARP timeout expires" 
and to add raw packets).
And I wonder if this patch (below) is still valid?
--------------------------------------------------------------------------------
Index: net/bsd_tcpip/current/src/ecos/synch.c
===================================================================
RCS file: /cvs/ecos/ecos-opt/net/net/bsd_tcpip/current/src/ecos/synch.c,v
retrieving revision 1.3
diff -u -5 -p -r1.3 synch.c
--- net/bsd_tcpip/current/src/ecos/synch.c      29 Jan 2009 17:49:56 
-0000      1.3
+++ net/bsd_tcpip/current/src/ecos/synch.c      27 Jun 2013 09:12:54 -0000
@@ -115,12 +115,14 @@ static volatile cyg_handle_t splx_thread
  static inline cyg_uint32
  spl_any( cyg_uint32 which )
  {
      cyg_uint32 old_spl = spl_state;
      if ( cyg_thread_self() != splx_thread ) {
-        while ( !cyg_mutex_lock( &splx_mutex ) )
+        while ( !cyg_mutex_trylock( &splx_mutex ) ) {
+          cyg_thread_delay(1);
              continue;
+        }
          old_spl = 0; // Free when we unlock this context
          CYG_ASSERT( 0 == splx_thread, "Thread still owned" );
          CYG_ASSERT( 0 == spl_state, "spl still set" );
          splx_thread = cyg_thread_self();
      }
--------------------------------------------------------------------------------
Kind regards,
Jürgen

-- 
Jürgen Lambrecht
R&D Associate
Mobile: +32 499 644 531
Tel: +32 (0)51 303045    Fax: +32 (0)51 310670
http://www.televic-rail.com
Televic Rail NV - Leo Bekaertlaan 1 - 8870 Izegem - Belgium
Company number 0825.539.581 - RPR Kortrijk

RE: RFC: bsd_tcpip patch on synch.c

[Bernd Edlinger]

Hi Jürgen,


the variant with cyg_mutex_trylock is like busy waiting, and
should be reverted. But I agree that the
"while (!cyg_mutex_lock()) continue;"
construct is somehow really bad style.

I do not think that cyg_mutex_lock can ever return false,
unless the spl_mutex is completely invalid.

If you really expect cyg_mutex_lock to ever return false,
then the right thing to do would be to assert(false)
and print a callstack or directly enter the debugger.


Regards
Bernd Edlinger.


> From: J.Lambrecht@TELEVIC.com
> To: ecos-devel@ecos.sourceware.org
> CC: bernd.edlinger@hotmail.de
> Date: Thu, 27 Jun 2013 11:34:12 +0200
> Subject: RFC: bsd_tcpip patch on synch.c
> 
> Hello,
> 
> The patch below we already use since 2009. But I did not code the patch, 
> so I do not know what problem it solves. Here are our CVS logs about the 
> patch:
> 
> - To avoid deadlock on mutex 'splx_mutex', I changed 'cyg_mutex_lock' 
> into 'cyg_mutex_trylock'.
> - Add 'trylock' and 'cyg_thread_delay' to spl_any() in order to handle 
> the deadlock issue on the mutex 'splx_mutex'
> 
> I am applying the patches from Bernd Edlinger 
> (http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001656)(to fix an 
> anoying problem "packet loss every 20 minutes when ARP timeout expires" 
> and to add raw packets).
> And I wonder if this patch (below) is still valid?
> --------------------------------------------------------------------------------
> Index: net/bsd_tcpip/current/src/ecos/synch.c
> ===================================================================
> RCS file: /cvs/ecos/ecos-opt/net/net/bsd_tcpip/current/src/ecos/synch.c,v
> retrieving revision 1.3
> diff -u -5 -p -r1.3 synch.c
> --- net/bsd_tcpip/current/src/ecos/synch.c      29 Jan 2009 17:49:56 
> -0000      1.3
> +++ net/bsd_tcpip/current/src/ecos/synch.c      27 Jun 2013 09:12:54 -0000
> @@ -115,12 +115,14 @@ static volatile cyg_handle_t splx_thread
>   static inline cyg_uint32
>   spl_any( cyg_uint32 which )
>   {
>       cyg_uint32 old_spl = spl_state;
>       if ( cyg_thread_self() != splx_thread ) {
> -        while ( !cyg_mutex_lock( &splx_mutex ) )
> +        while ( !cyg_mutex_trylock( &splx_mutex ) ) {
> +          cyg_thread_delay(1);
>               continue;
> +        }
>           old_spl = 0; // Free when we unlock this context
>           CYG_ASSERT( 0 == splx_thread, "Thread still owned" );
>           CYG_ASSERT( 0 == spl_state, "spl still set" );
>           splx_thread = cyg_thread_self();
>       }
> --------------------------------------------------------------------------------
> Kind regards,
> Jürgen
> 
> -- 
> Jürgen Lambrecht
> R&D Associate
> Mobile: +32 499 644 531
> Tel: +32 (0)51 303045    Fax: +32 (0)51 310670
> http://www.televic-rail.com
> Televic Rail NV - Leo Bekaertlaan 1 - 8870 Izegem - Belgium
> Company number 0825.539.581 - RPR Kortrijk 		 	   		  

Re: RFC: bsd_tcpip patch on synch.c

[Lambrecht Jürgen]

On 06/27/2013 02:51 PM, Bernd Edlinger wrote:
> Hi Jürgen,
>
>
> the variant with cyg_mutex_trylock is like busy waiting, and
> should be reverted. But I agree that the
> "while (!cyg_mutex_lock()) continue;"
> construct is somehow really bad style.
>
> I do not think that cyg_mutex_lock can ever return false,
> unless the spl_mutex is completely invalid.
After digging deep, I found that our problem was caused by something 
else, so this fix is not needed, but we kept it because we did not like 
that construct as you also point out.
>
> If you really expect cyg_mutex_lock to ever return false,
> then the right thing to do would be to assert(false)
> and print a callstack or directly enter the debugger.
No problem anymore..
But is there a better way to loop over the mutex lock?

Kind regards,
Jürgen
>
>
> Regards
> Bernd Edlinger.
>
>
>> From: J.Lambrecht@TELEVIC.com
>> To: ecos-devel@ecos.sourceware.org
>> CC: bernd.edlinger@hotmail.de
>> Date: Thu, 27 Jun 2013 11:34:12 +0200
>> Subject: RFC: bsd_tcpip patch on synch.c
>>
>> Hello,
>>
>> The patch below we already use since 2009. But I did not code the patch,
>> so I do not know what problem it solves. Here are our CVS logs about the
>> patch:
>>
>> - To avoid deadlock on mutex 'splx_mutex', I changed 'cyg_mutex_lock'
>> into 'cyg_mutex_trylock'.
>> - Add 'trylock' and 'cyg_thread_delay' to spl_any() in order to handle
>> the deadlock issue on the mutex 'splx_mutex'
>>
>> I am applying the patches from Bernd Edlinger
>> (http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001656)(to fix an
>> anoying problem "packet loss every 20 minutes when ARP timeout expires"
>> and to add raw packets).
>> And I wonder if this patch (below) is still valid?
>> --------------------------------------------------------------------------------
>> Index: net/bsd_tcpip/current/src/ecos/synch.c
>> ===================================================================
>> RCS file: /cvs/ecos/ecos-opt/net/net/bsd_tcpip/current/src/ecos/synch.c,v
>> retrieving revision 1.3
>> diff -u -5 -p -r1.3 synch.c
>> --- net/bsd_tcpip/current/src/ecos/synch.c      29 Jan 2009 17:49:56
>> -0000      1.3
>> +++ net/bsd_tcpip/current/src/ecos/synch.c      27 Jun 2013 09:12:54 -0000
>> @@ -115,12 +115,14 @@ static volatile cyg_handle_t splx_thread
>>    static inline cyg_uint32
>>    spl_any( cyg_uint32 which )
>>    {
>>        cyg_uint32 old_spl = spl_state;
>>        if ( cyg_thread_self() != splx_thread ) {
>> -        while ( !cyg_mutex_lock( &splx_mutex ) )
>> +        while ( !cyg_mutex_trylock( &splx_mutex ) ) {
>> +          cyg_thread_delay(1);
>>                continue;
>> +        }
>>            old_spl = 0; // Free when we unlock this context
>>            CYG_ASSERT( 0 == splx_thread, "Thread still owned" );
>>            CYG_ASSERT( 0 == spl_state, "spl still set" );
>>            splx_thread = cyg_thread_self();
>>        }
>> --------------------------------------------------------------------------------
>> Kind regards,
>> Jürgen
>>
>> -- 
>> Jürgen Lambrecht
>> R&D Associate
>> Mobile: +32 499 644 531
>> Tel: +32 (0)51 303045    Fax: +32 (0)51 310670
>> http://www.televic-rail.com
>> Televic Rail NV - Leo Bekaertlaan 1 - 8870 Izegem - Belgium
>> Company number 0825.539.581 - RPR Kortrijk 		 	   		


-- 
Jürgen Lambrecht
R&D Associate
Mobile: +32 499 644 531
Tel: +32 (0)51 303045    Fax: +32 (0)51 310670
http://www.televic-rail.com
Televic Rail NV - Leo Bekaertlaan 1 - 8870 Izegem - Belgium
Company number 0825.539.581 - RPR Kortrijk

FW: RFC: bsd_tcpip patch on synch.c

[Bernd Edlinger]

Jürgen,

> After digging deep, I found that our problem was caused by something 
> else, so this fix is not needed, but we kept it because we did not like 
> that construct as you also point out.
>>
>> If you really expect cyg_mutex_lock to ever return false,
>> then the right thing to do would be to assert(false)
>> and print a callstack or directly enter the debugger.
> No problem anymore..
> But is there a better way to loop over the mutex lock?

no, IMHO a simple "cyg_mutex_lock(&spl_mutex);" would have been enough.
But these patches are not about style at all.

But by the way there is another patch in that vicinitry:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001629

That may apply to everyone who uses a tick count other than the default.


Regards
Bernd.