RFR: 8145725: Remove the WorkAroundNPTLTimedWaitHang workaround

[Thomas Stüfe]

Hi David,

I took a look at this, this is a nice cleanup.

One question, in Parker::unpark, you moved the comparison (s<1) outside the
mutex lock scope. The pthread_cond_signal() call was already outside the
scope.

Would it make not sense to move the comparison and conditional
pthread_cond_signal into the mutex scope?

Kind Regards, Thomas



On Wed, Feb 10, 2016 at 5:41 AM, David Holmes <david.holmes at oracle.com>
wrote:

> Thanks Dmitry!
>
> Can I have a second Reviewer please.
>
> David
>
>
> On 9/02/2016 6:06 PM, Dmitry Dmitriev wrote:
>
>> Hello David,
>>
>> Looks good to me!
>>
>> Thanks,
>> Dmitry
>>
>> On 09.02.2016 4:17, David Holmes wrote:
>>
>>> Hi Dmitry,
>>>
>>> Great cleanup suggestions. I also did some missing assert ->
>>> assert_status conversions for the pthread_* functions. (And reverted a
>>> change that shouldn't have been made - assert_status is for when the
>>> "status" is the error code.)
>>>
>>> New webrev: http://cr.openjdk.java.net/~dholmes/8145725/webrev.v2/
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>> On 8/02/2016 11:24 PM, Dmitry Dmitriev wrote:
>>>
>>>> Hello David,
>>>>
>>>> Looks good to me. Just few additional clean-ups which you can consider
>>>> for implementing.
>>>> 1) hotspot/src/os/bsd/vm/os_bsd.cpp module
>>>>
>>>> I think that you can remove duplicated logic for mutex unlock from
>>>> Parker::unpark function.
>>>>
>>>> I.e. transform it from:
>>>> 4376 void Parker::unpark() {
>>>> ...
>>>> 4381   if (s < 1) {
>>>> 4382     status = pthread_mutex_unlock(_mutex);
>>>> 4383     assert_status(status == 0, status, "invariant");
>>>> 4384     status = pthread_cond_signal(_cond);
>>>> 4385     assert_status(status == 0, status, "invariant");
>>>> 4386   } else {
>>>> 4387     pthread_mutex_unlock(_mutex);
>>>> 4388     assert_status(status == 0, status, "invariant");
>>>> 4389   }
>>>> 4390 }
>>>>
>>>> To something like that:
>>>> void Parker::unpark() {
>>>> ...
>>>>    status = pthread_mutex_unlock(_mutex);
>>>>    assert_status(status == 0, status, "invariant");
>>>>    if (s < 1) {
>>>>      status = pthread_cond_signal(_cond);
>>>>      assert_status(status == 0, status, "invariant");
>>>>    }
>>>>   }
>>>>
>>>> 2) hotspot/src/os/linux/vm/os_linux.cpp
>>>>
>>>> Similar refactoring for Parker::unpark for removing duplicated logic for
>>>> mutex unlock.
>>>>
>>>> From:
>>>> 5720 void Parker::unpark() {
>>>> ...
>>>> 5725   if (s < 1) {
>>>> 5726     // thread might be parked
>>>> 5727     if (_cur_index != -1) {
>>>> 5728       // thread is definitely parked
>>>> 5729       // must capture correct index before unlocking
>>>> 5730       int index = _cur_index;
>>>> 5731       status = pthread_mutex_unlock(_mutex);
>>>> 5732       assert(status == 0, "invariant");
>>>> 5733       status = pthread_cond_signal(&_cond[index]);
>>>> 5734       assert(status == 0, "invariant");
>>>> 5735     } else {
>>>> 5736       pthread_mutex_unlock(_mutex);
>>>> 5737       assert(status == 0, "invariant");
>>>> 5738     }
>>>> 5739   } else {
>>>> 5740     pthread_mutex_unlock(_mutex);
>>>> 5741     assert(status == 0, "invariant");
>>>> 5742   }
>>>> 5743 }
>>>>
>>>> To:
>>>>
>>>> void Parker::unpark() {
>>>> ...
>>>>    if ((s < 1) && (_cur_index != -1)) {
>>>>        // thread is definitely parked
>>>>        // must capture correct index before unlocking
>>>>        int index = _cur_index;
>>>>        status = pthread_mutex_unlock(_mutex);
>>>>        assert(status == 0, "invariant");
>>>>        status = pthread_cond_signal(&_cond[index]);
>>>>        assert(status == 0, "invariant");
>>>>     } else {
>>>>       pthread_mutex_unlock(_mutex);
>>>>       assert(status == 0, "invariant");
>>>>     }
>>>> }
>>>>
>>>>
>>>> Thanks,
>>>> Dmitry
>>>>
>>>> On 08.02.2016 10:16, David Holmes wrote:
>>>>
>>>>> webrev: http://cr.openjdk.java.net/~dholmes/8145725/webrev/
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8145725
>>>>>
>>>>> In JDK-8130728 we disabled the workaround by default and have not seen
>>>>> any issues (other than JDK-8029453). So now the workaround can be
>>>>> removed completely.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>
>>>>
>>