8153107: Unbalanced recursive locking

classic Classic list List threaded Threaded
35 messages Options
12
Reply | Threaded
Open this post in threaded view
|

8153107: Unbalanced recursive locking

Andrey Petushkov
Dear Hotspot developers,

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:

diff --git a/src/hotspot/share/runtime/sharedRuntime.cpp b/src/hotspot/share/runtime/sharedRuntime.cpp
--- a/src/hotspot/share/runtime/sharedRuntime.cpp
+++ b/src/hotspot/share/runtime/sharedRuntime.cpp
@@ -1985,9 +1985,7 @@
 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;
diff --git a/src/hotspot/share/runtime/synchronizer.cpp b/src/hotspot/share/runtime/synchronizer.cpp
--- a/src/hotspot/share/runtime/synchronizer.cpp
+++ b/src/hotspot/share/runtime/synchronizer.cpp
@@ -220,11 +220,6 @@
     // Case: light contention possibly amenable to TLE
     // Case: TLE inimical operations such as nested/recursive synchronization
 
-    if (owner == Self) {
-      m->_recursions++;
-      return true;
-    }
-
     // This Java Monitor is inflated so obj's header will never be
     // displaced to this thread's BasicLock. Make the displaced header
     // non-NULL so this BasicLock is not seen as recursive nor as
@@ -237,6 +232,11 @@
     // and last are the inflated Java Monitor (ObjectMonitor) checks.
     lock->set_displaced_header(markOopDesc::unused_mark());
 
+    if (owner == Self) {
+      m->_recursions++;
+      return true;
+    }
+
     if (owner == NULL && Atomic::replace_if_null(Self, &(m->_owner))) {
       assert(m->_recursions == 0, "invariant");
       assert(m->_owner == Self, "invariant");

Regards,
Andrey
Reply | Threaded
Open this post in threaded view
|

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

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.

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?

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

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

Andrey Petushkov

> On 13 Jun 2018, at 16:37, Andrew Haley <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: [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 <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Andrew Haley
On 06/13/2018 03:54 PM, Daniel D. Daugherty wrote:
> 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.

Yeah, my thoughts exactly.  It doesn't help that the platform-
specific variants of quick_enter() are all subtly different.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

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

White, Derek
In reply to this post by Andrew Haley
Hi Andrey, Andrew,

> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> [hidden email]] 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

Reply | Threaded
Open this post in threaded view
|

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

Andrew Haley
In reply to this post by Daniel D. Daugherty
Hi Dan,

Have you got any torture tests for recursive locking?

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
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?
>

Reply | Threaded
Open this post in threaded view
|

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

Andrey Petushkov
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 <[hidden email]> 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?
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: [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 <[hidden email]>
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 <
> [hidden email]> 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?
> >>
> >
>
>
Reply | Threaded
Open this post in threaded view
|

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

Andrew Haley
On 06/14/2018 12:59 PM, Andrey Petushkov wrote:
> 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:

Something awful happened to the formatting of your mail.  Can you put
it up somewhere we can see the patch?

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

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

Andrey Petushkov
Hm, strange. It displays well for me in the archives page. Anyway, I've put
the webrev at http://cr.openjdk.java.net/~apetushkov/8153107/

On Thu, Jun 14, 2018 at 3:15 PM Andrew Haley <[hidden email]> wrote:

> On 06/14/2018 12:59 PM, Andrey Petushkov wrote:
> > 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:
>
> Something awful happened to the formatting of your mail.  Can you put
> it up somewhere we can see the patch?
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332
> <https://maps.google.com/?q=6035+332&entry=gmail&source=g>F A671
>
Reply | Threaded
Open this post in threaded view
|

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

White, Derek
Hi Andrey,

I like this version.

My only suggestion is minor - the "ands" at line 3016 could be a simple "and". It causes no harm but could be confusing to a reader:

3016   ands(Rscratch, Rscratch, imm);

No need to see a new webrev.
- Derek

> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> [hidden email]] On Behalf Of Andrey Petushkov
> Sent: Thursday, June 14, 2018 8:25 AM
> To: Andrew Haley <[hidden email]>
> Cc: [hidden email]; aarch64-port-
> [hidden email]; AArch32 Port Project <aarch32-port-
> [hidden email]>
> Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive locking
>
> External Email
>
> Hm, strange. It displays well for me in the archives page. Anyway, I've put
> the webrev at http://cr.openjdk.java.net/~apetushkov/8153107/
>
> On Thu, Jun 14, 2018 at 3:15 PM Andrew Haley <[hidden email]> wrote:
>
> > On 06/14/2018 12:59 PM, Andrey Petushkov wrote:
> > > 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:
> >
> > Something awful happened to the formatting of your mail.  Can you put
> > it up somewhere we can see the patch?
> >
> > --
> > Andrew Haley
> > Java Platform Lead Engineer
> > Red Hat UK Ltd. <https://www.redhat.com>
> > EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332
> > <https://maps.google.com/?q=6035+332&entry=gmail&source=g>F A671
> >
Reply | Threaded
Open this post in threaded view
|

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

Andrey Petushkov
Hi Derek,

that shall be ands since it expected to leave the result in the flags
register, see the cmpFastLock instruct in aarch64.ad
to my taste this version is error-prone, since it's quite hard to deduce
that contract FastLock node involves writing of non 0 value into DHW of
stack lock when it has failed. however I don't insist. so if everyone
agrees could someone please commit. sorry, I don't have necessary status

Regards,
Andrey

On Thu, Jun 14, 2018 at 9:13 PM White, Derek <[hidden email]> wrote:

> Hi Andrey,
>
> I like this version.
>
> My only suggestion is minor - the "ands" at line 3016 could be a simple
> "and". It causes no harm but could be confusing to a reader:
>
> 3016   ands(Rscratch, Rscratch, imm);
>
> No need to see a new webrev.
> - Derek
>
> > -----Original Message-----
> > From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> > [hidden email]] On Behalf Of Andrey Petushkov
> > Sent: Thursday, June 14, 2018 8:25 AM
> > To: Andrew Haley <[hidden email]>
> > Cc: [hidden email]; aarch64-port-
> > [hidden email]; AArch32 Port Project <aarch32-port-
> > [hidden email]>
> > Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive locking
> >
> > External Email
> >
> > Hm, strange. It displays well for me in the archives page. Anyway, I've
> put
> > the webrev at http://cr.openjdk.java.net/~apetushkov/8153107/
> >
> > On Thu, Jun 14, 2018 at 3:15 PM Andrew Haley <[hidden email]> wrote:
> >
> > > On 06/14/2018 12:59 PM, Andrey Petushkov wrote:
> > > > 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:
> > >
> > > Something awful happened to the formatting of your mail.  Can you put
> > > it up somewhere we can see the patch?
> > >
> > > --
> > > Andrew Haley
> > > Java Platform Lead Engineer
> > > Red Hat UK Ltd. <https://www.redhat.com>
> > > EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332
> > > <https://maps.google.com/?q=6035+332&entry=gmail&source=g>F A671
> > >
>
Reply | Threaded
Open this post in threaded view
|

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

