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

[Daniel D. Daugherty]

On 9/12/13 3:47 PM, David Holmes wrote:
> Hi Dan,
>
> 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...


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

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