Request for reviews (update)(S): 6863420: os::javaTimeNanos() go backward on Solaris x86]

[David Holmes - Sun Microsystems]

On 10/21/09 03:02, Vladimir Kozlov wrote:
> David Holmes - Sun Microsystems wrote:
>> Sorry for the blast from the past ...
>>
>> On 07/25/09 05:39, Paul Hohensee wrote:
>>> Nice. :)
>>>
>>> Sometime we should update the other os platforms with their own 
>>> implementations.
>>> Nop for now of course.
>>
>> So I missed this back in July ... why didn't we define this for the 
>> other OS as needed? I assume Linux x86 has exactly the same problem.
> 
> I am not Linux expert but based on comments in os_linux.cpp in
> os::Linux::clock_init() newest kernels support monotonic clock
> so we don't need to cache timer for them.

Solaris gethrtime is also "guaranteed" to be monotonic - except there 
were bugs in that area and so we needed to enforce it ourselves. There's 
no reason to assume that the CLOCK_MONOTONIC clock might not also be 
non-monotonic on some platforms at some time - in fact we (Java RTS) 
have an open bug with RedHat MRG (real-time linux) precisely because on 
some systems we saw non-monotonic behaviour (which fortunately 
disappeared ...).

But I was referring more to defining Atomic::load on all platforms, even 
if we didn't have a definite use for it at the time on Linux. (Aside: I 
still wonder whether we have other places where we might have this same 
word-tearing issue.)

Cheers,
David
> Vladimir
> 
>> backporting this to JRTS and duplicating the caching code on Linux 
>> (for uniformity) and I need Atomic::load on Linux to do that :(
>>
>> David
>>
>>> Paul
>>>
>>> Vladimir Kozlov wrote:
>>>> I updated the fix to use Atomic::load() always as David H. suggested.
>>>>
>>>> http://cr.openjdk.java.net/~kvn/6863420/webrev.01
>>>>
>>>> Fixed 6863420: os::javaTimeNanos() go backward on Solaris x86
>>>>
>>>> Problem:
>>>> The problem is non atomic load from max_hrtime in getTimeNanos()
>>>> on platforms where long is kept in 2 32-bit register.
>>>> The loaded value could be invalid (> current time) since registers
>>>> are loaded separately and store could happened in between.
>>>> It could be returned as result if it is greater then current time.
>>>> And if the next call getTimeNanos() returns the correct time
>>>> it will be less then previous result.
>>>>
>>>> Solution:
>>>> Use new atomic long load method Atomic::load() which uses
>>>> FPU to move long value atomically on 32-bit x86 or
>>>> simply returns *src in other cases.
>>>>
>>>> Added the regression test which shows the problem.
>>>>
>>>> Reviewed by:
>>>>
>>>> Fix verified (y/n): y, regression test
>>>>
>>>> Other testing:
>>>> JPRT, ScaleHarness(6784100)
>>>>