White, Derek
Hi Andrey,

It took me looking at the code three times, but I finally saw what you mean about condition code register. OK, looks fine to me.

You still need a (R)eviewer’s OK and then a sponsor.


  *   Derek

From: Andrey Petushkov [mailto:[hidden email]]
Sent: Thursday, June 14, 2018 4:55 PM
To: White, Derek <[hidden email]>
Cc: Andrew Haley <[hidden email]>; [hidden email]; [hidden email]; AArch32 Port Project <[hidden email]>
Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive locking


External Email
Hi Derek,

that shall be ands since it expected to leave the result in the flags register, see the cmpFastLock instruct in aarch64.ad<http://aarch64.ad>
to my taste this version is error-prone, since it's quite hard to deduce that contract FastLock node involves writing of non 0 value into DHW of stack lock when it has failed. however I don't insist. so if everyone agrees could someone please commit. sorry, I don't have necessary status


Regards,
Andrey

On Thu, Jun 14, 2018 at 9:13 PM White, Derek <[hidden email]<mailto:[hidden email]>> wrote:
Hi Andrey,

I like this version.

My only suggestion is minor - the "ands" at line 3016 could be a simple "and". It causes no harm but could be confusing to a reader:

3016   ands(Rscratch, Rscratch, imm);

No need to see a new webrev.
- Derek

> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-<mailto:hotspot-runtime-dev->
> [hidden email]<mailto:[hidden email]>] On Behalf Of Andrey Petushkov
> Sent: Thursday, June 14, 2018 8:25 AM
> To: Andrew Haley <[hidden email]<mailto:[hidden email]>>
> Cc: [hidden email]<mailto:[hidden email]>; aarch64-port-
> [hidden email]<mailto:[hidden email]>; AArch32 Port Project <aarch32-port-
> [hidden email]<mailto:[hidden email]>>
> Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive locking
>
> External Email
>
> Hm, strange. It displays well for me in the archives page. Anyway, I've put
> the webrev at http://cr.openjdk.java.net/~apetushkov/8153107/
>
> On Thu, Jun 14, 2018 at 3:15 PM Andrew Haley <[hidden email]<mailto:[hidden email]>> wrote:
>
> > On 06/14/2018 12:59 PM, Andrey Petushkov wrote:
> > > 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:
> >
> > Something awful happened to the formatting of your mail.  Can you put
> > it up somewhere we can see the patch?
> >
> > --
> > Andrew Haley
> > Java Platform Lead Engineer
> > Red Hat UK Ltd. <https://www.redhat.com>
> > EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332
> > <https://maps.google.com/?q=6035+332&entry=gmail&source=g>F A671
> >
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
 > http://cr.openjdk.java.net/~apetushkov/8153107/

src/hotspot/share/runtime/sharedRuntime.cpp
     No comments.

src/hotspot/cpu/arm/macroAssembler_arm.cpp
     Looks correct. Here's my analysis:

     The X64 version of this code does this:

     src/hotspot/cpu/x86/macroAssembler_x86.cpp:

       // Recursive locking.
       // The object is stack-locked: markword contains stack pointer to
BasicLock.
       // Locked by current thread if difference with current SP is less
than one page.
       subptr(tmpReg, rsp);
       // Next instruction set ZFlag == 1 (Success) if difference is
less then one page.
       andptr(tmpReg, (int32_t) (NOT_LP64(0xFFFFF003) LP64_ONLY(7 -
os::vm_page_size())) );
       movptr(Address(boxReg, 0), tmpReg);
       if (counters != NULL) {
         cond_inc32(Assembler::equal,
ExternalAddress((address)counters->fast_path_entry_count_addr()));
       }
       jmp(DONE_LABEL);

     This code path always updates "Address(boxReg, 0)" with the
     "tmpReg" value so the markword always gets updated with a zero
     or non-zero value.

     The previous AMD64 code did this:

       L3011: #ifdef AARCH64
       L3012:   intptr_t mask = ((intptr_t)3) -
((intptr_t)os::vm_page_size());
       L3013:   Assembler::LogicalImmediate imm(mask, false);
       L3014:   mov(Rscratch, SP);
       L3015:   sub(Rscratch, Rmark, Rscratch);
       L3016:   ands(Rscratch, Rscratch, imm);
       L3017:   b(done, ne); // exit with failure
       L3018:   str(Rscratch, Address(Rbox,
BasicLock::displaced_header_offset_in_bytes())); // set to zero
       L3019:   b(done);
       L3020:
       L3021: #else
       L3022:   // -1- test low 2 bits
       L3023:   movs(Rscratch, AsmOperand(Rmark, lsl, 30));
       L3024:   // -2- test (hdr - SP) if the low two bits are 0
       L3025:   sub(Rscratch, Rmark, SP, eq);
       L3026:   movs(Rscratch, AsmOperand(Rscratch, lsr,
exact_log2(os::vm_page_size())), eq);
       L3027:   // If still 'eq' then recursive locking OK
       L3028:   str(Rscratch, Address(Rbox,
BasicLock::displaced_header_offset_in_bytes()), eq); // set to zero
       L3029:   b(done);
       L3030: #endif

     For the AARCH64 version, the key bug is the "b(done, ne); // exit
with failure"
     which results in the markword not getting set to a specific value.
     Someone mentioned that we're getting a random value here and that
     contributes to the intermittent nature of the hang. Removing L3017
     so that we always set the markword makes perfect sense.

     For the non-AARCH64 version, the key bug is a little more obscure
     (at least to me): "str(Rscratch, Address(Rbox, ...), eq); // set to
zero".
     I don't really know ARM assembly, but I'm guessing that the 'eq'
parameter
     controls whether the store of Rscratch into "Address(Rbox, ...)"
occurs or
     not. Removing the "eq" parameter in the fix so that we always set the
     markword makes perfect sense.


So thumbs up on the fix and I can sponsor it. Please provide me with an
importable changeset and I'll take care of that after confirmation that
the ARM folks have tested the fix and are happy with it.

Dan


On 6/14/18 6:08 PM, White, Derek wrote:

