gettimeofday seconds rollover problem?
Andrew Sinclair
Andrew.Sinclair at elprotech.com
Fri Feb 24 08:18:42 UTC 2006
Heheh definitely not one of my better ideas...
-----Original Message-----
From: Till Straumann [mailto:strauman at slac.stanford.edu]
Sent: Friday, 24 February 2006 5:46 PM
To: Andrew Sinclair
Cc: Chris Johns; Eric Norum; Ian Caddy; Joel Sherrill; rtems-users
Subject: Re: gettimeofday seconds rollover problem?
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
>>
>>
>
>
>
>
>
More information about the users
mailing list