RFR: 8145725: Remove the WorkAroundNPTLTimedWaitHang workaround

David Holmes david.holmes at oracle.com
Wed Feb 10 11:10:58 UTC 2016


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.
>
> Would it make not sense to move the comparison and conditional
> pthread_cond_signal into the mutex scope?

The signalling is deliberately done outside the mutex scope as it is 
more performant. This is the code we have been executing in the else 
part of the "if (WorkAroundNPTLTimedWaitHang) ..."

The move of the s<1 was not intentionally done, but s is a local 
variable and so the change is not affected by the mutex.

Thanks,
David

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


More information about the hotspot-runtime-dev mailing list