> Hi Andrey,
>
> It took me looking at the code three times, but I finally saw what you mean about condition code register. OK, looks fine to me.
>
> You still need a (R)eviewer’s OK and then a sponsor.
>
>
>    *   Derek
>
> From: Andrey Petushkov [mailto:[hidden email]]
> Sent: Thursday, June 14, 2018 4:55 PM
> To: White, Derek <[hidden email]>
> Cc: Andrew Haley <[hidden email]>; [hidden email]; [hidden email]; AArch32 Port Project <[hidden email]>
> Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive locking
>
>
> External Email
> Hi Derek,
>
> that shall be ands since it expected to leave the result in the flags register, see the cmpFastLock instruct in aarch64.ad<http://aarch64.ad>
> to my taste this version is error-prone, since it's quite hard to deduce that contract FastLock node involves writing of non 0 value into DHW of stack lock when it has failed. however I don't insist. so if everyone agrees could someone please commit. sorry, I don't have necessary status
>
>
> Regards,
> Andrey
>
> On Thu, Jun 14, 2018 at 9:13 PM White, Derek <[hidden email]<mailto:[hidden email]>> wrote:
> Hi Andrey,
>
> I like this version.
>
> My only suggestion is minor - the "ands" at line 3016 could be a simple "and". It causes no harm but could be confusing to a reader:
>
> 3016   ands(Rscratch, Rscratch, imm);
>
> No need to see a new webrev.
> - Derek
>
>> -----Original Message-----
>> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-<mailto:hotspot-runtime-dev->
>> [hidden email]<mailto:[hidden email]>] On Behalf Of Andrey Petushkov
>> Sent: Thursday, June 14, 2018 8:25 AM
>> To: Andrew Haley <[hidden email]<mailto:[hidden email]>>
>> Cc: [hidden email]<mailto:[hidden email]>; aarch64-port-
>> [hidden email]<mailto:[hidden email]>; AArch32 Port Project <aarch32-port-
>> [hidden email]<mailto:[hidden email]>>
>> Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive locking
>>
>> External Email
>>
>> Hm, strange. It displays well for me in the archives page. Anyway, I've put
>> the webrev at http://cr.openjdk.java.net/~apetushkov/8153107/
>>
>> On Thu, Jun 14, 2018 at 3:15 PM Andrew Haley <[hidden email]<mailto:[hidden email]>> wrote:
>>
>>> On 06/14/2018 12:59 PM, Andrey Petushkov wrote:
>>>> 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:
>>> Something awful happened to the formatting of your mail.  Can you put
>>> it up somewhere we can see the patch?
>>>
>>> --
>>> Andrew Haley
>>> Java Platform Lead Engineer
>>> Red Hat UK Ltd. <https://www.redhat.com>
>>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332
>>> <https://maps.google.com/?q=6035+332&entry=gmail&source=g>F A671
>>>

Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
Andrey,

I'm about to send the following changeset thru Mach5 testing:

$ hg log -v -r tip
changeset:   50592:dc9e3b56c8ab
tag:         tip
user:        apetushkov
date:        Fri Jun 15 13:57:32 2018 -0400
files:       src/hotspot/cpu/arm/macroAssembler_arm.cpp
src/hotspot/share/runtime/sharedRuntime.cpp
description:
8153107: enabling ObjectSynchronizer::quick_enter() on ARM64 causes hangs
Summary: Always set the markword for recursive monitors in
MacroAssembler::fast_lock().
Reviewed-by: aph, drwhite, dcubed


I don't expect any problems, but I'm paranoid so...

Dan


On 6/15/18 1:29 PM, Daniel D. Daugherty wrote:

> > http://cr.openjdk.java.net/~apetushkov/8153107/
>
> src/hotspot/share/runtime/sharedRuntime.cpp
>     No comments.
>
> src/hotspot/cpu/arm/macroAssembler_arm.cpp
>     Looks correct. Here's my analysis:
>
>     The X64 version of this code does this:
>
>     src/hotspot/cpu/x86/macroAssembler_x86.cpp:
>
>       // Recursive locking.
>       // The object is stack-locked: markword contains stack pointer
> to BasicLock.
>       // Locked by current thread if difference with current SP is
> less than one page.
>       subptr(tmpReg, rsp);
>       // Next instruction set ZFlag == 1 (Success) if difference is
> less then one page.
>       andptr(tmpReg, (int32_t) (NOT_LP64(0xFFFFF003) LP64_ONLY(7 -
> os::vm_page_size())) );
>       movptr(Address(boxReg, 0), tmpReg);
>       if (counters != NULL) {
>         cond_inc32(Assembler::equal,
> ExternalAddress((address)counters->fast_path_entry_count_addr()));
>       }
>       jmp(DONE_LABEL);
>
>     This code path always updates "Address(boxReg, 0)" with the
>     "tmpReg" value so the markword always gets updated with a zero
>     or non-zero value.
>
>     The previous AMD64 code did this:
>
>       L3011: #ifdef AARCH64
>       L3012:   intptr_t mask = ((intptr_t)3) -
> ((intptr_t)os::vm_page_size());
>       L3013:   Assembler::LogicalImmediate imm(mask, false);
>       L3014:   mov(Rscratch, SP);
>       L3015:   sub(Rscratch, Rmark, Rscratch);
>       L3016:   ands(Rscratch, Rscratch, imm);
>       L3017:   b(done, ne); // exit with failure
>       L3018:   str(Rscratch, Address(Rbox,
> BasicLock::displaced_header_offset_in_bytes())); // set to zero
>       L3019:   b(done);
>       L3020:
>       L3021: #else
>       L3022:   // -1- test low 2 bits
>       L3023:   movs(Rscratch, AsmOperand(Rmark, lsl, 30));
>       L3024:   // -2- test (hdr - SP) if the low two bits are 0
>       L3025:   sub(Rscratch, Rmark, SP, eq);
>       L3026:   movs(Rscratch, AsmOperand(Rscratch, lsr,
> exact_log2(os::vm_page_size())), eq);
>       L3027:   // If still 'eq' then recursive locking OK
>       L3028:   str(Rscratch, Address(Rbox,
> BasicLock::displaced_header_offset_in_bytes()), eq); // set to zero
>       L3029:   b(done);
>       L3030: #endif
>
>     For the AARCH64 version, the key bug is the "b(done, ne); // exit
> with failure"
>     which results in the markword not getting set to a specific value.
>     Someone mentioned that we're getting a random value here and that
>     contributes to the intermittent nature of the hang. Removing L3017
>     so that we always set the markword makes perfect sense.
>
>     For the non-AARCH64 version, the key bug is a little more obscure
>     (at least to me): "str(Rscratch, Address(Rbox, ...), eq); // set
> to zero".
>     I don't really know ARM assembly, but I'm guessing that the 'eq'
> parameter
>     controls whether the store of Rscratch into "Address(Rbox, ...)"
> occurs or
>     not. Removing the "eq" parameter in the fix so that we always set the
>     markword makes perfect sense.
>
>
> So thumbs up on the fix and I can sponsor it. Please provide me with an
> importable changeset and I'll take care of that after confirmation that
> the ARM folks have tested the fix and are happy with it.
>
> Dan
>
>
> On 6/14/18 6:08 PM, White, Derek wrote:
>> Hi Andrey,
>>
>> It took me looking at the code three times, but I finally saw what
>> you mean about condition code register. OK, looks fine to me.
>>
>> You still need a (R)eviewer’s OK and then a sponsor.
>>
>>
>>    *   Derek
>>
>> From: Andrey Petushkov [mailto:[hidden email]]
>> Sent: Thursday, June 14, 2018 4:55 PM
>> To: White, Derek <[hidden email]>
>> Cc: Andrew Haley <[hidden email]>;
>> [hidden email];
>> [hidden email]; AArch32 Port Project
>> <[hidden email]>
>> Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive locking
>>
>>
>> External Email
>> Hi Derek,
>>
>> that shall be ands since it expected to leave the result in the flags
>> register, see the cmpFastLock instruct in aarch64.ad<http://aarch64.ad>
>> to my taste this version is error-prone, since it's quite hard to
>> deduce that contract FastLock node involves writing of non 0 value
>> into DHW of stack lock when it has failed. however I don't insist. so
>> if everyone agrees could someone please commit. sorry, I don't have
>> necessary status
>>
>>
>> Regards,
>> Andrey
>>
>> On Thu, Jun 14, 2018 at 9:13 PM White, Derek
>> <[hidden email]<mailto:[hidden email]>> wrote:
>> Hi Andrey,
>>
>> I like this version.
>>
>> My only suggestion is minor - the "ands" at line 3016 could be a
>> simple "and". It causes no harm but could be confusing to a reader:
>>
>> 3016   ands(Rscratch, Rscratch, imm);
>>
>> No need to see a new webrev.
>> - Derek
>>
>>> -----Original Message-----
>>> From: hotspot-runtime-dev
>>> [mailto:hotspot-runtime-dev-<mailto:hotspot-runtime-dev->
>>> [hidden email]<mailto:[hidden email]>] On Behalf
>>> Of Andrey Petushkov
>>> Sent: Thursday, June 14, 2018 8:25 AM
>>> To: Andrew Haley <[hidden email]<mailto:[hidden email]>>
>>> Cc:
>>> [hidden email]<mailto:[hidden email]>;
>>> aarch64-port-
>>> [hidden email]<mailto:[hidden email]>; AArch32 Port
>>> Project <aarch32-port-
>>> [hidden email]<mailto:[hidden email]>>
>>> Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive locking
>>>
>>> External Email
>>>
>>> Hm, strange. It displays well for me in the archives page. Anyway,
>>> I've put
>>> the webrev at http://cr.openjdk.java.net/~apetushkov/8153107/
>>>
>>> On Thu, Jun 14, 2018 at 3:15 PM Andrew Haley
>>> <[hidden email]<mailto:[hidden email]>> wrote:
>>>
>>>> On 06/14/2018 12:59 PM, Andrey Petushkov wrote:
>>>>> 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:
>>>> Something awful happened to the formatting of your mail.  Can you put
>>>> it up somewhere we can see the patch?
>>>>
>>>> --
>>>> Andrew Haley
>>>> Java Platform Lead Engineer
>>>> Red Hat UK Ltd. <https://www.redhat.com>
>>>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332
>>>> <https://maps.google.com/?q=6035+332&entry=gmail&source=g>F A671
>>>>
>
>

