RFR: 8210832: Remove sneaky locking in class Monitor
David Holmes
david.holmes at oracle.com
Sat Feb 2 00:35:00 UTC 2019
On 2/02/2019 5:22 am, 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.
Not quite true. If you expand the truth table for this there is one case
where global_poll() could be true and we won't call block(), and that is
when uses_thread_local_poll() is true and local_poll_armed is false.
David
-----
>
> 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