[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

gettimeofday seconds rollover problem?



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.

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


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