Reply | Threaded
Open this post in threaded view
|

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

Andrey Petushkov
Hi Dan,

Thank you for sponsorship and yes,
> I don't really know ARM assembly, but I'm guessing that the 'eq'
> parameter
>     controls whether the store of Rscratch into "Address(Rbox, ...)"
> occurs or
>     not
You guess is perfectly correct!

Thanks again,
Andrey


On Fri, Jun 15, 2018 at 9:00 PM Daniel D. Daugherty <
[hidden email]> wrote:

> Andrey,
>
> I'm about to send the following changeset thru Mach5 testing:
>
> $ hg log -v -r tip
> changeset:   50592:dc9e3b56c8ab
> tag:         tip
> user:        apetushkov
> date:        Fri Jun 15 13:57:32 2018 -0400
> files:       src/hotspot/cpu/arm/macroAssembler_arm.cpp
> src/hotspot/share/runtime/sharedRuntime.cpp
> description:
> 8153107: enabling ObjectSynchronizer::quick_enter() on ARM64 causes hangs
> Summary: Always set the markword for recursive monitors in
> MacroAssembler::fast_lock().
> Reviewed-by: aph, drwhite, dcubed
>
>
> I don't expect any problems, but I'm paranoid so...
>
> Dan
>
>
> On 6/15/18 1:29 PM, Daniel D. Daugherty wrote:
> > > http://cr.openjdk.java.net/~apetushkov/8153107/
> >
> > src/hotspot/share/runtime/sharedRuntime.cpp
> >     No comments.
> >
> > src/hotspot/cpu/arm/macroAssembler_arm.cpp
> >     Looks correct. Here's my analysis:
> >
> >     The X64 version of this code does this:
> >
> >     src/hotspot/cpu/x86/macroAssembler_x86.cpp:
> >
> >       // Recursive locking.
> >       // The object is stack-locked: markword contains stack pointer
> > to BasicLock.
> >       // Locked by current thread if difference with current SP is
> > less than one page.
> >       subptr(tmpReg, rsp);
> >       // Next instruction set ZFlag == 1 (Success) if difference is
> > less then one page.
> >       andptr(tmpReg, (int32_t) (NOT_LP64(0xFFFFF003) LP64_ONLY(7 -
> > os::vm_page_size())) );
> >       movptr(Address(boxReg, 0), tmpReg);
> >       if (counters != NULL) {
> >         cond_inc32(Assembler::equal,
> > ExternalAddress((address)counters->fast_path_entry_count_addr()));
> >       }
> >       jmp(DONE_LABEL);
> >
> >     This code path always updates "Address(boxReg, 0)" with the
> >     "tmpReg" value so the markword always gets updated with a zero
> >     or non-zero value.
> >
> >     The previous AMD64 code did this:
> >
> >       L3011: #ifdef AARCH64
> >       L3012:   intptr_t mask = ((intptr_t)3) -
> > ((intptr_t)os::vm_page_size());
> >       L3013:   Assembler::LogicalImmediate imm(mask, false);
> >       L3014:   mov(Rscratch, SP);
> >       L3015:   sub(Rscratch, Rmark, Rscratch);
> >       L3016:   ands(Rscratch, Rscratch, imm);
> >       L3017:   b(done, ne); // exit with failure
> >       L3018:   str(Rscratch, Address(Rbox,
> > BasicLock::displaced_header_offset_in_bytes())); // set to zero
> >       L3019:   b(done);
> >       L3020:
> >       L3021: #else
> >       L3022:   // -1- test low 2 bits
> >       L3023:   movs(Rscratch, AsmOperand(Rmark, lsl, 30));
> >       L3024:   // -2- test (hdr - SP) if the low two bits are 0
> >       L3025:   sub(Rscratch, Rmark, SP, eq);
> >       L3026:   movs(Rscratch, AsmOperand(Rscratch, lsr,
> > exact_log2(os::vm_page_size())), eq);
> >       L3027:   // If still 'eq' then recursive locking OK
> >       L3028:   str(Rscratch, Address(Rbox,
> > BasicLock::displaced_header_offset_in_bytes()), eq); // set to zero
> >       L3029:   b(done);
> >       L3030: #endif
> >
> >     For the AARCH64 version, the key bug is the "b(done, ne); // exit
> > with failure"
> >     which results in the markword not getting set to a specific value.
> >     Someone mentioned that we're getting a random value here and that
> >     contributes to the intermittent nature of the hang. Removing L3017
> >     so that we always set the markword makes perfect sense.
> >
> >     For the non-AARCH64 version, the key bug is a little more obscure
> >     (at least to me): "str(Rscratch, Address(Rbox, ...), eq); // set
> > to zero".
> >     I don't really know ARM assembly, but I'm guessing that the 'eq'
> > parameter
> >     controls whether the store of Rscratch into "Address(Rbox, ...)"
> > occurs or
> >     not. Removing the "eq" parameter in the fix so that we always set the
> >     markword makes perfect sense.
> >
> >
> > So thumbs up on the fix and I can sponsor it. Please provide me with an
> > importable changeset and I'll take care of that after confirmation that
> > the ARM folks have tested the fix and are happy with it.
> >
> > Dan
> >
> >
> > On 6/14/18 6:08 PM, White, Derek wrote:
> >> Hi Andrey,
> >>
> >> It took me looking at the code three times, but I finally saw what
> >> you mean about condition code register. OK, looks fine to me.
> >>
> >> You still need a (R)eviewer’s OK and then a sponsor.
> >>
> >>
> >>    *   Derek
> >>
> >> From: Andrey Petushkov [mailto:[hidden email]]
> >> Sent: Thursday, June 14, 2018 4:55 PM
> >> To: White, Derek <[hidden email]>
> >> Cc: Andrew Haley <[hidden email]>;
> >> [hidden email];
> >> [hidden email]; AArch32 Port Project
> >> <[hidden email]>
> >> Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive locking
> >>
> >>
> >> External Email
> >> Hi Derek,
> >>
> >> that shall be ands since it expected to leave the result in the flags
> >> register, see the cmpFastLock instruct in aarch64.ad<http://aarch64.ad>
> >> to my taste this version is error-prone, since it's quite hard to
> >> deduce that contract FastLock node involves writing of non 0 value
> >> into DHW of stack lock when it has failed. however I don't insist. so
> >> if everyone agrees could someone please commit. sorry, I don't have
> >> necessary status
> >>
> >>
> >> Regards,
> >> Andrey
> >>
> >> On Thu, Jun 14, 2018 at 9:13 PM White, Derek
> >> <[hidden email]<mailto:[hidden email]>> wrote:
> >> Hi Andrey,
> >>
> >> I like this version.
> >>
> >> My only suggestion is minor - the "ands" at line 3016 could be a
> >> simple "and". It causes no harm but could be confusing to a reader:
> >>
> >> 3016   ands(Rscratch, Rscratch, imm);
> >>
> >> No need to see a new webrev.
> >> - Derek
> >>
> >>> -----Original Message-----
> >>> From: hotspot-runtime-dev
> >>> [mailto:hotspot-runtime-dev-<mailto:hotspot-runtime-dev->
> >>> [hidden email]<mailto:[hidden email]>] On Behalf
> >>> Of Andrey Petushkov
> >>> Sent: Thursday, June 14, 2018 8:25 AM
> >>> To: Andrew Haley <[hidden email]<mailto:[hidden email]>>
> >>> Cc:
> >>> [hidden email]<mailto:
> [hidden email]>;
> >>> aarch64-port-
> >>> [hidden email]<mailto:[hidden email]>; AArch32 Port
> >>> Project <aarch32-port-
> >>> [hidden email]<mailto:[hidden email]>>
> >>> Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive locking
> >>>
> >>> External Email
> >>>
> >>> Hm, strange. It displays well for me in the archives page. Anyway,
> >>> I've put
> >>> the webrev at http://cr.openjdk.java.net/~apetushkov/8153107/
> >>>
> >>> On Thu, Jun 14, 2018 at 3:15 PM Andrew Haley
> >>> <[hidden email]<mailto:[hidden email]>> wrote:
> >>>
> >>>> On 06/14/2018 12:59 PM, Andrey Petushkov wrote:
> >>>>> 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:
> >>>> Something awful happened to the formatting of your mail.  Can you put
> >>>> it up somewhere we can see the patch?
> >>>>
> >>>> --
> >>>> Andrew Haley
> >>>> Java Platform Lead Engineer
> >>>> Red Hat UK Ltd. <https://www.redhat.com>
> >>>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332
> >>>> <https://maps.google.com/?q=6035+332&entry=gmail&source=g>F A671
> >>>>
> >
> >
>
>
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
Hi Andrey,

