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

[White, Derek]

Hi Andrey, Andrew,

> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> bounces at openjdk.java.net] On Behalf Of Andrew Haley
 
> 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.

Yes, I've been testing this last week myself (e.g. setting SyncFlags). It's responsible for about a 2% drop in SPECjbb on aarch64 when they added 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?

I'm similarly suspicious of pointing the blame at the shared code. I'll look some more also.

 - Derek