RFR: 8210832: Remove sneaky locking in class Monitor
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Jan 30 01:24:59 UTC 2019
On 1/29/19 7:39 PM, Patricio Chilano wrote:
> Hi Dan,
>
> On 1/29/19 11:00 AM, Daniel D. Daugherty wrote:
>> Hi Patricio,
>>
>> I'm stripping this down to just the possible issue in mutex.cpp...
>>
>>
>> On 1/28/19 6:13 PM, Patricio Chilano wrote:
>>> On 1/28/19 4:29 PM, Daniel D. Daugherty wrote:
>>>> On 1/28/19 2:18 PM, Patricio Chilano wrote:
>>>>> Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v02/webrev
>>>> src/hotspot/share/runtime/mutex.cpp
>>>> 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() ?
>>
>> The fact that the lock() on L214 is really Monitor::lock()
>> changes everything with respect to my comment. I'm sorry
>> I missed that crucial detail. Please ignore the above from
>> me and undo the changes that I suggested.
>>
>> Here's a new set of comments:
>>
>> L195: in_flight_monitor = this;
>> Please consider adding a comment:
>>
>> in_flight_monitor = this; // save for
>> ~ThreadLockBlockInVM
>>
>> L204: in_flight_monitor = NULL;
>> Please consider adding a comment:
>>
>> in_flight_monitor = this; // ~ThreadLockBlockInVM
>> does not need to unlock
>>
>> L209: if (in_flight_monitor != NULL) {
>> Please consider adding a comment below L209:
>>
>> // Not unlocked by ~ThreadLockBlockInVM or
>> self-suspend above.
>>
>> L214: lock();
>> I missed the fact that this is Monitor::lock(). However, the
>> problem with Monitor::lock() here is that if this call blocks,
>> then we don't have a ThreadLockBlockInVM to change our state
>> so we won't be recognized as blocking.
>>
>> Please consider changing to:
>>
>> Monitor::lock(self);
>>
>> that will give us full locking semantics.
>>
>> Okay, I think this new set of comments is on the right track, but
>> obviously I need other reviewers to point out my silliness... :-)
>>
>> I am a bit worried about whether our use of a ThreadLockBlockInVM
>> in wait() and another in lock(self) on this code path could cause
>> any confusion by observing threads. I don't think so, but I haven't
>> traced it all out from beginning to end yet.
> If you mean observing a state of _thread_blocked and performing a
> safepoint or handshake by the VMThread, the blocked thread will still
> stop in the destructor of the TLBIVM anyways. If the JavaThread has to
> execute in a new TLBIVM jacket then the same logic applies. But that
> would be the same as in Monitor::lock, since by looping around you can
> also execute different TLBIVM jackets. Not sure if that's what you meant.
>
> I sent v03 adding your fixes and Per suggestions:
>
> Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v03/webrev/
> Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v03/inc/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
No comments.
src/hotspot/os/windows/os_windows.hpp
No comments.
src/hotspot/share/runtime/interfaceSupport.inline.hpp
No comments.
src/hotspot/share/runtime/mutex.cpp
L158: ", self=" INTPTR_FORMAT, p2i(_owner),
p2i(Thread::current()));
Please change 'Thread::current()' -> 'self'.
new L188: OSThreadWaitState osts(self->osthread(), false /* not
Object.wait() */);
old L187: OSThreadWaitState osts(self->osthread(), false /*
not Object.wait() */);
Please move the OSThreadWaitState back where it was. It belongs
after the ThreadLockBlockInVM in the wait() code path.
src/hotspot/share/runtime/safepointMechanism.hpp
No comments.
Dan
>
>
> Thanks!
> Patricio
>
>> Dan
>>
>
More information about the hotspot-dev
mailing list