I updated the bug report with the Mach5 results so the only thing I need
now is confirmation that ARM testing went fine. So whenever you or aph
or both have some ARM test results we should be good to go.

In particular it would be good to verify that your test that reproduces
this hang no longer reproduces the hang in some relatively high number
of iterations. So if your test repros the hang in 5/100 runs, I would
be happy if the hang didn't repro in a 1000 runs.

Dan


On 6/16/18 12:11 PM, Andrey Petushkov wrote:

> Hi Dan,
>
> Thank you for sponsorship and yes,
> > I don't really know ARM assembly, but I'm guessing that the 'eq'
> > parameter
> >     controls whether the store of Rscratch into "Address(Rbox, ...)"
> > occurs or
> >     not
> You guess is perfectly correct!
>
> Thanks again,
> Andrey
>
>
> On Fri, Jun 15, 2018 at 9:00 PM Daniel D. Daugherty
> <[hidden email] <mailto:[hidden email]>> wrote:
>
>     Andrey,
>
>     I'm about to send the following changeset thru Mach5 testing:
>
>     $ hg log -v -r tip
>     changeset:   50592:dc9e3b56c8ab
>     tag:         tip
>     user:        apetushkov
>     date:        Fri Jun 15 13:57:32 2018 -0400
>     files:       src/hotspot/cpu/arm/macroAssembler_arm.cpp
>     src/hotspot/share/runtime/sharedRuntime.cpp
>     description:
>     8153107: enabling ObjectSynchronizer::quick_enter() on ARM64
>     causes hangs
>     Summary: Always set the markword for recursive monitors in
>     MacroAssembler::fast_lock().
>     Reviewed-by: aph, drwhite, dcubed
>
>
>     I don't expect any problems, but I'm paranoid so...
>
>     Dan
>
>
>     On 6/15/18 1:29 PM, Daniel D. Daugherty wrote:
>     > > http://cr.openjdk.java.net/~apetushkov/8153107/
>     <http://cr.openjdk.java.net/%7Eapetushkov/8153107/>
>     >
>     > src/hotspot/share/runtime/sharedRuntime.cpp
>     >     No comments.
>     >
>     > src/hotspot/cpu/arm/macroAssembler_arm.cpp
>     >     Looks correct. Here's my analysis:
>     >
>     >     The X64 version of this code does this:
>     >
>     >     src/hotspot/cpu/x86/macroAssembler_x86.cpp:
>     >
>     >       // Recursive locking.
>     >       // The object is stack-locked: markword contains stack
>     pointer
>     > to BasicLock.
>     >       // Locked by current thread if difference with current SP is
>     > less than one page.
>     >       subptr(tmpReg, rsp);
>     >       // Next instruction set ZFlag == 1 (Success) if difference is
>     > less then one page.
>     >       andptr(tmpReg, (int32_t) (NOT_LP64(0xFFFFF003) LP64_ONLY(7 -
>     > os::vm_page_size())) );
>     >       movptr(Address(boxReg, 0), tmpReg);
>     >       if (counters != NULL) {
>     >         cond_inc32(Assembler::equal,
>     > ExternalAddress((address)counters->fast_path_entry_count_addr()));
>     >       }
>     >       jmp(DONE_LABEL);
>     >
>     >     This code path always updates "Address(boxReg, 0)" with the
>     >     "tmpReg" value so the markword always gets updated with a zero
>     >     or non-zero value.
>     >
>     >     The previous AMD64 code did this:
>     >
>     >       L3011: #ifdef AARCH64
>     >       L3012:   intptr_t mask = ((intptr_t)3) -
>     > ((intptr_t)os::vm_page_size());
>     >       L3013:   Assembler::LogicalImmediate imm(mask, false);
>     >       L3014:   mov(Rscratch, SP);
>     >       L3015:   sub(Rscratch, Rmark, Rscratch);
>     >       L3016:   ands(Rscratch, Rscratch, imm);
>     >       L3017:   b(done, ne); // exit with failure
>     >       L3018:   str(Rscratch, Address(Rbox,
>     > BasicLock::displaced_header_offset_in_bytes())); // set to zero
>     >       L3019:   b(done);
>     >       L3020:
>     >       L3021: #else
>     >       L3022:   // -1- test low 2 bits
>     >       L3023:   movs(Rscratch, AsmOperand(Rmark, lsl, 30));
>     >       L3024:   // -2- test (hdr - SP) if the low two bits are 0
>     >       L3025:   sub(Rscratch, Rmark, SP, eq);
>     >       L3026:   movs(Rscratch, AsmOperand(Rscratch, lsr,
>     > exact_log2(os::vm_page_size())), eq);
>     >       L3027:   // If still 'eq' then recursive locking OK
>     >       L3028:   str(Rscratch, Address(Rbox,
>     > BasicLock::displaced_header_offset_in_bytes()), eq); // set to zero
>     >       L3029:   b(done);
>     >       L3030: #endif
>     >
>     >     For the AARCH64 version, the key bug is the "b(done, ne); //
>     exit
>     > with failure"
>     >     which results in the markword not getting set to a specific
>     value.
>     >     Someone mentioned that we're getting a random value here and
>     that
>     >     contributes to the intermittent nature of the hang. Removing
>     L3017
>     >     so that we always set the markword makes perfect sense.
>     >
>     >     For the non-AARCH64 version, the key bug is a little more
>     obscure
>     >     (at least to me): "str(Rscratch, Address(Rbox, ...), eq); //
>     set
>     > to zero".
>     >     I don't really know ARM assembly, but I'm guessing that the
>     'eq'
>     > parameter
>     >     controls whether the store of Rscratch into "Address(Rbox,
>     ...)"
>     > occurs or
>     >     not. Removing the "eq" parameter in the fix so that we
>     always set the
>     >     markword makes perfect sense.
>     >
>     >
>     > So thumbs up on the fix and I can sponsor it. Please provide me
>     with an
>     > importable changeset and I'll take care of that after
>     confirmation that
>     > the ARM folks have tested the fix and are happy with it.
>     >
>     > Dan
>     >
>     >
>     > On 6/14/18 6:08 PM, White, Derek wrote:
>     >> Hi Andrey,
>     >>
>     >> It took me looking at the code three times, but I finally saw what
>     >> you mean about condition code register. OK, looks fine to me.
>     >>
>     >> You still need a (R)eviewer’s OK and then a sponsor.
>     >>
>     >>
>     >>    *   Derek
>     >>
>     >> From: Andrey Petushkov [mailto:[hidden email]
>     <mailto:[hidden email]>]
>     >> Sent: Thursday, June 14, 2018 4:55 PM
>     >> To: White, Derek <[hidden email]
>     <mailto:[hidden email]>>
>     >> Cc: Andrew Haley <[hidden email] <mailto:[hidden email]>>;
>     >> [hidden email]
>     <mailto:[hidden email]>;
>     >> [hidden email]
>     <mailto:[hidden email]>; AArch32 Port Project
>     >> <[hidden email]
>     <mailto:[hidden email]>>
>     >> Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive
>     locking
>     >>
>     >>
>     >> External Email
>     >> Hi Derek,
>     >>
>     >> that shall be ands since it expected to leave the result in the
>     flags
>     >> register, see the cmpFastLock instruct in aarch64.ad
>     <http://aarch64.ad><http://aarch64.ad>
>     >> to my taste this version is error-prone, since it's quite hard to
>     >> deduce that contract FastLock node involves writing of non 0 value
>     >> into DHW of stack lock when it has failed. however I don't
>     insist. so
>     >> if everyone agrees could someone please commit. sorry, I don't
>     have
>     >> necessary status
>     >>
>     >>
>     >> Regards,
>     >> Andrey
>     >>
>     >> On Thu, Jun 14, 2018 at 9:13 PM White, Derek
>     >> <[hidden email]
>     <mailto:[hidden email]><mailto:[hidden email]
>     <mailto:[hidden email]>>> wrote:
>     >> Hi Andrey,
>     >>
>     >> I like this version.
>     >>
>     >> My only suggestion is minor - the "ands" at line 3016 could be a
>     >> simple "and". It causes no harm but could be confusing to a reader:
>     >>
>     >> 3016   ands(Rscratch, Rscratch, imm);
>     >>
>     >> No need to see a new webrev.
>     >> - Derek
>     >>
>     >>> -----Original Message-----
>     >>> From: hotspot-runtime-dev
>     >>> [mailto:hotspot-runtime-dev-
>     <mailto:hotspot-runtime-dev-><mailto:hotspot-runtime-dev-
>     <mailto:hotspot-runtime-dev->>
>     >>> [hidden email]
>     <mailto:[hidden email]><mailto:[hidden email]
>     <mailto:[hidden email]>>] On Behalf
>     >>> Of Andrey Petushkov
>     >>> Sent: Thursday, June 14, 2018 8:25 AM
>     >>> To: Andrew Haley <[hidden email]
>     <mailto:[hidden email]><mailto:[hidden email]
>     <mailto:[hidden email]>>>
>     >>> Cc:
>     >>> [hidden email]
>     <mailto:[hidden email]><mailto:[hidden email]
>     <mailto:[hidden email]>>;
>     >>> aarch64-port-
>     >>> [hidden email]
>     <mailto:[hidden email]><mailto:[hidden email]
>     <mailto:[hidden email]>>; AArch32 Port
>     >>> Project <aarch32-port-
>     >>> [hidden email]
>     <mailto:[hidden email]><mailto:[hidden email]
>     <mailto:[hidden email]>>>
>     >>> Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive
>     locking
>     >>>
>     >>> External Email
>     >>>
>     >>> Hm, strange. It displays well for me in the archives page.
>     Anyway,
>     >>> I've put
>     >>> the webrev at http://cr.openjdk.java.net/~apetushkov/8153107/
>     <http://cr.openjdk.java.net/%7Eapetushkov/8153107/>
>     >>>
>     >>> On Thu, Jun 14, 2018 at 3:15 PM Andrew Haley
>     >>> <[hidden email] <mailto:[hidden email]><mailto:[hidden email]
>     <mailto:[hidden email]>>> wrote:
>     >>>
>     >>>> On 06/14/2018 12:59 PM, Andrey Petushkov wrote:
>     >>>>> 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:
>     >>>> Something awful happened to the formatting of your mail.  Can
>     you put
>     >>>> it up somewhere we can see the patch?
>     >>>>
>     >>>> --
>     >>>> Andrew Haley
>     >>>> Java Platform Lead Engineer
>     >>>> Red Hat UK Ltd. <https://www.redhat.com>
>     >>>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332
>     >>>> <https://maps.google.com/?q=6035+332&entry=gmail&source=g>F A671
>     >>>>
>     >
>     >
>

