RFR: 8210832: Remove sneaky locking in class Monitor
Patricio Chilano
patricio.chilano.mateo at oracle.com
Mon Jan 28 23:13:26 UTC 2019
Hi Dan,
On 1/28/19 4:29 PM, Daniel D. Daugherty wrote:
> On 1/28/19 2:18 PM, Patricio Chilano wrote:
>> Hi Robbin,
>>
>> Thanks for reviewing this! Removing the block_in_safepoint_check
>> thread local attribute is a great idea, here is v02:
>>
>> Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v02/webrev
>
> src/hotspot/os/posix/os_posix.cpp
> No comments.
>
> src/hotspot/os/posix/os_posix.hpp
> No comments.
>
> src/hotspot/os/solaris/os_solaris.cpp
> No comments.
>
> src/hotspot/os/solaris/os_solaris.hpp
> No comments.
>
> src/hotspot/os/windows/os_windows.cpp
> L5309: else {
> L5310: DWORD err = GetLastError();
> L5311: assert(err == ERROR_TIMEOUT,
> "SleepConditionVariableCS: %ld:", err);
> L5312: }
> nit - please reduce indent by 2 spaces.
Done!
> src/hotspot/os/windows/os_windows.hpp
> No comments.
>
> src/hotspot/share/logging/logTag.hpp
> No comments.
>
> src/hotspot/share/runtime/interfaceSupport.inline.hpp
> No comments.
>
> src/hotspot/share/runtime/mutex.cpp
> L42: // Clear unhandled oops in JavaThreads so we get a crash
> right away
> nit - please add '.' and the end of the sentence.
Done!
> L128: assert(_owner == Thread::current(), "invariant");
> L134: assert(_owner == Thread::current(), "invariant");
> L139: assert(_owner == Thread::current(), "invariant");
> Please consider the following for better diagnostics:
> assert(_owner == Thread::current(), "should be equal:
> owner=" INTPTR_FORMAT
> ", current=" INTPTR_FORMAT, p2i(_owner),
> p2i(Thread::current()));
>
> Sorry I missed this one in the preliminary review.
>
> L155: assert(_owner == self, "invariant");
> assert(_owner == self, "should be equal: owner="
> INTPTR_FORMAT
> ", self=" INTPTR_FORMAT, p2i(_owner), p2i(self));
Done!
> L205: jt->java_suspend_self();
> L206: }
> L207: }
> L208:
> L209: if (in_flight_monitor != NULL) {
> L210: // Conceptually reestablish ownership of the lock.
> L211: assert(_owner == NULL, "should be NULL: owner="
> INTPTR_FORMAT, p2i(_owner));
> L212: set_owner(self);
> L213: } else {
> L214: lock();
> L215: }
> The lock reacquire on L214 used to be done after the
> java_suspend_self() on L205 which is inside the block
> context for the ThreadLockBlockInVM and OSThreadWaitState
> helps. If the lock() blocks due to a racing thread, then
> the calling JavaThread won't have the right thread state
> of OS thread wait state, etc...
> So after java_suspend_self(), you have to re-lock() so that
> a potential block on that re-lock has the right states. This
> also means that this line has to be deleted:
>
> L204 in_flight_monitor = NULL;
>
> so that ThreadLockBlockInVM destructor can do the right thing
> if the thread is suspended and then relocks.
Yes, I see your point. The problem is that after executing the TLBIVM
destructor (which executes after the OSThreadWaitState destructor with
the current order) there is always the possibility that we had to
release the lock, and so afterwards we will have to re-acquire it with a
different state. One simple way of solving this could be moving the
OSThreadWaitState object outside the TLBIVM block. Based on David's
comment about OSThreadWaitState I don't think changing the order should
break things, since it seems more like a debugging tool. What do you
think then of doing something like this: (I also included a re-lock
after java_suspend_self() and removed in_flight_monitor=NULL as you
suggested.)
diff --git a/src/hotspot/share/runtime/mutex.cpp
b/src/hotspot/share/runtime/mutex.cpp
--- a/src/hotspot/share/runtime/mutex.cpp
+++ b/src/hotspot/share/runtime/mutex.cpp
@@ -182,9 +186,9 @@
JavaThread *jt = (JavaThread *)self;
Monitor* in_flight_monitor = NULL;
+ OSThreadWaitState osts(self->osthread(), false /* not Object.wait()
*/);
{
ThreadLockBlockInVM tlbivm(jt, &in_flight_monitor);
- OSThreadWaitState osts(self->osthread(), false /* not
Object.wait() */);
if (as_suspend_equivalent) {
jt->set_suspend_equivalent();
// cleared by handle_special_suspend_equivalent_condition() or
@@ -201,8 +205,8 @@
// want to hold the lock while suspended because that
// would surprise the thread that suspended us.
_lock.unlock();
- in_flight_monitor = NULL;
jt->java_suspend_self();
+ _lock.lock();
}
}
> Also after lock() on L214, you never call set_owner(self)
> so the ownership is not complete for that relocated code.
> You won't need the set_owner(self) call if you move the
> lock() on L214 back to after java_suspend_self().
That lock() is actually Monitor::lock(). But I agree is confusing, every
time I look at it I say the same. Maybe I can rewrite it as
Monitor::lock() ?
> I'm not sure how I missed these two things in the preliminary
> review. I can't go back to that webrev since it looks like the
> preliminary webrev has been overwritten.
Yes, sorry I moved it to
http://cr.openjdk.java.net/~pchilanomate/8210832/preview/
> src/hotspot/share/runtime/mutex.hpp
> No comments.
>
> src/hotspot/share/runtime/mutexLocker.hpp
> No comments.
>
> src/hotspot/share/runtime/safepoint.cpp
> No comments.
>
> src/hotspot/share/runtime/safepoint.hpp
> No comments.
>
> src/hotspot/share/runtime/safepointMechanism.hpp
> L75, L78, and L81 - nit - need a period at the end of the sentence.
Done!
> src/hotspot/share/runtime/safepointMechanism.inline.hpp
> No comments.
>
> src/hotspot/share/runtime/thread.cpp
> No comments.
>
> src/hotspot/share/runtime/thread.hpp
> No comments.
Thanks for the review (and the pre-review) Dan! Waiting for you comments
to send v03.
Thanks,
Patricio
> Dan
>
>
>
>> Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v02/inc/webrev/
>>
>> Running mach5 again.
>>
>> Thanks,
>> Patricio
>>
>> On 1/28/19 8:31 AM, Robbin Ehn wrote:
>>> Hi Patricio,
>>>
>>> Mostly looks good!
>>>
>>> block_at_safepoint is always called with block_in_safepoint_check =
>>> true. (correct?)
>>> Changing that to a local state instead of global simplifies the code.
>>>
>>> So I'm suggesting something like below.
>>>
>>> Thanks, Robbin
>>>
>>> diff -r e65cc445234c
>>> src/hotspot/share/runtime/interfaceSupport.inline.hpp
>>> --- a/src/hotspot/share/runtime/interfaceSupport.inline.hpp Mon Jan
>>> 28 13:10:15 2019 +0100
>>> +++ b/src/hotspot/share/runtime/interfaceSupport.inline.hpp Mon Jan
>>> 28 14:10:59 2019 +0100
>>> @@ -308,2 +308,1 @@
>>> - thread->block_in_safepoint_check = false;
>>> - SafepointMechanism::block_at_safepoint(thread);
>>> + SafepointMechanism::callback_if_safepoint(thread);
>>> @@ -323,2 +322,1 @@
>>> - SafepointMechanism::block_at_safepoint(_thread);
>>> - _thread->block_in_safepoint_check = true;
>>> + SafepointMechanism::callback_if_safepoint(_thread);
>>> @@ -335,2 +332,0 @@
>>> - } else {
>>> - _thread->block_in_safepoint_check = true;
>>> @@ -337,0 +334,1 @@
>>> +
>>> diff -r e65cc445234c src/hotspot/share/runtime/safepoint.cpp
>>> --- a/src/hotspot/share/runtime/safepoint.cpp Mon Jan 28 13:10:15
>>> 2019 +0100
>>> +++ b/src/hotspot/share/runtime/safepoint.cpp Mon Jan 28 14:10:59
>>> 2019 +0100
>>> @@ -795,1 +795,1 @@
>>> -void SafepointSynchronize::block(JavaThread *thread) {
>>> +void SafepointSynchronize::block(JavaThread *thread, bool
>>> block_in_safepoint_check) {
>>> @@ -850,1 +850,1 @@
>>> - if (thread->block_in_safepoint_check) {
>>> + if (block_in_safepoint_check) {
>>> @@ -880,1 +880,1 @@
>>> - thread->block_in_safepoint_check) {
>>> + block_in_safepoint_check) {
>>> diff -r e65cc445234c src/hotspot/share/runtime/safepoint.hpp
>>> --- a/src/hotspot/share/runtime/safepoint.hpp Mon Jan 28 13:10:15
>>> 2019 +0100
>>> +++ b/src/hotspot/share/runtime/safepoint.hpp Mon Jan 28 14:10:59
>>> 2019 +0100
>>> @@ -146,1 +146,1 @@
>>> - static void block(JavaThread *thread);
>>> + static void block(JavaThread *thread, bool
>>> block_in_safepoint_check = true);
>>> diff -r e65cc445234c src/hotspot/share/runtime/safepointMechanism.hpp
>>> --- a/src/hotspot/share/runtime/safepointMechanism.hpp Mon Jan 28
>>> 13:10:15 2019 +0100
>>> +++ b/src/hotspot/share/runtime/safepointMechanism.hpp Mon Jan 28
>>> 14:10:59 2019 +0100
>>> @@ -82,1 +82,1 @@
>>> - static inline void block_at_safepoint(JavaThread* thread);
>>> + static inline void callback_if_safepoint(JavaThread* thread);
>>> diff -r e65cc445234c
>>> src/hotspot/share/runtime/safepointMechanism.inline.hpp
>>> --- a/src/hotspot/share/runtime/safepointMechanism.inline.hpp Mon
>>> Jan 28 13:10:15 2019 +0100
>>> +++ b/src/hotspot/share/runtime/safepointMechanism.inline.hpp Mon
>>> Jan 28 14:10:59 2019 +0100
>>> @@ -82,1 +82,1 @@
>>> -void SafepointMechanism::block_at_safepoint(JavaThread* thread) {
>>> +void SafepointMechanism::callback_if_safepoint(JavaThread* thread) {
>>> @@ -84,1 +84,1 @@
>>> - SafepointSynchronize::block(thread);
>>> + SafepointSynchronize::block(thread, false);
>>> diff -r e65cc445234c src/hotspot/share/runtime/thread.cpp
>>> --- a/src/hotspot/share/runtime/thread.cpp Mon Jan 28 13:10:15
>>> 2019 +0100
>>> +++ b/src/hotspot/share/runtime/thread.cpp Mon Jan 28 14:10:59
>>> 2019 +0100
>>> @@ -298,2 +297,0 @@
>>> - block_in_safepoint_check = true;
>>> -
>>> diff -r e65cc445234c src/hotspot/share/runtime/thread.hpp
>>> --- a/src/hotspot/share/runtime/thread.hpp Mon Jan 28 13:10:15
>>> 2019 +0100
>>> +++ b/src/hotspot/share/runtime/thread.hpp Mon Jan 28 14:10:59
>>> 2019 +0100
>>> @@ -788,2 +787,0 @@
>>> - bool block_in_safepoint_check; // to decide whether
>>> to block in SS::block or not
>>> -
>>>
>>>
>>> On 1/28/19 9:42 AM, Patricio Chilano wrote:
>>>> Hi all,
>>>>
>>>> Please review the following patch:
>>>>
>>>> Bug URL: https://bugs.openjdk.java.net/browse/JDK-8210832
>>>> Webrev: http://cr.openjdk.java.net/~pchilanomate/8210832/v01/webrev/
>>>>
>>>> The current implementation of native monitors uses a technique that
>>>> we name "sneaky locking" to prevent possible deadlocks of the JVM
>>>> during safepoints. The implementation of this technique though
>>>> introduces a race when a monitor is shared between the VMThread and
>>>> non-JavaThreads. This patch aims to solve that problem and at the
>>>> same time simplify the code.
>>>>
>>>> The proposal is based on the introduction of the new class
>>>> PlatformMonitor, which serves as a wrapper for the actual
>>>> synchronization primitives in each platform (mutexes and condition
>>>> variables). Most of the API calls can thus be implemented as simple
>>>> wrappers around PlatformMonitor, adding more assertions and very
>>>> little extra metadata.
>>>> To be able to remove the lock sneaking code and at the same time
>>>> avoid deadlocking scenarios, we combine two techniques:
>>>>
>>>> -When a JavaThread that has just acquired the lock, detects there
>>>> is a safepoint request in the ThreadLockBlockInVM destructor, it
>>>> releases the lock before blocking at the safepoint. After resuming
>>>> from it, the JavaThread will have to acquire the lock again.
>>>>
>>>> - In the ThreadLockBlockInVM constructor for the Monitor::wait()
>>>> method, in order to avoid blocking we allow for a possible
>>>> safepoint request to make progress but without letting the
>>>> JavaThread block for it (since we would be stopped by the
>>>> destructor anyways). We also do that for the Monitor::lock() case
>>>> although no deadlock is being prevented there.
>>>>
>>>> The ThreadLockBlockInVM jacket is a new ThreadStateTransition class
>>>> used instead of the ThreadBlockInVM one. This allowed more
>>>> flexibility to handle the two techniques mentioned above. Also,
>>>> ThreadBlockInVM calls SafepointMechanism::block_if_requested()
>>>> which creates some problems when trying to allow safepoints to
>>>> continue without stopping, since that method not only checks for
>>>> safepoints but also processes handshakes.
>>>>
>>>> In terms of performance, benchmarks show very similar results to
>>>> what we have now.
>>>>
>>>> So far mach5 hs-tier1-6 on Linux, OS X, Windows and Solaris have
>>>> been tested.
>>>>
>>>> Thanks,
>>>> Patricio
>>>>
>>
>
More information about the hotspot-dev
mailing list