RFR: 6900441 PlatformEvent.park(millis) on Linux could still be affected by changes to the time-of-day clock
david.holmes at oracle.com
Thu Sep 12 17:59:22 PDT 2013
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:
>> 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.
>> On 13/09/2013 1:14 AM, Daniel D. Daugherty wrote:
>>> On 9/11/13 5:14 PM, David Holmes wrote:
>>>> updated webrev:
>>> 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.
More information about the hotspot-dev