RFR: 8203817: Monitor::try_lock() should not call check_prelock_state()

[Erik Osterlund]

Hi David,

The change Per is proposing would make try_lock perform the same checks that locking without safepoint checks does. Perhaps locking without safepont checks could perform more sanity checks, but that is a separate issue. I think try_lock should perform the same checks that locking without safepoint checks does. The alternatives are then to

1) Remove checking the prelock state like Per suggests for try_lock (then they do the same checks), or
2) Overhaul the safepoint checking refactoring out the bits that check the safepointing sanity chrcka from other deadlock checks (and correct those to check for rank <= special, and not == special), remove the safepoint checking part from try_lock and adding the deadlock checking parts to lock without safepoints.

Doing #2 seems like a different RFE. In fact I believe that would be https://bugs.openjdk.java.net/browse/JDK-8184732 that I filed a while back.

In summary, there is a whole bunch of problems in the deadlock detection system, and #2 makes it hard to not get dragged down in the rabbit hole. #1 is sufficient to  make try_lock check as much (or little) as locking without safepoint checking. And I think that is enough for the scope of this change.

Looks good to me.

Thanks,
/Erik

> On 25 May 2018, at 08:53, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Per,
> 
> Exactly what condition(s) does JFR violate? This is throwing away all the checks that guard against incorrect monitor use. It's not just about whether you'd block trying to acquire the Monitor, it's also about whether it is safe to acquire it from that code/thread in the first place. (Though I think some of the checks in there should also be considering the value of _safepoint_check_required.)
> 
> Thanks,
> David
> 
> 
>> On 25/05/2018 4:39 PM, Per Liden wrote:
>> In debug builds, Monitor::try_lock() calls check_prelock_state() to check the thread state, etc. The intention is to verify that the call is made from the correct context, a context where we're allowed to block and potentially safepoint. Unlike Monitor::lock(), Monitor::try_lock() will never block, hence the call to check_prelock_state() is overly strict and we should remove it. Removing it would match the behavior of all other non-blocking functions, like Monitor::lock_without_safepoint_check(), which doesn't call check_prelock_state() either (for a good reason).
>> The specific problem I've run into with this is related to JFR. Monitor::try_lock() is by JFR to allow non-blocking event generation, so that you can generate JFR events from "any" context without risk blocking/safepointing (the logic is doing something like, if try_lock() fails then put the event on a different queue and let the next owner of the lock handle it). The overly strict checks done by check_prelock_state() in try_lock() breaks this logic, which in turn means that you can't generate JFR event from "any" context as was intended.
>> The patch to fix this is a one-liner, just remove the call to check_prelock_state().
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203817
>> Webrev: http://cr.openjdk.java.net/~pliden/8203817/webrev.0
>> /Per