RFR(s): 8205580: PPC64: RTM: Don't retry lock on abort if abort was intentional

Mon Jul 2 14:03:00 UTC 2018

Hi Martin,

On 07/02/2018 10:20 AM, Doerr, Martin wrote:
> I meant retrying "on abort", not "on busy". There are different counters for these two retry functions.
> RTM for Stack Locks only supports "on abort".
> Inflated RTM locking supports both using both counters.

Yup, I meant "on abort" too, but I missed your point regarding the retry
"on abort" in the RTM for Stack Locks case.

> But I see that x86 uses the same behavior as you when using -XX:-UseRTMXendForLockBusy.
> I think it's not so good to treat "abort instruction on lock busy" as permanent abort reason.
> So the behavior is fine with UseRTMXendForLockBusy, but not without it.

hmm I see. So you think it's also not fine on x64, right? In general I think
it's not good to finish a RTM atomic block with `xabort/tabort`, but maybe it
makes more sense on x86. Luckily -XX:+UseRTMXendForLockBusy is the default.

> But I can live with your change because it only has a negative effect on an unsupported experimental option. And I think your change is fine for the other usages of tabort().

OK. I'll revisit that when doing additional performance tests with RTM for Stack
Locks.

Thanks!

Best regards,
Gustavo

> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Gustavo Romero [mailto:gromero at linux.vnet.ibm.com]
> Sent: Dienstag, 26. Juni 2018 18:01
> To: Doerr, Martin <martin.doerr at sap.com>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-compiler-dev at openjdk.java.net
> Cc: ppc-aix-port-dev at openjdk.java.net
> Subject: Re: RFR(s): 8205580: PPC64: RTM: Don't retry lock on abort if abort was intentional
> 
> Hi Martin,
> 
> On 06/25/2018 01:49 PM, Doerr, Martin wrote:
>> Looks good for the case UseRTMXendForLockBusy is active (which is default).
> 
> I did all the tests focusing on when it's deactivated (-UseRTMXendForLockBusy).
> This is also the flag passed in jtreg tests since if it's active there are no
> aborts caused by 'tabort or xabort' and so no abort statistics (related to
> that event, which is used by the jtreg tests).
> 
> 
>> If this flag is deactivated, we use tabort if we see the object locked so your change prevents retrying the transaction in this case.
>> I guess this was not intended?
> 
> I think that rtm_retry_lock_on_abort() is a misleading name, it should be
> something like rtm_retry_lock_on_conflict(), since the purpose of this
> function is to no retry if abort is caused by a tabort/xabort in my
> understanding.
> 
> On Intel that function checks for bit 1 (0x2 mask) and if it is set the operation
> is retried. But to bit 1 be set it implies that transaction didn't abort due to
> xabort, otherwise that bit would be clear as:
> 
>    77   //     0     Set if abort caused by XABORT instruction.
>    78   //     1     If set, the transaction may succeed on a retry.
>                      This bit is always clear if bit 0 is set (or is always clear if abort is caused by XABORT)
> 
> That's why filtering on Power by the "Abort" bit in TEXASR makes the number of
> aborts behave like on x64. If we don't filter abort caused by tabort we find the
> pattern X*2+1 times of retries, because both rtm_retry_lock_on_abort() and
> rtm_retry_lock_on_busy() will try RTMRetryCount of times the operation.
> 
> My change won't prevent retrying because after rtm_retry_lock_on_abort(), if
> cmpxchgd() does not succeed it calls rtm_retry_lock_on_busy(), which by its turn
> will retry the operation based too on the value specified by RTMRetryCount.
> 
> I prepared a simple test-case where UseRTMXendForLockBusy is deactivated to show
> that if we increase the number of RTMRetryCount even with that flag deactivated
> the operation is retried exactly RTMRetryCount+1 times after the fix, like on
> Intel:
> 
> https://github.com/gromero/retry
> 
> You just need to clone and run it pointing to a build dir:
> 
> $ git clone https://github.com/gromero/retry && cd retry
> $ ./retry <build_dir> <RTMRetryCount_value>
> 
> You have to build the WhiteBox lib through "make build-test-lib" before running
> it.
> 
> So for RTMRetryCount=1 and RTMRetryCount=2 w/ -UseRTMXendForLockBusy before the
> change:
> 
> gromero at gromero16:~/git/retry$ ./retry.sh /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release 1
> ++ /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/jdk/bin/javac -cp /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/support/test/lib/wb.jar --add-exports java.base/jdk.internal.misc=ALL-UNNAMED retry.java
> ++ LD_LIBRARY_PATH=/home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/jdk/../..//src/utils/hsdis/build/linux-ppc64le
> ++ /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/jdk/bin/java -Xbootclasspath/a:/home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/support/test/lib/wb.jar -XX:+UnlockExperimentalVMOptions -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseRTMLocking -XX:+PrintPreciseRTMLockingStatistics -XX:-TieredCompilation -Xcomp -XX:-UseRTMXendForLockBusy -XX:RTMTotalCountIncrRate=1 -XX:RTMRetryCount=1 -XX:CompileOnly=RTM.syncAndTest --add-exports java.base/jdk.internal.misc=ALL-UNNAMED retry
> Creating thread0...
> Trying to inflate lock...
> Is monitor inflated? Yes
> Entering thread to sleep...
> RTM.syncAndTest at 26
> # rtm locks total (estimated): 3
> # rtm lock aborts  : 3
> # rtm lock aborts 0: 3
> # rtm lock aborts 1: 3
> # rtm lock aborts 2: 0
> # rtm lock aborts 3: 0
> # rtm lock aborts 4: 0
> # rtm lock aborts 5: 0
> ++ set +x
> gromero at gromero16:~/git/retry$ ./retry.sh /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release 2
> ++ /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/jdk/bin/javac -cp /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/support/test/lib/wb.jar --add-exports java.base/jdk.internal.misc=ALL-UNNAMED retry.java
> ++ LD_LIBRARY_PATH=/home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/jdk/../..//src/utils/hsdis/build/linux-ppc64le
> ++ /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/jdk/bin/java -Xbootclasspath/a:/home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/support/test/lib/wb.jar -XX:+UnlockExperimentalVMOptions -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseRTMLocking -XX:+PrintPreciseRTMLockingStatistics -XX:-TieredCompilation -Xcomp -XX:-UseRTMXendForLockBusy -XX:RTMTotalCountIncrRate=1 -XX:RTMRetryCount=2 -XX:CompileOnly=RTM.syncAndTest --add-exports java.base/jdk.internal.misc=ALL-UNNAMED retry
> Creating thread0...
> Trying to inflate lock...
> Is monitor inflated? Yes
> Entering thread to sleep...
> RTM.syncAndTest at 26
> # rtm locks total (estimated): 5
> # rtm lock aborts  : 5
> # rtm lock aborts 0: 5
> # rtm lock aborts 1: 5
> # rtm lock aborts 2: 0
> # rtm lock aborts 3: 0
> # rtm lock aborts 4: 0
> # rtm lock aborts 5: 0
> ++ set +x
> 
> 
> and after the change:
> 
> gromero at gromero16:~/git/retry$ ./retry.sh /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release 1
> ++ /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/jdk/bin/javac -cp /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/support/test/lib/wb.jar --add-exports java.base/jdk.internal.misc=ALL-UNNAMED retry.java
> ++ LD_LIBRARY_PATH=/home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/jdk/../..//src/utils/hsdis/build/linux-ppc64le
> ++ /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/jdk/bin/java -Xbootclasspath/a:/home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/support/test/lib/wb.jar -XX:+UnlockExperimentalVMOptions -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseRTMLocking -XX:+PrintPreciseRTMLockingStatistics -XX:-TieredCompilation -Xcomp -XX:-UseRTMXendForLockBusy -XX:RTMTotalCountIncrRate=1 -XX:RTMRetryCount=1 -XX:CompileOnly=RTM.syncAndTest --add-exports java.base/jdk.internal.misc=ALL-UNNAMED retry
> Creating thread0...
> Trying to inflate lock...
> Is monitor inflated? Yes
> Entering thread to sleep...
> RTM.syncAndTest at 26
> # rtm locks total (estimated): 2
> # rtm lock aborts  : 2
> # rtm lock aborts 0: 2
> # rtm lock aborts 1: 2
> # rtm lock aborts 2: 0
> # rtm lock aborts 3: 0
> # rtm lock aborts 4: 0
> # rtm lock aborts 5: 0
> ++ set +x
> gromero at gromero16:~/git/retry$ ./retry.sh /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release 2
> ++ /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/jdk/bin/javac -cp /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/support/test/lib/wb.jar --add-exports java.base/jdk.internal.misc=ALL-UNNAMED retry.java
> ++ LD_LIBRARY_PATH=/home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/jdk/../..//src/utils/hsdis/build/linux-ppc64le
> ++ /home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/jdk/bin/java -Xbootclasspath/a:/home/gromero/hg/jdk/jdk/build/linux-ppc64le-normal-server-release/support/test/lib/wb.jar -XX:+UnlockExperimentalVMOptions -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI -XX:+UseRTMLocking -XX:+PrintPreciseRTMLockingStatistics -XX:-TieredCompilation -Xcomp -XX:-UseRTMXendForLockBusy -XX:RTMTotalCountIncrRate=1 -XX:RTMRetryCount=2 -XX:CompileOnly=RTM.syncAndTest --add-exports java.base/jdk.internal.misc=ALL-UNNAMED retry
> Creating thread0...
> Trying to inflate lock...
> Is monitor inflated? Yes
> Entering thread to sleep...
> RTM.syncAndTest at 26
> # rtm locks total (estimated): 3
> # rtm lock aborts  : 3
> # rtm lock aborts 0: 3
> # rtm lock aborts 1: 3
> # rtm lock aborts 2: 0
> # rtm lock aborts 3: 0
> # rtm lock aborts 4: 0
> # rtm lock aborts 5: 0
> ++ set +x
> 
> 
> Best regards,
> Gustavo
> 
>> Thanks and best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: Gustavo Romero [mailto:gromero at linux.vnet.ibm.com]
>> Sent: Montag, 25. Juni 2018 10:24
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Doerr, Martin <martin.doerr at sap.com>; hotspot-compiler-dev at openjdk.java.net
>> Cc: ppc-aix-port-dev at openjdk.java.net
>> Subject: RFR(s): 8205580: PPC64: RTM: Don't retry lock on abort if abort was intentional
>>
>> Hi,
>>
>> Could the following change be reviewed please?
>>
>> bug   : https://bugs.openjdk.java.net/browse/JDK-8205580
>> webrev: http://cr.openjdk.java.net/~gromero/8205580/v1/
>>
>> It changes the behavior of rtm_retry_lock_on_abort() by avoiding retry if abort
>> was a deliberate abort, i.e. caused by a 'tabort r0' instruction.
>>
>> On Intel bit 1 in abort_status_Reg (which communicates the abort status) is
>> always clear when a 'xabort 0' instruction is executed in order to inform that a
>> transactional retry /can not/ succeed on retry. So rtm_retry_lock_on_abort() on
>> Intel, on finding bit 1 clear in abort_status_Reg, skips the retry (don't
>> retry).
>>
>> Currently on Power rtm_retry_lock_on_abort() is just checking the persistent bit
>> (if set => skip) which /is not set/ by 'tabort r0'. Hence
>> rtm_retry_lock_on_abort() does retry to lock on an intentional abort caused by
>> 'tabort'. It leads, for instance when -XX:RTMRetryCount=1, to the following
>> discrepancy between Intel and Power regarding the number of retries/aborts:
>>
>> [Power]
>> # rtm locks total (estimated): 3
>> # rtm lock aborts  : 3
>> # rtm lock aborts 0: 3
>> # rtm lock aborts 1: 3
>> # rtm lock aborts 2: 0
>> # rtm lock aborts 3: 0
>> # rtm lock aborts 4: 0
>> # rtm lock aborts 5: 0
>>
>> [Intel]
>> # rtm locks total (estimated): 2
>> # rtm lock aborts  : 2
>> # rtm lock aborts 0: 2
>> # rtm lock aborts 1: 2
>> # rtm lock aborts 2: 0
>> # rtm lock aborts 3: 0
>> # rtm lock aborts 4: 0
>> # rtm lock aborts 5: 0
>>
>> So for -XX:RTMRetryCount=X:
>> on Power the number of aborts is: X*2+1 [1 first failure + 1 rtm_retry_lock_on_abort() + 1 rtm_retry_lock_on_busy()];
>> on Intel the number of aborts is: X+1   [1 first failure + 1 rtm_retry_lock_on_busy()]
>>
>> This change fixes that discrepancy by using bit "Abort" in TEXASR register
>> (abort_status_Reg) that tells if a transaction was aborted due to a 'tabort'
>> instruction and skip the retry if such a bit is set.
>>
>> It fixes the following tests:
>>
>> +Passed: compiler/rtm/locking/TestRTMRetryCount.java
>> +Passed: compiler/rtm/locking/TestRTMAbortThreshold.java
>>
>>
>> Thank you and best regards,
>> Gustavo
>>
> 