gettimeofday seconds rollover problem?

[Till Straumann]

Andrew Sinclair wrote:
> 1) interesting...
> 2) I think an alternative here is adding all the registers to the
> clobber list in the "barrier", we can force the compiler to reload any
> data into registers, and perform stores within the critical sections.

I don't see the advantage this has over 'memory' in the clobber list.

In any case, this can be costly on a RISC machine with many registers.
It may require many memory accesses involving possible
cache misses and write-backs. That's why on such a machine
a subroutine may be cheaper (on a powerpc, it's only 2
instructions plus save/restore of data in volatile registers
of the caller). Definitely, proper declaration of
'volatile' objects is better (but I'm repeating myself ;-)

> 
> 3) Yes, on second thoughts I think you have something here. It is safer
> as one single asm volatile statement.

agreed

T.

> 
> 
> For example on the Coldfire...
> 
> #define m68k_disable_interrupts( _level ) \
>     do { register unsigned32   _tmpsr = 0x0700; \
>          asm __volatile__ ( "move.w %%sr,%0\n\t" \
>   		       "or.l   %0,%1\n\t" \
>   		       "move.w %1,%%sr" \
>   		       : "=d" (_level), "=d"(_tmpsr) : "1"(_tmpsr) \
>   		       :"cc","d0","d1","d2","d3","d4","d5", \
>  		       "a0","a1","a2","a3","a4","a5","a6","memory" ); \
>     } while( 0 )
> 
> #define m68k_enable_interrupts( _level ) \
>     do { asm __volatile__ ( "move.w  %0,%%sr " : : "d" (_level) \
>        :"cc","d0","d1","d2","d3","d4","d5", \
>        "a0","a1","a2","a3","a4","a5","a6","memory" ); \
> 	} while( 0 )
> 
> #define m68k_flash_interrupts( _level ) \
>    do { register unsigned32 _tmpsr = 0x0700; \
> 		asm __volatile__ ( "move.w %2,%%sr\n\t" \
> 		       "or.l   %2,%1\n\t" \
> 		       "move.w %1,%%sr" \
> 		       : "=d"(_tmpsr) : "0"(_tmpsr), "d"(_level) \
>  		       :"cc","d0","d1","d2","d3","d4","d5", \
>  		       "a0","a1","a2","a3","a4","a5","a6","memory" ); \
>    } while( 0 )
> 
> I couldn't get d6, d7, a7 registers into the clobber list. I don't know
> why.
> 
> I agree that these clock variables should be volatile. As an exercise, I
> am doing a fresh rebuild of rtems currently to see if this is ok without
> setting the clock variables volatile... 
> 
> Kind Regards
> Andrew Sinclair
> -----Original Message-----
> From: Till Straumann [mailto:strauman at slac.stanford.edu] 
> Sent: Friday, 24 February 2006 5:01 PM
> To: Andrew Sinclair
> Cc: Chris Johns; Eric Norum; Ian Caddy; Joel Sherrill; rtems-users
> Subject: Re: gettimeofday seconds rollover problem?
> 
> I have a few remarks.
> 
> 1) rtems_interrupt_disable/enable() is not the only problem.
>     Unless I'm missing something, _Thread_Dispatch_disable/enable()
>     are similar (currently just macros/inlines).
> 
> 2) I still believe that all global variables that are accessed
>     from critical sections (interrupt or dispatch disabled)
>     should be 'volatile' and that this takes care of the problem.
>     It also makes the special nature of such objects more obvious.
> 
> 3) I believe (one of) the proposed 'memory barrier' implementation(s)
> 
>           disable interrupts
>           membarrier
>            <protected section>
>           enable interrupts
>                   <<<---------------------------------------
>           membarrier                                        |
>                                                             |
>     is incorrect - compiler could move critical access here |
>     it should be:
> 
>           disable interrupts
>           membarrier
>             <protected section>
>           membarrier
>           enable interrupts
> 
>     ( same applies to interrupt_flash ).
> 
> 
> Till
> 
> Andrew Sinclair wrote:
> 
>>I agree with Chris, inline are definitely the way to go if we can.
>>
>>Originally, I was a little concerned about the separate asm volatile 
>>statements in Chris's example. Now I think that it is good. The 
>>article mentions ordering between asm volatile statements could be an 
>>issue, but those condition codes should set it right anyway.
>>
>>Also flashing interrupts would need to be dealt with if it is called.
>>Sometimes checks are made following a flash interrupt that some 
>>variable is still ok. What if that check was moved before a flash ?
>>
>>#define M68K_BARRIER() asm volatile ( "" : : : "memory", "cc" )
>>
>>...
>>
>>#if ( M68K_COLDFIRE_ARCH == 1 )
>>#define m68k_flash_interrupts( _level ) \
>>   do { register unsigned32 _tmpsr = 0x0700; \
>>		asm __volatile__ ( "move.w %2,%%sr\n\t" \
>>		       "or.l   %2,%1\n\t" \
>>		       "move.w %1,%%sr" \
>>		       : "=d"(_tmpsr) : "0"(_tmpsr), "d"(_level)); \
>>         M68K_BARRIER( ); \
>>   } while( 0 )
>>#else
>>#define m68k_flash_interrupts( _level ) \
>>	do { \
>>	  asm __volatile__ ( "move.w  %0,%%sr\n\t" \
>>                 "or.w    #0x0700,%%sr" \
>>                    : : "d" (_level)) \
>>         M68K_BARRIER( ); \
>>   } while( 0 )
>>#endif
>>
>>
>>-----Original Message-----
>>From: Chris Johns [mailto:chrisj at rtems.org]
>>Sent: Friday, 24 February 2006 9:15 AM
>>To: Eric Norum
>>Cc: Till Straumann; Andrew Sinclair; Ian Caddy; Joel Sherrill; 
>>rtems-users
>>Subject: Re: gettimeofday seconds rollover problem?
>>
>>Eric Norum wrote:
>>
>>
>>>On Feb 23, 2006, at 2:24 PM, Till Straumann wrote:
>>>
>>>
>>>
>>>>One moment, folks -- I'm not sure the "memory" approach is best for 
>>>>all architectures. On CPUs with many registers, making 
>>>>rtems_interrupt_enable()/disable a real subroutine might be cheaper.
>>>>
>>
>>
>>I think for the m68k targets having the calls in lined is still the 
>>way to go.
>>
>>
>>
>>>>In any case, I believe that declaring the relevant global variables 
>>>>'volatile' is the correct way to deal with this problem.
>>>
>>>
>>>I agree that  _TOD_Current and _TOD_Seconds_since_epoch should be 
>>>declared volatile since they are certainly updated by code in a way 
>>>that is not apparent to the optimizer.
>>
>>
>>I also agree. I have added RTEMS_VOLATILE to 
>>sccore/include/rtems/system.h and changed tod.h. Joel, should I commit
> 
> 
>>this change ?
>>
>>
>>
>>>However, I'm concerned that
>>>there are other places where code may be unexpectedly hoisted outside
>>
>>
>>>an enable/disable pair.   Adding the "memory" seems prudent.
>>
>>
>>I would like to see the barrier added. I would be concerned we miss a 
>>place that should be handled that is not.
>>
>>I also wonder if the compiler can move accesses to a volatile object 
>>to a place it thinks is ok while still making sure the object access 
>>is performed. I see we have 2 issues that should be addressed. In the 
>>case of gettimeofday I tested volatile variables and/or a barrier and 
>>the same code was generated, how-ever a newer compiler may need both.
>>
>>
>>
>>>Notice that the compiler is quite happy to use after the enable/
>>
>>disable
>>
>>
>>>the value which was saved in d0 before the enable/disable.   How has 
>>>adding the "memory" hurt anything?
>>>
>>
>>
>>Do we only need the barrier on the "enable" interrupts ?
>>
>>
>>
>>>Even if we don't add "memory" to the list of clobbered registers I 
>>>think that we should add "cc" since the condition codes really do get 
>>>mangled by the interrupt enable/disable operation.
>>>
>>
>>
>>Yes.
>>
>>Regards
>>Chris
>>
>>
> 
> 
> 
> 
>