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

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


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: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