RFR: 8145725: Remove the WorkAroundNPTLTimedWaitHang workaround

[David Holmes]

Hi Thomas,

On 10/02/2016 7:32 PM, Thomas Stüfe wrote:
> 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.

I presume you were really flagging the complete expression here not the 
s<1 part specifically. As you may have seen checking _cur_index outside 
the mutex's protection was very broken :(

David
-----

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