gettimeofday seconds rollover problem?

[Till Straumann]

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