RFR: 8210832: Remove sneaky locking in class Monitor
Patricio Chilano
patricio.chilano.mateo at oracle.com
Fri Feb 1 23:51:35 UTC 2019
On 2/1/19 5:55 PM, Daniel D. Daugherty wrote:
>
>
> On 2/1/19 3:20 PM, Patricio Chilano wrote:
>> Hi Dan,
>>
>>
>> On 2/1/19 2:22 PM, Daniel D. Daugherty wrote:
>>> On 2/1/19 1:01 PM, Patricio Chilano wrote:
>>>> Hi Dan,
>>>>
>>>> On 2/1/19 11:10 AM, Daniel D. Daugherty wrote:
>>>>> > Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v04/inc/
>>>>>
>>>>> src/hotspot/share/logging/logTag.hpp
>>>>> No comments.
>>>>>
>>>>> src/hotspot/share/runtime/interfaceSupport.inline.hpp
>>>>> L295: // - When transitioning in (constructor), it checks for
>>>>> safepoints without having to block, i.e calls
>>>>> Perhaps:
>>>>> // - When transitioning in (constructor), it checks for
>>>>> safepoints without blocking, i.e., calls
>>>>>
>>>>> L298: // the monitor that is in the process of being
>>>>> acquired (only setting the owner is missing).
>>>>> Perhaps:
>>>>> // the monitor that is only partially acquired.
>>>> Done.
>>>>> src/hotspot/share/runtime/mutex.cpp
>>>>> L59: log_trace(vmmonitor)("JavaThread " INTPTR_FORMAT " on %d
>>>>> attempt trying to acquire native monitor %s", p2i(self),
>>>>> retry_cnt, _name);
>>>>> nit - s/native monitor/vmmonitor/ - for consistency.
>>>> Done.
>>>>
>>>>> src/hotspot/share/runtime/mutex.hpp
>>>>> No comments.
>>>>>
>>>>> src/hotspot/share/runtime/safepointMechanism.inline.hpp
>>>>> L83: if(!uses_thread_local_poll() ||
>>>>> local_poll_armed(thread)) {
>>>>> L84: if (global_poll()) {
>>>>> I'm having trouble with this construct:
>>>>>
>>>>> - if uses_thread_local_poll() is false, then we check
>>>>> global_poll()
>>>>> and call SafepointSynchronize::block() if global_poll()
>>>>> == true.
>>>>> That makes sense since we're not using local polling.
>>>>>
>>>>> - if uses_thread_local_poll() is true, then we check
>>>>> if local_poll_armed(thread) == true and then we check
>>>>> global_poll()
>>>>> and call SafepointSynchronize::block() if global_poll()
>>>>> == true.
>>>>>
>>>>> Huh?
>>>>>
>>>>> So we only call SafepointSynchronize::block() if
>>>>> global_poll() == true
>>>>> no matter what. It seems like the checks for local polling
>>>>> don't really
>>>>> do anything here.
>>>>>
>>>>> What am I missing?
>>>> A couple of observations before going straight to the question. The
>>>> check for local poll in SafepointMechanism::should_block()
>>>> (TBIVMWDP destructor) is needed because we need to check for both,
>>>> pending safepoints or pending handshakes. In other words once we
>>>> transition back from the _thread_blocked state, there could be
>>>> either a safepoint or a handshake for that thread being processed.
>>>> If SafepointMechanism::should_block() returns true, then
>>>> SafepointMechanism::callback_if_safepoint() only checks for pending
>>>> safepoints. After that I do the check "if
>>>> (_thread->has_handshake())" for pending handshakes. I couldn't use
>>>> SafepointMechanism::block_if_requested() which checks for both like
>>>> in the normal TBIVM, because of two reasons:
>>>> - On one hand, I need to pass the "false" parameter to
>>>> SafepointSynchronize::block() to avoid blocking when transitioning
>>>> in, and avoid failing the test that we have a ThreadSafepointState
>>>> of _call_back when transitioing back. Without introducing
>>>> SafepointMechanism::callback_if_safepoint() I would have to modify
>>>> a couple of methods in SafepointMechanism to receive that extra
>>>> argument. I think this was cleaner.
>>>> -But even if I did add the extra argument to those functions and
>>>> use SafepointMechanism::block_if_requested(), I would need also to
>>>> avoid checking for handshakes in the TBIVMWDP constructor. The
>>>> processing of handshakes uses its own ThreadInVMForHandshake jacket
>>>> which internally calls SafepointMechanism::block_if_requested().
>>>> Since in the constructor we do not block for safepoints, executing
>>>> again SafepointMechanism::block_if_requested() in a handshake would
>>>> execute SS::block twice.
>>>> So to solve that I just introduced
>>>> SafepointMechanism::callback_if_safepoint() which only checks for
>>>> safepoints and uses the extra argument in SS::block.
>>>>
>>>> Now, as to why I can't just check for global polls in
>>>> SafepointMechanism::callback_if_safepoint() (since I only want to
>>>> check/stop for safepoints), this has to do with the race found by
>>>> Robbin. Since the thread local poll is armed after changing _state
>>>> to _synchronizing, if I only do a global poll,
>>>> SafepointMechanism::callback_if_safepoint(in the TBIVMWDP
>>>> constructor) could detect there is a safepoint in progress and
>>>> callback but when coming back in the destructor
>>>> SafepointMechanism::should_block() could miss detecting the
>>>> safepoint in progress since that method checks for local polls
>>>> first (it cares about both safepoints and handshakes). So I need to
>>>> match the order of checks done by SafepointMechanism::should_block().
>>>>
>>>> Any of this makes sense?
>>>
>>> Not really, but now I have a better way to ask about this.
>>>
>>> Here's should_block():
>>>
>>> 50 bool SafepointMechanism::should_block(Thread* thread) {
>>> 51 if (uses_thread_local_poll()) {
>>> 52 return local_poll(thread);
>>> 53 } else {
>>> 54 return global_poll();
>>> 55 }
>>> 56 }
>>>
>>> If uses_thread_local_poll() == true, then we return the result
>>> of local_poll(thread) and skip calling global_poll().
>>>
>>> Here's callback_if_safepoint():
>>>
>>> 82 void SafepointMechanism::callback_if_safepoint(JavaThread*
>>> thread) {
>>> 83 if(!uses_thread_local_poll() || local_poll_armed(thread)) {
>>> 84 if (global_poll()) {
>>> 85 SafepointSynchronize::block(thread, false);
>>> 86 }
>>> 87 }
>>> 88 }
>>>
>>> If uses_thread_local_poll() == false and global_poll() == true,
>>> then we call block().
>>> If uses_thread_local_poll() == true and local_poll_armed() == true
>>> and global_poll() == true, then we call block().
>>>
>>> Since we always check global_poll() == true before calling block(),
>>> then preceding expressions are moot. They do not affect whether we
>>> call block() at all.
>> It's true that those checks for local poll are not actually needed if
>> you get to execute SafepointMechanism::callback_if_safepoint() in the
>> TBIVMWDP destructor. At that point I could just check for
>> global_poll(). But because I'm previously executing
>> SafepointMechanism::should_block() to first decide whether or not I
>> need to block for a safepoint or handshake, that forces me to add the
>> local polls checks when executing
>> SafepointMechanism::callback_if_safepoint() in the TBIVMWDP
>> constructor. Otherwise let's say I removed them:
>>
>> 1- VMThread in SafepointSynchronize::begin() executes "_state =
>> _synchronizing;" and gets context switched by the OS.
>> 2 - JavaThread executes SafepointMechanism::callback_if_safepoint()
>> in TBIVMWDP constructor, checks global_poll() which returns true, and
>> callsback for safepoint without blocking.
>> 3- JavaThread goes into the _thread_blocked state and blocks, let's
>> say in "_lock.lock();" in Monitor::lock().
>> 4- JavaThread acquires the lock and transitions back. In the TBIVMWDP
>> destructor calls SafepointMechanism::should_block(). If we are using
>> local polls, should_block() will check local_poll_armed(). That will
>> return false since the VMThread didn't armed the local poll yet.
>> 5- JavaThread continues to execute in VM.
>> 6- VMThread is scheduled again, and arms local polls. At this point
>> you can have serveral problems, either the JavaThread could try to
>> callback again(i.e twice) at some point later, or not and the
>> safepoint will start while that JavaThread is executing in the VM.
>
> Making sure I understand why you think you need L83:
>
> At step #2, if you put this check back:
>
> L83 if(!uses_thread_local_poll() || local_poll_armed(thread)) {
>
> then the change in behavior is that you don't call
>
> L84 if (global_poll()) {
> L85 SafepointSynchronize::block(thread, false);
>
> because the VMThread hasn't armed local polls yet right? So L83
> keeps the target thread from calling block() too early?
Right. The idea is that if SafepointMechanism::callback_if_safepoint()
detects there is a pending safepoint then
SafepointMechanism::should_block() cannot miss detecting that same
safepoint if it is still going on.
> And then in
> step 4, in should_block(), uses_thread_local_poll() will return
> false
I think by "uses_thread_local_poll()" you probably meant
"local_poll_armed()". If we are using local polls
uses_thread_local_poll() will always return true.
> because the local poll still isn't armed yet and the target
> will call global_poll() which will return true because of _state
> == _synchronizing. The caller of should_block() in the TBIVMWDP
> destructor realizes that we need to block for a safepoint so it
> will release the lock that the VMThread _might_ want and block()
> for the safepoint.
If the local poll was still not set, the target thread will just exit
the TBIVMWDP jacket and will continue executing in the VM. At some point
later it will detect there is a safepoint and it will call SS::block().
If the local poll was set, then yes, it will release the monitor and
block() for the safepoint.
> Okay, I think I have understood what you're arguing.
>
>
> Here's a minor correction that I see in the above:
>
>> 4- JavaThread acquires the lock and transitions back. In the TBIVMWDP
>> destructor calls SafepointMechanism::should_block(). If we are using
>> local polls, should_block() will check local_poll_armed(). That will
>> return false since the VMThread didn't armed the local poll yet.
>
> In Step 4, you're missing that should_block() will call
> global_poll() and global_poll() is:
If we are using thread local polls should_block() will not call
global_poll(). It will just return whatever local_poll() returns, and
that is if the local poll is armed or not.
> 37 bool SafepointMechanism::global_poll() {
> 38 return SafepointSynchronize::do_call_back();
> 39 }
>
> which calls:
>
> 141 inline static bool do_call_back() {
> 142 return (_state != _not_synchronized);
> 143 }
>
> and the _state == _synchronizing still so should_block()
> will return true. The TBIVMWDP destructor realizes that
> we need to block for a safepoint so it will release the
> lock that the VMThread _might_ want and block() for the
> safepoint. So we do get the second block() call for the
> same safepoint from the save thread.
>
> So for this function, consider adding this comment:
>
> L82: void SafepointMechanism::callback_if_safepoint(JavaThread*
> thread) {
> L83: if (!uses_thread_local_poll() || local_poll_armed(thread)) {
> // If using thread local polls, then we should not check the
> // global_poll() and callback via block() yet if the VMThread
> // has not armed the local poll yet. Otherwise, we'll callback
> // twice for the same thread.
> L84: if (global_poll()) {
> L85: SafepointSynchronize::block(thread, false);
> L86: }
> L87: }
> L88: }
I can add that comment. Although I'm not convinced about the last
sentence since it is only a possibility to call back twice. It could
also be that nothing happens because when that JavaThread transition
back, the local poll could be already set. Or even if it is not set and
you don't detect the safepoint, the VM thread might finish executing the
safepoint before the JavaThread calls back the second time.
Patricio
> Maybe I'm the only one who isn't grok'ing this code. Dunno. The above
> comment clears it up for me, but others may say "That's obvious!"
>
> Dan
>
>
>>
>> I could add another argument in
>> SafepointMechanism::callback_if_safepoint() to skip checking local
>> polls in the destructor if that's what you mean.
>>
>>
>> Patricio
>>
>>> Dan
>>>
>>>
>>>>
>>>>
>>>> Thanks,
>>>> Patricio
>>>>
>>>>> Dan
>>>>>
>>>>> On 2/1/19 12:05 AM, Patricio Chilano wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> On 1/31/19 12:54 AM, David Holmes wrote:
>>>>>>> On 31/01/2019 7:37 am, Patricio Chilano wrote:
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>> On 1/30/19 2:29 AM, David Holmes wrote:
>>>>>>>>> Hi Patricio,
>>>>>>>>>
>>>>>>>>> <trimming>
>>>>>>>>>
>>>>>>>>> First, thanks for all the many weeks of work you've put into
>>>>>>>>> this, pulling together a number of ideas from different people
>>>>>>>>> to make it all work!
>>>>>>>> Thanks! Credit to you for the PlatformMonitor implementation : )
>>>>>>>
>>>>>>> :) Nothing innovative there.
>>>>>>>
>>>>>>>>> I've only got a few minor comments/suggestions.
>>>>>>>>>
>>>>>>>>> On 30/01/2019 10:24 am, Patricio Chilano wrote:
>>>>>>>>>> Full:
>>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8210832/v03/webrev/
>>>>>>>>>> Inc:
>>>>>>>>>> http://cr.openjdk.java.net/~pchilanomate/8210832/v03/inc/webrev/
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> src/hotspot/share/runtime/interfaceSupport.inline.hpp
>>>>>>>>>
>>>>>>>>> I'm very unclear how ThreadLockBlockInVM differs from
>>>>>>>>> ThreadBlockInVM. You've duplicated a lot of complex code which
>>>>>>>>> is masking the actual difference between the two wrappers to
>>>>>>>>> me. It seems to me that an extra arg to transition_and_fence
>>>>>>>>> should allow you to handle the new behaviour without having to
>>>>>>>>> duplicate so much of this code. In any case the semantics of
>>>>>>>>> ThreadLockBlockInVM needs to be described.
>>>>>>>> I could do it with one extra argument, but I would need to add
>>>>>>>> two extra branches in transition_and_fence(), one to decide if
>>>>>>>> I'm in the Monitor case to avoid calling
>>>>>>>> SafepointMechanism::block_if_requested() directly and another
>>>>>>>> one to actually decide if I'm transitioning in or out, since
>>>>>>>> the actions to perform are different. I think it is easier to
>>>>>>>> read without adding new conditionals, and also we will save
>>>>>>>> those extra branches, but if you think it's better this way I
>>>>>>>> can change it.
>>>>>>>
>>>>>>> I would like something that tells me more clearly how this new
>>>>>>> transition helper differs from the existing TBIVM. Sharing the
>>>>>>> code between them and using different args would be one way.
>>>>>>> Documenting the difference in comments would be another. Your
>>>>>>> choice.
>>>>>> Ok, I added a description on top of TLBIVM.
>>>>>>
>>>>>>>>> Also I'm unclear what the "Lock" in ThreadLockBlockInVM
>>>>>>>>> actually refers to. I find the name quite jarring to read.
>>>>>>>> What about changing it to ThreadBlockinMonitor?
>>>>>>>
>>>>>>> That's not quite conveying the semantics. The problem is that
>>>>>>> the semantics we are changing compared to TBIVM are not evident
>>>>>>> in the TBIVM name. If TBIVM was actually
>>>>>>> ThreadBlockInVMWithSafepointBlocking, then this new transition
>>>>>>> would obviously be ThreadBlockInVMWithoutSafepointBlocking - but
>>>>>>> perhaps that lengthy, but clear name would be okay anyway?
>>>>>> Not convinced on that name since we are blocking at safepoints in
>>>>>> the destructor. Based on the comments I added how about
>>>>>> ThreadBlockinVMWithDeadlockPrevention ? or
>>>>>> ThreadBlockinVMWithPreemption? (as in eliminate one of the
>>>>>> conditions for deadlocks).
>>>>>>
>>>>>>>>> On the subject of naming, do_preempt and preempt_by_safepoint
>>>>>>>>> don't really convey to me what happens - what is being
>>>>>>>>> "preempted" here? I would suggest a more direct
>>>>>>>>> Monitor::release_for_safepoint
>>>>>>>> Changed.
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Logging: why "nativemonitor"? The logging in mutex.cpp doesn't
>>>>>>>>> relate to a "native" monitor?? Actually I'm not even sure if
>>>>>>>>> we need bother at all with the one logging statement that is
>>>>>>>>> present.
>>>>>>>> I added it to eventually track unbounded try locks. Not sure I
>>>>>>>> follow you with the name, isn't that how we name this monitors?
>>>>>>>> I tried to differentiate them from Java monitors. What about
>>>>>>>> just "monitor"?
>>>>>>>
>>>>>>> How about vmmonitor ?
>>>>>> Ok, changed.
>>>>>>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> src/hotspot/share/runtime/mutex.cpp
>>>>>>>>>
>>>>>>>>> void Monitor::lock_without_safepoint_check(Thread * self) {
>>>>>>>>> // Ensure that the Monitor does not require or allow
>>>>>>>>> safepoint checks.
>>>>>>>>>
>>>>>>>>> The comment there should only say "not require".
>>>>>>>> Done.
>>>>>>>>
>>>>>>>>> void Monitor::preempt_by_safepoint() {
>>>>>>>>> _lock.unlock();
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> Apart from renaming this as suggested above, aren't there any
>>>>>>>>> suitable assertions we should have here? safepoint-in-progress
>>>>>>>>> or handshake-in-progress? _owner == Thread::current?
>>>>>>>> Ok, I added an assertion that owner should be NULL. Asserting
>>>>>>>> safepoint-in-progress does not really work because _state could
>>>>>>>> change to _not_synchronized right after you checked for it in
>>>>>>>> TLBIVM.
>>>>>>>
>>>>>>> Okay.
>>>>>>>
>>>>>>>>> Nit:
>>>>>>>>>
>>>>>>>>> assert(_owner == Thread::current(), "should be equal: owner="
>>>>>>>>> INTPTR_FORMAT
>>>>>>>>> ", self=" INTPTR_FORMAT, p2i(_owner),
>>>>>>>>> p2i(Thread::current()));
>>>>>>>>>
>>>>>>>>> with Dan's enhanced assertions there's an indentation issue.
>>>>>>>>> The second line should indent to the first comma, but that
>>>>>>>>> will make the second line extend way past 80 columns.
>>>>>>>>>
>>>>>>>>> Also you could factor that assertion for
>>>>>>>>> _owner==Thread::current() into its own function or macro to
>>>>>>>>> avoid the repetition.
>>>>>>>> Corrected indentation based on Dan's reply to align with _owner.
>>>>>>>
>>>>>>> I though it should indent to the comma because it is a
>>>>>>> continuation of the same argument being passed to the assert
>>>>>>> "function". But I'm okay with Dan's suggestion.
>>>>>>>
>>>>>>> Factoring it into its own little function or macro would still
>>>>>>> be good to avoid the repetition.
>>>>>> Ok, added new function assert_owned_by_self(). I could change it
>>>>>> to assert_owner(Thread*) and use it for the NULL case too
>>>>>> unifying the printed messages to something like "invalid owner:
>>>>>> owner=" INTPTR_FORMAT ", should be:" INTPTR_FORMAT. What do you
>>>>>> think?
>>>>>>
>>>>>>>>> OSThreadWaitState osts(self->osthread(), false /* not
>>>>>>>>> Object.wait() */);
>>>>>>>>>
>>>>>>>>> This needs to be returned to its original place as per Dan's
>>>>>>>>> comments.
>>>>>>>> Done.
>>>>>>>>
>>>>>>>>> } else {
>>>>>>>>> Monitor::lock(self);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> You don't need Monitor:: here
>>>>>>>> Removed.
>>>>>>>>
>>>>>>>>> // Temporary JVM_RawMonitor* support. A raw monitor can just
>>>>>>>>> be a PlatformMonitor now.
>>>>>>>>>
>>>>>>>>> This needs to be resolved before committing. Some of the
>>>>>>>>> existing commentary on what raw monitors are needs to be
>>>>>>>>> retained. Not clear if we need to set the _owner field or can
>>>>>>>>> just skip it.
>>>>>>>> Is it okay if I keep the following comments?
>>>>>>>>
>>>>>>>> // Yet another degenerate version of Monitor::lock() or
>>>>>>>> lock_without_safepoint_check()
>>>>>>>> // jvm_raw_lock() and _unlock() can be called by non-Java
>>>>>>>> threads via JVM_RawMonitorEnter.
>>>>>>>> //
>>>>>>>> // There's no expectation that JVM_RawMonitors will
>>>>>>>> interoperate properly with the native
>>>>>>>> // Mutex-Monitor constructs. We happen to implement
>>>>>>>> JVM_RawMonitors in terms of
>>>>>>>> // native Mutex-Monitors simply as a matter of convenience.
>>>>>>>
>>>>>>> Yep that's perfect. And as a future RFE we can replace them with
>>>>>>> direct use of PlatformMonitor (or PlatformMutex).
>>>>>>>
>>>>>>>>
>>>>>>>> I could keep setting the owner as _owner =
>>>>>>>> Thread::current_or_null() in jvm_raw_lock(), at least it
>>>>>>>> wouldn't hurt.
>>>>>>>
>>>>>>> It's useful for checking usage errors, but we won't have that if
>>>>>>> we replace with PlatformMonitor, so may as well drop it now IMO.
>>>>>> Ok, I added asserts that _owner should be NULL after acquiring it
>>>>>> and before releasing it though.
>>>>>>
>>>>>>>>> Monitor::~Monitor() {
>>>>>>>>> assert(_owner == NULL, "should be NULL: owner="
>>>>>>>>> INTPTR_FORMAT, p2i(_owner));
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> Will this automatically result in the PlatformMonitor
>>>>>>>>> destructor being called?
>>>>>>>> Yes, should I add a comment to make it clear that
>>>>>>>> ~PlatformMonitor() will be executed?
>>>>>>>
>>>>>>> No need - assume other people have a better understanding of C++
>>>>>>> than I do :)
>>>>>>
>>>>>> Below is version v04, which also contains a correction pointed
>>>>>> out off-list by Robbin to do a local poll first in
>>>>>> SafepointMechanism::callback_if_safepoint() if we are using local
>>>>>> polls. Since the thread local poll is armed after changing _state
>>>>>> to _synchronizing, if we only do a global poll in the TLBIVM
>>>>>> constructor we could detect there is a safepoint in progress and
>>>>>> callback but when coming back in the destructor
>>>>>> SafepointMechanism::should_block() could miss detecting the
>>>>>> safepoint in progress since that method checks first for local
>>>>>> polls.
>>>>>>
>>>>>>
>>>>>> Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v04/
>>>>>> Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v04/inc/
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Patricio
>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks for looking into this! Waiting on your comments to send
>>>>>>>> v04.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Patricio
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>> -----
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list