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
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
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
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.