[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