Was last Barrier Patch OK?

Tue Mar 7 21:15:50 UTC 2006

Thomas Doerfler wrote:
> Joel,
> 
> how often will I look at the patch and find bugs again, sigh!

Darn! Had missed that one,too. How long will it take 4 people to
get this right???!!! It's a *#@$ JOKE

;-)
T.

> 
> In the macro _Thread_Disable_dispatch(), the memory barrier seems to be 
> misplaced.
> 
> I have corrected it (hopefully) and nothing else in the attached patch.
> 
> So the temperature in Alabama is almost similar to munich, it seems. we 
> got half a meter of snow this weekend, this is the absolute record at 
> least for a century. And it just starts snowing again =8>
> 
> wkr,
> Thomas.
> 
> 
> Joel Sherrill wrote:
> 
>> Thomas Doerfler (nt) wrote:
>>
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>> Till,
>>>
>>> ups, you got it, I actually missed that one when I checked the patch.
>>>
>>> Joel, can you fix it (AFTER! the camping trip with your boys)?
>>>
>>> wkr,
>>> Thomas.
>>>
>>>
>>>
>>> Till Straumann schrieb:
>>>  
>>>
>>>> Sorry, NO, it's still *not* OK.
>>>>
>>>> - the '.inl' version of _Thread_Unnest_dispatch()
>>>>  has the barrier after the decrement operation instead
>>>>  of before.
>>>>
>>>> Looks like you could use some vacation...
>>>>
>>>> ;-)
>>>>
>>>> -- T.
>>>>   
>>
>>
>>
>> Yeah and time to recover from it afterwards. :)  We were near Auburn 
>> Alabama and it got down
>> below freezing Friday night.  The days were warm and sunny while the 
>> nights were cold.  Combine
>> that with the 4 hour drive there and I didn't get much sleep the first 
>> night.  I will barely have time to
>> catch up on my sleep before doing another trip to Montgomery this 
>> weekend.  One of my sons is on
>> the school Science Olympiad team which won the regional competition 
>> and is going to state!
>>
>> Anyway I think I have this all fixed in the attached patch.
>>  critical section enter
>>  barrier
>>    protected operation
>>  barrier
>>  critical section exit
>>
>> One other thought.  Can gcc move the critical section enter earlier 
>> and exit later to include more
>> in the critical section?
>> --joel
>>
>>
>> ------------------------------------------------------------------------
>>
>> Index: cpukit/score/include/rtems/system.h
>> ===================================================================
>> RCS file: /usr1/CVS/rtems/cpukit/score/include/rtems/system.h,v
>> retrieving revision 1.32.2.5
>> diff -u -r1.32.2.5 system.h
>> --- cpukit/score/include/rtems/system.h    14 Jan 2005 05:23:21 
>> -0000    1.32.2.5
>> +++ cpukit/score/include/rtems/system.h    7 Mar 2006 12:14:04 -0000
>> @@ -102,6 +102,17 @@
>>  #endif
>>  
>>  /*
>> + *  The following macro is a compiler specific way to ensure that memory
>> + *  writes are not reordered around certian points.  This 
>> specifically can
>> + *  impact interrupt disable and thread dispatching critical sections.
>> + */
>> +#ifdef __GNUC__
>> +  #define RTEMS_COMPILER_MEMORY_BARRIER() asm volatile("" ::: "memory")
>> +#else
>> +  #define RTEMS_COMPILER_MEMORY_BARRIER()
>> +#endif
>> +
>> +/*
>>   *  The following are used by the POSIX implementation to catch bad 
>> paths.
>>   */
>>  
>> Index: cpukit/score/include/rtems/score/isr.h
>> ===================================================================
>> RCS file: /usr1/CVS/rtems/cpukit/score/include/rtems/score/isr.h,v
>> retrieving revision 1.17.2.1
>> diff -u -r1.17.2.1 isr.h
>> --- cpukit/score/include/rtems/score/isr.h    4 Sep 2003 18:55:05 
>> -0000    1.17.2.1
>> +++ cpukit/score/include/rtems/score/isr.h    7 Mar 2006 12:14:04 -0000
>> @@ -111,7 +111,10 @@
>>   */
>>  
>>  #define _ISR_Disable( _level ) \
>> -        _CPU_ISR_Disable( _level )
>> +  do { \
>> +    _CPU_ISR_Disable( _level ); \
>> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
>> +  } while (0)
>>  
>>  /*
>>   *  _ISR_Enable
>> @@ -124,7 +127,10 @@
>>   */
>>  
>>  #define _ISR_Enable( _level ) \
>> -        _CPU_ISR_Enable( _level )
>> +  do { \
>> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
>> +    _CPU_ISR_Enable( _level ); \
>> +  } while (0)
>>  
>>  /*
>>   *  _ISR_Flash
>> @@ -144,7 +150,11 @@
>>   */
>>  
>>  #define _ISR_Flash( _level ) \
>> -        _CPU_ISR_Flash( _level )
>> +  do { \
>> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
>> +    _CPU_ISR_Flash( _level ); \
>> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
>> +  } while (0)
>>  
>>  /*
>>   *  _ISR_Install_vector
>> Index: cpukit/score/inline/rtems/score/thread.inl
>> ===================================================================
>> RCS file: /usr1/CVS/rtems/cpukit/score/inline/rtems/score/thread.inl,v
>> retrieving revision 1.17.2.2
>> diff -u -r1.17.2.2 thread.inl
>> --- cpukit/score/inline/rtems/score/thread.inl    15 Sep 2003 02:12:47 
>> -0000    1.17.2.2
>> +++ cpukit/score/inline/rtems/score/thread.inl    7 Mar 2006 12:14:04 
>> -0000
>> @@ -184,6 +184,7 @@
>>  RTEMS_INLINE_ROUTINE void _Thread_Disable_dispatch( void )
>>  {
>>    _Thread_Dispatch_disable_level += 1;
>> +  RTEMS_COMPILER_MEMORY_BARRIER();
>>  }
>>  
>>  /*PAGE
>> @@ -201,6 +202,7 @@
>>  #if ( CPU_INLINE_ENABLE_DISPATCH == TRUE )
>>  RTEMS_INLINE_ROUTINE void _Thread_Enable_dispatch()
>>  {
>> +  RTEMS_COMPILER_MEMORY_BARRIER();
>>    if ( (--_Thread_Dispatch_disable_level) == 0 )
>>      _Thread_Dispatch();
>>  }
>> @@ -223,6 +225,7 @@
>>  
>>  RTEMS_INLINE_ROUTINE void _Thread_Unnest_dispatch( void )
>>  {
>> +  RTEMS_COMPILER_MEMORY_BARRIER();
>>    _Thread_Dispatch_disable_level -= 1;
>>  }
>>  
>> Index: cpukit/score/macros/rtems/score/thread.inl
>> ===================================================================
>> RCS file: /usr1/CVS/rtems/cpukit/score/macros/rtems/score/thread.inl,v
>> retrieving revision 1.12.2.1
>> diff -u -r1.12.2.1 thread.inl
>> --- cpukit/score/macros/rtems/score/thread.inl    4 Sep 2003 18:55:06 
>> -0000    1.12.2.1
>> +++ cpukit/score/macros/rtems/score/thread.inl    7 Mar 2006 12:14:04 
>> -0000
>> @@ -126,7 +126,10 @@
>>   */
>>  
>>  #define _Thread_Disable_dispatch() \
>> -  _Thread_Dispatch_disable_level += 1
>> +  do { \
>> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
>> +    _Thread_Dispatch_disable_level += 1; \
>> +  } while (0)
>>  
>>  /*PAGE
>>   *
>> @@ -136,9 +139,11 @@
>>  
>>  #if ( CPU_INLINE_ENABLE_DISPATCH == TRUE )
>>  #define _Thread_Enable_dispatch()  \
>> -      { if ( (--_Thread_Dispatch_disable_level) == 0 ) \
>> -             _Thread_Dispatch();  \
>> -      }
>> +  do { \
>> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
>> +    if ( (--_Thread_Dispatch_disable_level) == 0 ) \
>> +       _Thread_Dispatch(); \
>> +  } while (0)
>>  #endif
>>  
>>  #if ( CPU_INLINE_ENABLE_DISPATCH == FALSE )
>> @@ -152,7 +157,10 @@
>>   */
>>  
>>  #define _Thread_Unnest_dispatch()  \
>> -  _Thread_Dispatch_disable_level -= 1
>> +  do { \
>> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
>> +    _Thread_Dispatch_disable_level -= 1; \
>> +  } while (0)
>>  
>>  /*PAGE
>>   *
> 
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Index: cpukit/score/include/rtems/system.h
> ===================================================================
> RCS file: /usr1/CVS/rtems/cpukit/score/include/rtems/system.h,v
> retrieving revision 1.32.2.5
> diff -u -r1.32.2.5 system.h
> --- cpukit/score/include/rtems/system.h	14 Jan 2005 05:23:21 -0000	1.32.2.5
> +++ cpukit/score/include/rtems/system.h	7 Mar 2006 12:14:04 -0000
> @@ -102,6 +102,17 @@
>  #endif
>  
>  /*
> + *  The following macro is a compiler specific way to ensure that memory
> + *  writes are not reordered around certian points.  This specifically can
> + *  impact interrupt disable and thread dispatching critical sections.
> + */
> +#ifdef __GNUC__
> +  #define RTEMS_COMPILER_MEMORY_BARRIER() asm volatile("" ::: "memory")
> +#else
> +  #define RTEMS_COMPILER_MEMORY_BARRIER()
> +#endif
> +
> +/*
>   *  The following are used by the POSIX implementation to catch bad paths.
>   */
>  
> Index: cpukit/score/include/rtems/score/isr.h
> ===================================================================
> RCS file: /usr1/CVS/rtems/cpukit/score/include/rtems/score/isr.h,v
> retrieving revision 1.17.2.1
> diff -u -r1.17.2.1 isr.h
> --- cpukit/score/include/rtems/score/isr.h	4 Sep 2003 18:55:05 -0000	1.17.2.1
> +++ cpukit/score/include/rtems/score/isr.h	7 Mar 2006 12:14:04 -0000
> @@ -111,7 +111,10 @@
>   */
>  
>  #define _ISR_Disable( _level ) \
> -        _CPU_ISR_Disable( _level )
> +  do { \
> +    _CPU_ISR_Disable( _level ); \
> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
> +  } while (0)
>  
>  /*
>   *  _ISR_Enable
> @@ -124,7 +127,10 @@
>   */
>  
>  #define _ISR_Enable( _level ) \
> -        _CPU_ISR_Enable( _level )
> +  do { \
> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
> +    _CPU_ISR_Enable( _level ); \
> +  } while (0)
>  
>  /*
>   *  _ISR_Flash
> @@ -144,7 +150,11 @@
>   */
>  
>  #define _ISR_Flash( _level ) \
> -        _CPU_ISR_Flash( _level )
> +  do { \
> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
> +    _CPU_ISR_Flash( _level ); \
> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
> +  } while (0)
>  
>  /*
>   *  _ISR_Install_vector
> Index: cpukit/score/inline/rtems/score/thread.inl
> ===================================================================
> RCS file: /usr1/CVS/rtems/cpukit/score/inline/rtems/score/thread.inl,v
> retrieving revision 1.17.2.2
> diff -u -r1.17.2.2 thread.inl
> --- cpukit/score/inline/rtems/score/thread.inl	15 Sep 2003 02:12:47 -0000	1.17.2.2
> +++ cpukit/score/inline/rtems/score/thread.inl	7 Mar 2006 12:14:04 -0000
> @@ -184,6 +184,7 @@
>  RTEMS_INLINE_ROUTINE void _Thread_Disable_dispatch( void )
>  {
>    _Thread_Dispatch_disable_level += 1;
> +  RTEMS_COMPILER_MEMORY_BARRIER();
>  }
>  
>  /*PAGE
> @@ -201,6 +202,7 @@
>  #if ( CPU_INLINE_ENABLE_DISPATCH == TRUE )
>  RTEMS_INLINE_ROUTINE void _Thread_Enable_dispatch()
>  {
> +  RTEMS_COMPILER_MEMORY_BARRIER();
>    if ( (--_Thread_Dispatch_disable_level) == 0 )
>      _Thread_Dispatch();
>  }
> @@ -223,6 +225,7 @@
>  
>  RTEMS_INLINE_ROUTINE void _Thread_Unnest_dispatch( void )
>  {
> +  RTEMS_COMPILER_MEMORY_BARRIER();
>    _Thread_Dispatch_disable_level -= 1;
>  }
>  
> Index: cpukit/score/macros/rtems/score/thread.inl
> ===================================================================
> RCS file: /usr1/CVS/rtems/cpukit/score/macros/rtems/score/thread.inl,v
> retrieving revision 1.12.2.1
> diff -u -r1.12.2.1 thread.inl
> --- cpukit/score/macros/rtems/score/thread.inl	4 Sep 2003 18:55:06 -0000	1.12.2.1
> +++ cpukit/score/macros/rtems/score/thread.inl	7 Mar 2006 12:14:04 -0000
> @@ -126,7 +126,10 @@
>   */
>  
>  #define _Thread_Disable_dispatch() \
> -  _Thread_Dispatch_disable_level += 1
> +  do { \
> +    _Thread_Dispatch_disable_level += 1; \
> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
> +  } while (0)
>  
>  /*PAGE
>   *
> @@ -136,9 +139,11 @@
>  
>  #if ( CPU_INLINE_ENABLE_DISPATCH == TRUE )
>  #define _Thread_Enable_dispatch()  \
> -      { if ( (--_Thread_Dispatch_disable_level) == 0 ) \
> -             _Thread_Dispatch();  \
> -      }
> +  do { \
> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
> +    if ( (--_Thread_Dispatch_disable_level) == 0 ) \
> +       _Thread_Dispatch(); \
> +  } while (0)
>  #endif
>  
>  #if ( CPU_INLINE_ENABLE_DISPATCH == FALSE )
> @@ -152,7 +157,10 @@
>   */
>  
>  #define _Thread_Unnest_dispatch()  \
> -  _Thread_Dispatch_disable_level -= 1
> +  do { \
> +    RTEMS_COMPILER_MEMORY_BARRIER(); \
> +    _Thread_Dispatch_disable_level -= 1; \
> +  } while (0)
>  
>  /*PAGE
>   *