JDK-8074355 : make MutexLocker smarter about non-JavaThreads
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 9
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2015-03-03
  • Updated: 2019-08-15
  • Resolved: 2019-05-01
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 13
13 b19Fixed
Related Reports
Relates :  
JDK-8223190 - Test gc/arguments/TestShrinkHeapInSteps.java breaks with change for JDK-8074355
Relates :  
JDK-8210832 - Remove sneaky locking in class Monitor
Description
During the code review cycle for the following bug fix:

    JDK-8072439 fix for 8047720 may need more work 

Markus had an idea for making MutexLocker smarter when it is
being used by non-JavaThreads. Here's the e-mail conversation:

On 2/24/15 2:33 PM, Daniel D. Daugherty wrote:
> 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 again for the reviews!
>
> Dan
>
>>  
>>
>> Cheers
>>
>> Markus
Comments
[~coleenp] > VMOperationQueue_lock sometimes -> never. The JavaThreads and > VMThread share this lock and it always taken with no_safepoint_check_flag > with the VMThread. You mention that JavaThreads share this lock, but you don't say how they take this lock. > Terminator_lock ... > WatcherThread itself is taken and waits with no_safepoint_check_flag. Perhaps: WatcherThread takes the lock and waits with no_safepoint_check_flag.
01-05-2019
Locks safepoint checking property changed with this patch: ParGCRareEvent_lock sometimes -> always. This lock always stops for safepoints for Java threads but has Mutex::_no_safepoint_check_flag when taken from GC threads. CGCPhaseManager_lock sometimes -> always. Same. VMOperationQueue_lock sometimes -> never. The JavaThreads and VMThread share this lock and it always taken with no_safepoint_check_flag with the VMThread. VMOperationRequest_lock sometimes -> always. The VMThread acquires and waits with no_safepoint_check but JavaThreads all safepoint check. Terminator_lock sometimes -> always. Used by the WatcherThread. When WatcherThread::start/stop are called by JavaThreads, they check for safepoints. The WatcherThread itself is taken and waits with no_safepoint_check_flag. PeriodicTask_lock sometimes -> always (same as Terminator_lock). These were sometimes because they had wait() and wait used to have a safepoint_check parameter, and asserted: // !no_safepoint_check logically implies java_thread guarantee(no_safepoint_check || Self->is_Java_thread(), "invariant"); So the wait was without safepoint check for non java threads to avoid this assertion, and the lock was made a sometimes lock to avoid the assert at the beginning of wait: // Make sure safepoint checking is used properly. assert(!(_safepoint_check_required == Monitor::_safepoint_check_never && no_safepoint_check == false), "This lock should never have a safepoint check: %s", name()); assert(!(_safepoint_check_required == Monitor::_safepoint_check_always && no_safepoint_check == true), "This lock should always have a safepoint check: %s", name()); This code is now smarter about asserting that safepoint_check_always means only the JavaThread must safepoint check consistently. JfrStacktrace_lock sometimes -> never. The only call to lock() with safepoint check was from code that asserted is_at_safepoint(), so presumably not a JavaThread. Changed to lock_without_safepoint_check() and changed the lock to _safepoint_check_never. NMethodSweeperStats_lock sometimes -> never. This lock is only acquired one place without a safepoint check so should be never.
30-04-2019
We don't need to to fix everything perfect directly, this sounds like good step in the right direction. (I believe the three exceptions can be removed)
23-04-2019
The change for JDK-8210832 removed the deadlock that can be caused by a Java thread inconsistently checking for safepoint that we've found. It used to be: thread 1 -> lock() -> TBIVM jacket -> ILock (acquire inner lock) -> block for safepoint in TBIVM destructor thread 2 -> lock_without_safepoint_check() -> ILock (wait on inner lock held by t1) -> Never reach safepoint blocked state With this change, thread1 gives up the lock in the ~TBIVMWithDeadlockCheck, so thread2 can get the inner lock and subsequently check for safepoint later. The original reason for adding _safepoint_check_{always,sometimes,never} is gone in that it is not needed now to prevent this specific deadlock. So why keep this field in Monitor? One reason is to have the flag is so we can use it to implicitly check for safepoints or not and not require the Mutex::_no_safepoint_check_flag to be passed for the safepoint_check_never locks, for example. [~kbarrett] didn't like this however because his preference is to see in the code whether the lock will stop for safepoints in the context of where it is taken. His preference is to have all locks that don't stop for safepoints, be taken with this flag. Currently this is not the case. There are safepoint_check_always locks that are taken by NonJavaThreads that don't actually check for safepoints but they're consistent, and they consistently check for Java threads. See ClassLoaderDataGraph_lock in SystemDictionary::do_unloading for example. Now that the code base has more locks that are shared between Java and NonJava Threads, the thought we had is that _safepoint_check_always, really means _safepoint_check_always_if_taken_by_JavaThread. One reason to keep this field is so that we can check that we are consistently checking for safepoints for certain locks every time they're taken by a JavaThread. This isn't to avoid deadlock; it would be to establish that the JavaThreads are being good citizens of the safepoint protocol. Changing _safepoint_check_always to _safepoint_check_JavaThread would also allow changing _safepoint_check_sometimes locks to _safepoint_check_JavaThread so that the NonJavaThreads that don't participate in the safepoint protocol are excluded from this consistency checking. The current code is a mess though. These _safepoint_check_sometimes checks effectively disable any sort of checking and there are many locks that are shared between Java and NonJava threads that have this value. I would like to change this so that JavaThreads consistent about safepoint checking (except 3 that are truly sometimes locks). This change could also enable other deadlock detection, for example: checking that if you acquire a _safepoint_check_JavaThread that you don't hold any safepoint_check_never locks.
16-04-2019
I'm currently experimenting with a simplified Mutex/Monitor implementation. As part of that I'm looking at augmenting the "safepoint checks" flags and logic.
03-10-2018
For reference here was the patch in the discussion: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/15b8ca9c2885 PeriodicTask_lock is another case where a NonJava thread has to pass around no_safepoint_check flag to the MutexLocker even though it never stops for a safepoint. It also requires it to be declared this way for the mutex safepoint consistency checks. def(PeriodicTask_lock , PaddedMonitor, nonleaf+5, true, Monitor::_safepoint_check_sometimes); even though it should *always* check for safepoint when locked from a Java thread. Because only Java threads follow the safepoint protocol (except Suspendible Thread Sets but they have their own thing).
03-10-2018
I'm not really changing mutex.cpp that much. Let's coordinate.
03-10-2018
Code cleanup issues; doesn't need to be done for 10.
05-04-2017