RFR: 6900441 PlatformEvent.park(millis) on Linux could still be affected by changes to the time-of-day clock

[David Holmes]

On 13/09/2013 9:30 AM, Daniel D. Daugherty wrote:
> On 9/12/13 3:47 PM, David Holmes wrote:
>>
>> I found a bug in the assertion related logic - new webrev:
>>
>> http://cr.openjdk.java.net/~dholmes/6900441/webrev.v3/
>>
>> For the untimed case in park I wasn't setting the _cur_index value (I
>> was implicitly using zero) and so the assertion it wasn't -1 was
>> failing in unpark.
>
> So is the usage model for timed park() such that no other thread
> is calling unpark() on the thread that called timed park()?
>
> If another thread can call unpark() on a thread calling timed park(),
> then I think you may have a race in:
>
> 5917      assert(_cur_index != -1, "invariant");
>
> The thread calling unpark() might reach the assertion after the
> thread that made the timed call to park() set:
>
> 5888   _cur_index = -1;
>
> Don't know if it's really possible, but...

The code you refer to is all inside a critical region locked with the 
mutex. No races possible.


>> With regard to assert versus assert_status - yes this would work:
>>
>> assert_status(status == 0, errno, "gettimeofday");
>>
>> I had never considered that. :) When I added assert_status to diagnoze
>> a particular issue I never attempted to change all the existing
>> asserts on the library calls. So we have a lot of simple
>> "assert(status==0,"invariant") where assert_status could have been
>> used, as well as the old gettimeofday style calls. Do you want me to
>> change them all in os_linux.cpp?
>
> I guess that depends on whether we think this fix will need to be
> backported to HSX-24 or earlier... Less changes, the better, if a
> backport is in the future...

Yes backports to 7 and 6 are definitely on the cards. The 
assert/assert_status can be cleaned up as part of the big cleanup for 9 
(where a lot of the calls will disappear anyway as we get rid of all the 
ancient workarounds). So I will leave this aspect as is.

Thanks,
David

> Dan
>
>
>>
>> Thanks,
>> David
>>
>> On 13/09/2013 1:14 AM, Daniel D. Daugherty wrote:
>>> On 9/11/13 5:14 PM, David Holmes wrote:
>>>> updated webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dholmes/6900441/webrev.v2/
>>>
>>> Looks good. Possibly modulo one comment below...
>>>
>>>
>>>> On 12/09/2013 7:10 AM, Daniel D. Daugherty wrote:
>>>>
>>>>>      line 5541: assert_status(status == 0, status, "clock_gettime");
>>>>>      line 5553: assert(status == 0, "gettimeofday");
>>>>>          So why is one an assert_status() call and the other is a
>>>>>          plain old assert() call?
>>>>
>>>> Different API's. The old unix API's, like gettimeofday return -1 on
>>>> error and set errno. The "modern" posix APIs, eg pthread APIs and
>>>> clock_gettime etc, return the actual error code on error - hence
>>>> assert_status can be used to show the actual error in that case.
>>>
>>> I don't quite get what you're trying to say here.
>>> It seems that both calls are trying to verify
>>> that "status == 0". Or are you saying that:
>>>
>>>      assert_status(status == 0, status, "gettimeofday");
>>>
>>> is kind of a waste because "status" always be either "0" or "-1".
>>> So how about this:
>>>
>>>      assert_status(status == 0, errno, "gettimeofday");
>>>
>>> instead? That pattern should work to get more info.
>>>
>>> Dan
>>>
>