RFR(XS) for PeriodicTask_lock cleanup (8072439)

Tue Feb 24 21:33:08 UTC 2015

More replies embedded below...

On 2/24/15 10:02 AM, Markus Gronlund wrote:
>
> 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”.
>

Coleen (and Max) would like that... :-)

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

That's why MutexLockerEx exists... but I see your point.
We're evolving this area with the _safepoint_check_* stuff
so why not make MutexLocker smarter...

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

This is definitely a little inconsistent.

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

Yes I agree this can likely be cleaned up a bit...

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

So I'll leave the further cleanup to your experiment
and a new bug. I'll move forward with the webrev plus
the tweaks I identified in my reply to your first e-mail.

Thanks againfor the reviews!

Dan

> 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; hotspot-runtime-dev at openjdk.java.net 
> <mailto:hotspot-runtime-dev at openjdk.java.net>; 
> serviceability-dev at openjdk.java.net 
> <mailto: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 
> <daniel.daugherty at oracle.com <mailto:daniel.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 (agarthwaite at twitter.com 
> <mailto:agarthwaite at twitter.com>) and Carsten
> Varming (varming at gmail.com <mailto:varming 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:
>
> http://cr.openjdk.java.net/~dcubed/8072439-webrev/0-for_jdk9_hs_rt/ 
> <http://cr.openjdk.java.net/%7Edcubed/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
>