RFR: 8210832: Remove sneaky locking in class Monitor

[Patricio Chilano]

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/


Thanks!
Patricio

> Dan
>