[aarch64-port-dev ] 8153107: Unbalanced recursive locking

[Daniel D. Daugherty]

Andrey and Andrew,

I've copied the three previous emails on this thread to:

https://bugs.openjdk.java.net/browse/JDK-8153107

This particular hang was never seen on any of Oracle's other supported
platforms so I'm a bit hesitant on making a change to shared code
without a better understand of why.

Dan


On 6/13/18 10:00 AM, Andrey Petushkov wrote:
>> On 13 Jun 2018, at 16:37, Andrew Haley <aph at redhat.com> wrote:
>>
>> On 06/13/2018 01:40 PM, Andrey Petushkov wrote:
>>
>>> It looks like the cause of the
>>> https://bugs.openjdk.java.net/browse/JDK-8153107
>>> <https://bugs.openjdk.java.net/browse/JDK-8153107> problem is that
>>> ObjectSynchronizer::quick_enter implementation leaves the BasicLock
>>> marked as recursive (DHW==0) in case that the lock is inflated and
>>> already taken. As such the routines which first check the recursive
>>> stack locking (e.g. fast_exit) consider this lock as non-inflated
>>> recursive and hence do not decrement _recursions field of
>>> ObjectMonitor, leaving the monitor locked forever. In contrary the
>>> slow path does work the right way: ObjectSynchronizer::slow_enter
>>> first marks stack lock as non-locked and non-recursive (DHW==3) and
>>> then delegates to ObjectMonitor::enter to increment _recursions.
>>> So from the source code prospective this bug looks like
>>> platform-independent, contrary to what’s was found under
>>> https://bugs.openjdk.java.net/browse/JDK-8131715
>>> <https://bugs.openjdk.java.net/browse/JDK-8131715>. Unfortunately I
>>> cannot prove it with an example. The only case it fails for me is
>>> runtime/CreateMirror/ArraysNewInstanceBug.java hotspot jtreg test on
>>> aarch32 openjdk port. The following changes fix the problem:
>> Ouch.  The code is due to 8167501, which was a bug in the Oracle ARM64
>> and ARM32 ports.  This bug was never seen in the AARCH64 open port,
>> so the workaround shouldn't be enabled for it.
>>
>> 8167501: ARMv7 Linux C2 compiler crashes running jtreg harness on MP systems
>> Reviewed-by: dcubed
>> diff -r afd6ae4fec81 -r 71d7ced6c439 hotspot/src/share/vm/runtime/sharedRuntime.cpp
>> --- a/hotspot/src/share/vm/runtime/sharedRuntime.cpp	Thu Nov 03 11:53:20 2016 +0530
>> +++ b/hotspot/src/share/vm/runtime/sharedRuntime.cpp	Thu Nov 03 10:44:17 2016 -0400
>> @@ -1983,8 +1983,10 @@
>> // Handles the uncommon case in locking, i.e., contention or an inflated lock.
>> JRT_BLOCK_ENTRY(void, SharedRuntime::complete_monitor_locking_C(oopDesc* _obj, BasicLock* lock, JavaThread* thread))
>>    // Disable ObjectSynchronizer::quick_enter() in default config
>> -  // on AARCH64 until JDK-8153107 is resolved.
>> -  if (AARCH64_ONLY((SyncFlags & 256) != 0 &&) !SafepointSynchronize::is_synchronizing()) {
>> +  // on AARCH64 and ARM until JDK-8153107 is resolved.
>> +  if (ARM_ONLY((SyncFlags & 256) != 0 &&)
>> +      AARCH64_ONLY((SyncFlags & 256) != 0 &&)
>> +      !SafepointSynchronize::is_synchronizing()) {
>>      // Only try quick_enter() if we're not trying to reach a safepoint
>>
>> Why did you move the code which handles recursive locking counts?  Do
>> you believe that it's incorrect where it is, and if so, why?
> That’s exactly what I’ve been trying to describe. The displaced header shall be assigned 3 when recursive locking happens on inflated lock. At least with the current design of the exit code which uses it as an indicator that inflated lock is present.
> So it seems to me that currently the number of recursive locks on an object is the count of relevant stack lock structures with mark(DHW)!=3 (i.e. first lock + any stack locks prior to inflation) + _recursions field value of the inflated lock structure. The sequence of operations under review was breaking this assumption: any recursion via quick_enter was increasing both number of stack locks with DHW==0 and value of _recursions. While the slow_enter only increased _recursions
> The above is idea I got from reading the code, I hope someone more experienced (@Daniel Daugherty?) can chime in and prove me right or wrong
>
> Andrey
>> -- 
>> Andrew Haley
>> Java Platform Lead Engineer
>> Red Hat UK Ltd. <https://www.redhat.com>
>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671