RFR(XS) for PeriodicTask_lock cleanup (8072439)

Tue Feb 24 17:02:11 UTC 2015

Actually thinking about this a bit more:

I think we could make all uses of PeriodicTask_lock to be acquired with MutexLocker (not Ex), and avoid passing the Mutex::_no_safepoint_check flag (and lengthy comments):

JavaThreads will (check for and) block for safepoints in WatcherThread::enroll/disenroll if the PeriodicTask_lock is being held by someone else. Same thing in before_exit().

Since the WatcherThread is not a JavaThread and will never check for a safepoint if there is a contended lock, it will call IWait() (to park) directly.

This would also make it possible to change the PeriodicTask_lock from being asserted as a “_safepoint_check_sometimes” to a “_safepoint_check_always”.

In order to do this however, we would need to rework Monitor::Wait():

The only place (currently) where there is a requirement to pass “Mutex::_no_safepoint_check” is when the WatcherThread calls Wait() – but this is because we have this in there:

  // !no_safepoint_check logically implies java_thread

  guarantee(no_safepoint_check || Self->is_Java_thread(), "invariant");

This does not make sense – a WatcherThread should not need to explicitly say “please go outside the safepoint protocol” - it is not a JavaThread so to it, there is no such thing as a safepoint.

In Monitor::lock() we branch to a safepoint check based on the Self->isJavaThread(), but in Monitor::wait() we also allow for JavaThreads to circumvent the protocol if they pass in the correct flag.

Maybe we can change Monitor::Wait() a wee bit (I know this is sensitive code), and still allow for arbitrary passings of “no_safepoint_checks” for JavaThreads, but if there is nothing passed, we take the safepoint route if there is a JavaThread, and not if there is anything else (similar to Monitor::lock()). Callers which are not JavaThreads should not need to pass these options. Combining this with the lock assertion states, such as, “_safepoint_check_always” will disallow anyone (any JavaThread) to circumvent the safepoint protocol for the PeriodicTask_lock.

I will try some experiments, so Dan please go ahead with what you already have.

Cheers

Markus

From: Markus Gronlund 
Sent: den 24 februari 2015 16:13
To: Daniel Daugherty
Cc: Alexander Garthwaite; Rickard Bäckman; David Holmes; Coleen Phillimore; hotspot-runtime-dev at openjdk.java.net; serviceability-dev at openjdk.java.net; Carsten Varming
Subject: RE: RFR(XS) for PeriodicTask_lock cleanup (8072439)

Hi Dan,

I have taken a look with your suggested patch – I think your suggestion looks very good.

I guess the original hang happened because the PeriodicTask_lock was attempted to be acquired by a JavaThread, but the PeriodicTask_lock was still held by someone else. Since the PeriodicTask_lock was taken with “Mutex::_no_safepoint_checks” it meant the JavaThread bypassed the callback for a potentially pending safepoint and instead called parked upon the PeriodicTask_lock straight away...

I think this lock should definitely be taken the way you have done in the patch.

I also think the placement of OrderAccess::fence() might have been due to some of the constructs being racy, take this for instance:

void WatcherThread::start() {

  assert(PeriodicTask_lock->owned_by_self(), "PeriodicTask_lock required");

  if (watcher_thread() == NULL && _startable) { _startable is visible since its the same thread

    _should_terminate = false; <<----------------------------- this is set but will not be visible to the WatcherThread being launched (it’s  a 0 in the static initializer however, so it is still “safe”)

    // Create the single instance of WatcherThread

    new WatcherThread();

// above the constructor for WatcherThread will start the thread, and the WatcherThread::run() might check _should_terminate before the launching thread releases the PeriodicTask_lock. Not that it will be an issue here, since _should_terminate is set to 0 in its static initializer. But thanks Dan for moving this _should_terminate lower in the loop, at least the WatcherThread will need now need a call to sleep() before reaching it (and sleep needs the PeriodicTask_lock)

But for the construct in WatcherThread::stop(), there is no need (any more?) for the OrderAccess::fence(), I think it can be safely removed.

Can you also remove the comment in thread.hpp : 704 that says:

  volatile static bool _should_terminate; // updated without holding lock

As this is not the case any longer.

Otherwise it looks good!

Thanks for fixing this

Cheers

Markus

From: Daniel D. Daugherty 
Sent: den 17 februari 2015 23:42
To: Carsten Varming
Cc: Alexander Garthwaite; Rickard Bäckman; David Holmes; Markus Grönlund; Coleen Phillimore; HYPERLINK "mailto:hotspot-runtime-dev at openjdk.java.net"hotspot-runtime-dev at openjdk.java.net; HYPERLINK "mailto:serviceability-dev at openjdk.java.net"serviceability-dev at openjdk.java.net
Subject: Re: RFR(XS) for PeriodicTask_lock cleanup (8072439)

On 2/17/15 3:22 PM, Carsten Varming wrote:

Dear Daniel, 

Looks good to me.

Thanks for the fast review.

The line: "OrderAccess::fence();  // ensure WatcherThread sees update in main loop" seems unnecessary as the lock acts as a memory barrier.

Yes, I keep looking at that line from the original work on
JDK-7127792 and wonder why it's there... I'll chase that down
with the original folks...

Dan

Carsten

On Tue, Feb 17, 2015 at 4:44 PM, Daniel D. Daugherty <HYPERLINK "mailto:daniel.daugherty at oracle.com" \ndaniel.daugherty at oracle.com> wrote:

Greetings,

My fix for the following bug:

    JDK-8047720 Xprof hangs on Solaris

that was pushed to JDK9 last June needs to be cleaned up.

Thanks to Alex Garthwaite (HYPERLINK "mailto:agarthwaite at twitter.com" \nagarthwaite at twitter.com) and Carsten
Varming (HYPERLINK "mailto:varming at gmail.com" \nvarming at gmail.com) for reporting the mess that I made
in WatcherThread::stop() and for suggesting fixes.

This code review is for a general cleanup pass on PeriodicTask_lock
and some of the surrounding code. This is a targeted review in that
I would like to hear from three groups of people:

1) The author and reviewers for:

   JDK-7127792 Add the ability to change an existing PeriodicTask's
               execution interval

   Rickard, David H, and Markus G.

2) The reviewers for:

   JDK-8047720 Xprof hangs on Solaris

   Markus G and Coleen

3) Alex and Carsten

Here's the webrev URL:

HYPERLINK "http://cr.openjdk.java.net/%7Edcubed/8072439-webrev/0-for_jdk9_hs_rt/" \nhttp://cr.openjdk.java.net/~dcubed/8072439-webrev/0-for_jdk9_hs_rt/

I've attached the original RFR for JDK-8047720 that explains
the original deadlock that was being fixed. Similar testing
will be done with this fix.

Dan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150224/c27eed3e/attachment-0001.html>