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

[Andrey Petushkov]

So then if you prefer to leave the different logic in shared code between
quick and slow paths I believe the fix for cpu/arm implementation (and
removal of unnecessary workaround for cpu/aarch64) should look like this:

--- CUT ---
diff -r db65921e9a9b src/hotspot/cpu/arm/macroAssembler_arm.cpp
--- a/src/hotspot/cpu/arm/macroAssembler_arm.cpp Thu Jun 07 06:27:09 2018
-0400
+++ b/src/hotspot/cpu/arm/macroAssembler_arm.cpp Thu Jun 14 14:56:38 2018
+0300
@@ -3014,8 +3014,8 @@
   mov(Rscratch, SP);
   sub(Rscratch, Rmark, Rscratch);
   ands(Rscratch, Rscratch, imm);
-  b(done, ne); // exit with failure
-  str(Rscratch, Address(Rbox,
BasicLock::displaced_header_offset_in_bytes())); // set to zero
+  // set to zero if recursive lock, set to non zero otherwise (see
discussion in JDK-8153107)
+  str(Rscratch, Address(Rbox,
BasicLock::displaced_header_offset_in_bytes()));
   b(done);

 #else
@@ -3025,7 +3025,8 @@
   sub(Rscratch, Rmark, SP, eq);
   movs(Rscratch, AsmOperand(Rscratch, lsr,
exact_log2(os::vm_page_size())), eq);
   // If still 'eq' then recursive locking OK
-  str(Rscratch, Address(Rbox,
BasicLock::displaced_header_offset_in_bytes()), eq); // set to zero
+  // set to zero if recursive lock, set to non zero otherwise (see
discussion in JDK-8153107)
+  str(Rscratch, Address(Rbox,
BasicLock::displaced_header_offset_in_bytes()));
   b(done);
 #endif

diff -r db65921e9a9b src/hotspot/share/runtime/sharedRuntime.cpp
--- a/src/hotspot/share/runtime/sharedRuntime.cpp Thu Jun 07 06:27:09 2018
-0400
+++ b/src/hotspot/share/runtime/sharedRuntime.cpp Thu Jun 14 14:56:38 2018
+0300
@@ -1983,11 +1983,7 @@

 // 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 and ARM until JDK-8153107 is resolved.
-  if (ARM_ONLY((SyncFlags & 256) != 0 &&)
-      AARCH64_ONLY((SyncFlags & 256) != 0 &&)
-      !SafepointSynchronize::is_synchronizing()) {
+  if (!SafepointSynchronize::is_synchronizing()) {
     // Only try quick_enter() if we're not trying to reach a safepoint
     // so that the calling thread reaches the safepoint more quickly.
     if (ObjectSynchronizer::quick_enter(_obj, thread, lock)) return;
--- END CUT ---

Tested on jdk10/port-aarch32 codebase

Regards,
Andrey

On Wed, Jun 13, 2018 at 8:26 PM Andrey Petushkov <andrey.petushkov at gmail.com>
wrote:

> Ok, I shall I admit I did not dig deep enough. Yes, there is platform
> specifics which plays a role here. That is: all platforms except Oracle ARM
> (all bitness) and aarch32 fill-in stack lock’s DHW with result of
> same-stack-page-lock probe result. Which is always !=0 when failed.
> Oracle’s ARM and aarch32 do that conditionally, based on whether this test
> actually passes. Hence for the latter platforms the DHW in the stack lock
> really contains random value which occasionally can be 0, and being not
> rewritten (e.g. with markOopDesc::unused_mark(), aka 3) will indicate
> recursive stack lock for ::fast_unlock(). Not sure about those stack
> walkers mentioned in the comment in ::quick_enter(). So, right, it’s
> possible to fix the problem in the platform-specific code only. Leaving up
> to you to decide whether it’s proper design :)
>
> Regards,
> Andrey
>
> > On 13 Jun 2018, at 18:17, Daniel D. Daugherty <
> daniel.daugherty at oracle.com> wrote:
> >
> > Not at my fingertips, but I will look thru my archives.
> >
> > Dan
> >
> >
> > On 6/13/18 11:14 AM, Andrew Haley wrote:
> >> Hi Dan,
> >>
> >> Have you got any torture tests for recursive locking?
> >>
> >
>
>