Reply | Threaded
Open this post in threaded view
|

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

Andrey Petushkov
Hi Dan,

The problem is that I’m not aware of any reproducer for any of the platforms part of main openjdk.
Also Andrew Haley as well as Derek cannot help here I think since they are working with cpu/aarch64 port code, which did not have mentioned problem at all.
Me myself is dealing with cpu/aarch32 port which is not part of main openjdk and jdk10 version of it is not yet even pushed into project-aarch32 openjdk repos. There is jdk9 version but there is no ready made reproducer there. The jdk10 version (on review now) has 100% reproducer for this problem, the runtime/CreateMirror/ArraysNewInstanceBug.java hotspot jtreg test, however I’m not sure anyone wants to try that out. Maybe Ed Nevill could help as a more or less independent party. Ed?

Thank you,
Andrey

> On 16 Jun 2018, at 23:08, Daniel D. Daugherty <[hidden email]> wrote:
>
> Hi Andrey,
>
> I updated the bug report with the Mach5 results so the only thing I need
> now is confirmation that ARM testing went fine. So whenever you or aph
> or both have some ARM test results we should be good to go.
>
> In particular it would be good to verify that your test that reproduces
> this hang no longer reproduces the hang in some relatively high number
> of iterations. So if your test repros the hang in 5/100 runs, I would
> be happy if the hang didn't repro in a 1000 runs.
>
> Dan
>
>
> On 6/16/18 12:11 PM, Andrey Petushkov wrote:
>> Hi Dan,
>>
>> Thank you for sponsorship and yes,
>> > I don't really know ARM assembly, but I'm guessing that the 'eq'
>> > parameter
>> >     controls whether the store of Rscratch into "Address(Rbox, ...)"
>> > occurs or
>> >     not
>> You guess is perfectly correct!
>>
>> Thanks again,
>> Andrey
>>  
>>
>> On Fri, Jun 15, 2018 at 9:00 PM Daniel D. Daugherty <[hidden email] <mailto:[hidden email]>> wrote:
>> Andrey,
>>
>> I'm about to send the following changeset thru Mach5 testing:
>>
>> $ hg log -v -r tip
>> changeset:   50592:dc9e3b56c8ab
>> tag:         tip
>> user:        apetushkov
>> date:        Fri Jun 15 13:57:32 2018 -0400
>> files:       src/hotspot/cpu/arm/macroAssembler_arm.cpp
>> src/hotspot/share/runtime/sharedRuntime.cpp
>> description:
>> 8153107: enabling ObjectSynchronizer::quick_enter() on ARM64 causes hangs
>> Summary: Always set the markword for recursive monitors in
>> MacroAssembler::fast_lock().
>> Reviewed-by: aph, drwhite, dcubed
>>
>>
>> I don't expect any problems, but I'm paranoid so...
>>
>> Dan
>>
>>
>> On 6/15/18 1:29 PM, Daniel D. Daugherty wrote:
>> > > http://cr.openjdk.java.net/~apetushkov/8153107/ <http://cr.openjdk.java.net/%7Eapetushkov/8153107/>
>> >
>> > src/hotspot/share/runtime/sharedRuntime.cpp
>> >     No comments.
>> >
>> > src/hotspot/cpu/arm/macroAssembler_arm.cpp
>> >     Looks correct. Here's my analysis:
>> >
>> >     The X64 version of this code does this:
>> >
>> >     src/hotspot/cpu/x86/macroAssembler_x86.cpp:
>> >
>> >       // Recursive locking.
>> >       // The object is stack-locked: markword contains stack pointer
>> > to BasicLock.
>> >       // Locked by current thread if difference with current SP is
>> > less than one page.
>> >       subptr(tmpReg, rsp);
>> >       // Next instruction set ZFlag == 1 (Success) if difference is
>> > less then one page.
>> >       andptr(tmpReg, (int32_t) (NOT_LP64(0xFFFFF003) LP64_ONLY(7 -
>> > os::vm_page_size())) );
>> >       movptr(Address(boxReg, 0), tmpReg);
>> >       if (counters != NULL) {
>> >         cond_inc32(Assembler::equal,
>> > ExternalAddress((address)counters->fast_path_entry_count_addr()));
>> >       }
>> >       jmp(DONE_LABEL);
>> >
>> >     This code path always updates "Address(boxReg, 0)" with the
>> >     "tmpReg" value so the markword always gets updated with a zero
>> >     or non-zero value.
>> >
>> >     The previous AMD64 code did this:
>> >
>> >       L3011: #ifdef AARCH64
>> >       L3012:   intptr_t mask = ((intptr_t)3) -
>> > ((intptr_t)os::vm_page_size());
>> >       L3013:   Assembler::LogicalImmediate imm(mask, false);
>> >       L3014:   mov(Rscratch, SP);
>> >       L3015:   sub(Rscratch, Rmark, Rscratch);
>> >       L3016:   ands(Rscratch, Rscratch, imm);
>> >       L3017:   b(done, ne); // exit with failure
>> >       L3018:   str(Rscratch, Address(Rbox,
>> > BasicLock::displaced_header_offset_in_bytes())); // set to zero
>> >       L3019:   b(done);
>> >       L3020:
>> >       L3021: #else
>> >       L3022:   // -1- test low 2 bits
>> >       L3023:   movs(Rscratch, AsmOperand(Rmark, lsl, 30));
>> >       L3024:   // -2- test (hdr - SP) if the low two bits are 0
>> >       L3025:   sub(Rscratch, Rmark, SP, eq);
>> >       L3026:   movs(Rscratch, AsmOperand(Rscratch, lsr,
>> > exact_log2(os::vm_page_size())), eq);
>> >       L3027:   // If still 'eq' then recursive locking OK
>> >       L3028:   str(Rscratch, Address(Rbox,
>> > BasicLock::displaced_header_offset_in_bytes()), eq); // set to zero
>> >       L3029:   b(done);
>> >       L3030: #endif
>> >
>> >     For the AARCH64 version, the key bug is the "b(done, ne); // exit
>> > with failure"
>> >     which results in the markword not getting set to a specific value.
>> >     Someone mentioned that we're getting a random value here and that
>> >     contributes to the intermittent nature of the hang. Removing L3017
>> >     so that we always set the markword makes perfect sense.
>> >
>> >     For the non-AARCH64 version, the key bug is a little more obscure
>> >     (at least to me): "str(Rscratch, Address(Rbox, ...), eq); // set
>> > to zero".
>> >     I don't really know ARM assembly, but I'm guessing that the 'eq'
>> > parameter
>> >     controls whether the store of Rscratch into "Address(Rbox, ...)"
>> > occurs or
>> >     not. Removing the "eq" parameter in the fix so that we always set the
>> >     markword makes perfect sense.
>> >
>> >
>> > So thumbs up on the fix and I can sponsor it. Please provide me with an
>> > importable changeset and I'll take care of that after confirmation that
>> > the ARM folks have tested the fix and are happy with it.
>> >
>> > Dan
>> >
>> >
>> > On 6/14/18 6:08 PM, White, Derek wrote:
>> >> Hi Andrey,
>> >>
>> >> It took me looking at the code three times, but I finally saw what
>> >> you mean about condition code register. OK, looks fine to me.
>> >>
>> >> You still need a (R)eviewer’s OK and then a sponsor.
>> >>
>> >>
>> >>    *   Derek
>> >>
>> >> From: Andrey Petushkov [mailto:[hidden email] <mailto:[hidden email]>]
>> >> Sent: Thursday, June 14, 2018 4:55 PM
>> >> To: White, Derek <[hidden email] <mailto:[hidden email]>>
>> >> Cc: Andrew Haley <[hidden email] <mailto:[hidden email]>>;
>> >> [hidden email] <mailto:[hidden email]>;
>> >> [hidden email] <mailto:[hidden email]>; AArch32 Port Project
>> >> <[hidden email] <mailto:[hidden email]>>
>> >> Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive locking
>> >>
>> >>
>> >> External Email
>> >> Hi Derek,
>> >>
>> >> that shall be ands since it expected to leave the result in the flags
>> >> register, see the cmpFastLock instruct in aarch64.ad <http://aarch64.ad/><http://aarch64.ad <http://aarch64.ad/>>
>> >> to my taste this version is error-prone, since it's quite hard to
>> >> deduce that contract FastLock node involves writing of non 0 value
>> >> into DHW of stack lock when it has failed. however I don't insist. so
>> >> if everyone agrees could someone please commit. sorry, I don't have
>> >> necessary status
>> >>
>> >>
>> >> Regards,
>> >> Andrey
>> >>
>> >> On Thu, Jun 14, 2018 at 9:13 PM White, Derek
>> >> <[hidden email] <mailto:[hidden email]><mailto:[hidden email] <mailto:[hidden email]>>> wrote:
>> >> Hi Andrey,
>> >>
>> >> I like this version.
>> >>
>> >> My only suggestion is minor - the "ands" at line 3016 could be a
>> >> simple "and". It causes no harm but could be confusing to a reader:
>> >>
>> >> 3016   ands(Rscratch, Rscratch, imm);
>> >>
>> >> No need to see a new webrev.
>> >> - Derek
>> >>
>> >>> -----Original Message-----
>> >>> From: hotspot-runtime-dev
>> >>> [mailto:hotspot-runtime-dev- <mailto:hotspot-runtime-dev-><mailto:hotspot-runtime-dev- <mailto:hotspot-runtime-dev->>
>> >>> [hidden email] <mailto:[hidden email]><mailto:[hidden email] <mailto:[hidden email]>>] On Behalf
>> >>> Of Andrey Petushkov
>> >>> Sent: Thursday, June 14, 2018 8:25 AM
>> >>> To: Andrew Haley <[hidden email] <mailto:[hidden email]><mailto:[hidden email] <mailto:[hidden email]>>>
>> >>> Cc:
>> >>> [hidden email] <mailto:[hidden email]><mailto:[hidden email] <mailto:[hidden email]>>;
>> >>> aarch64-port-
>> >>> [hidden email] <mailto:[hidden email]><mailto:[hidden email] <mailto:[hidden email]>>; AArch32 Port
>> >>> Project <aarch32-port-
>> >>> [hidden email] <mailto:[hidden email]><mailto:[hidden email] <mailto:[hidden email]>>>
>> >>> Subject: Re: [aarch64-port-dev ] 8153107: Unbalanced recursive locking
>> >>>
>> >>> External Email
>> >>>
>> >>> Hm, strange. It displays well for me in the archives page. Anyway,
>> >>> I've put
>> >>> the webrev at http://cr.openjdk.java.net/~apetushkov/8153107/ <http://cr.openjdk.java.net/%7Eapetushkov/8153107/>
>> >>>
>> >>> On Thu, Jun 14, 2018 at 3:15 PM Andrew Haley
>> >>> <[hidden email] <mailto:[hidden email]><mailto:[hidden email] <mailto:[hidden email]>>> wrote:
>> >>>
>> >>>> On 06/14/2018 12:59 PM, Andrey Petushkov wrote:
>> >>>>> 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:
>> >>>> Something awful happened to the formatting of your mail.  Can you put
>> >>>> it up somewhere we can see the patch?
>> >>>>
>> >>>> --
>> >>>> Andrew Haley
>> >>>> Java Platform Lead Engineer
>> >>>> Red Hat UK Ltd. <https://www.redhat.com <https://www.redhat.com/>>
>> >>>> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332
>> >>>> <https://maps.google.com/?q=6035+332&entry=gmail&source=g <https://maps.google.com/?q=6035+332&entry=gmail&source=g>>F A671
>> >>>>
>> >
>> >
>>
>

12