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

Daniel D. Daugherty daniel.daugherty at oracle.com
Fri Jun 15 17:29:57 UTC 2018


 > 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:andrey.petushkov at gmail.com]
> Sent: Thursday, June 14, 2018 4:55 PM
> To: White, Derek <Derek.White at cavium.com>
> Cc: Andrew Haley <aph at redhat.com>; hotspot-runtime-dev at openjdk.java.net; aarch64-port-dev at openjdk.java.net; AArch32 Port Project <aarch32-port-dev at openjdk.java.net>
> 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 <Derek.White at cavium.com<mailto:Derek.White at cavium.com>> 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->
>> bounces at openjdk.java.net<mailto:bounces at openjdk.java.net>] On Behalf Of Andrey Petushkov
>> Sent: Thursday, June 14, 2018 8:25 AM
>> To: Andrew Haley <aph at redhat.com<mailto:aph at redhat.com>>
>> Cc: hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>; aarch64-port-
>> dev at openjdk.java.net<mailto:dev at openjdk.java.net>; AArch32 Port Project <aarch32-port-
>> dev at openjdk.java.net<mailto:dev at openjdk.java.net>>
>> 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 <aph at redhat.com<mailto:aph at redhat.com>> 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
>>>



More information about the aarch32-port-dev mailing list