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
Hi Gustavo, Looks good for the case UseRTMXendForLockBusy is active (which is default). 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? Thanks and best regards, Martin -----Original Message----- From: Gustavo Romero [mailto:gromero@linux.vnet.ibm.com] Sent: Montag, 25. Juni 2018 10:24 To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Doerr, Martin <martin.doerr@sap.com>; hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@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
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@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@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@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@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@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@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@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@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@linux.vnet.ibm.com] Sent: Montag, 25. Juni 2018 10:24 To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Doerr, Martin <martin.doerr@sap.com>; hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@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
Hi Gustavo, 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. 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. 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(). Best regards, Martin -----Original Message----- From: Gustavo Romero [mailto:gromero@linux.vnet.ibm.com] Sent: Dienstag, 26. Juni 2018 18:01 To: Doerr, Martin <martin.doerr@sap.com>; Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@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@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@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@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@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@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@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@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@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@linux.vnet.ibm.com] Sent: Montag, 25. Juni 2018 10:24 To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Doerr, Martin <martin.doerr@sap.com>; hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@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
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@linux.vnet.ibm.com] Sent: Dienstag, 26. Juni 2018 18:01 To: Doerr, Martin <martin.doerr@sap.com>; Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@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@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@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@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@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@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@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@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@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@linux.vnet.ibm.com] Sent: Montag, 25. Juni 2018 10:24 To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Doerr, Martin <martin.doerr@sap.com>; hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@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
Hi, Could I get a second review for that change please? It's already reviewed by Martin. Best regards, Gustavo On 07/02/2018 11:03 AM, Gustavo Romero wrote:
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@linux.vnet.ibm.com] Sent: Dienstag, 26. Juni 2018 18:01 To: Doerr, Martin <martin.doerr@sap.com>; Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@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@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@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@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@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@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@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@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@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@linux.vnet.ibm.com] Sent: Montag, 25. Juni 2018 10:24 To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Doerr, Martin <martin.doerr@sap.com>; hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@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
Hi Gustavo, the change looks good, Reviewed. Small nit, don't need a new webrev: + // used in the JVM. Thus mostly (B) a Nesting Overflows or (C) a Footprint 'Overflows' should be singular I think. Best regards, Goetz.
-----Original Message----- From: Gustavo Romero [mailto:gromero@linux.vnet.ibm.com] Sent: Dienstag, 17. Juli 2018 15:44 To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Cc: Doerr, Martin <martin.doerr@sap.com>; hotspot-compiler- dev@openjdk.java.net; ppc-aix-port-dev@openjdk.java.net Subject: Re: RFR(s): 8205580: PPC64: RTM: Don't retry lock on abort if abort was intentional
Hi,
Could I get a second review for that change please? It's already reviewed by Martin.
Best regards, Gustavo
On 07/02/2018 11:03 AM, Gustavo Romero wrote:
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@linux.vnet.ibm.com] Sent: Dienstag, 26. Juni 2018 18:01 To: Doerr, Martin <martin.doerr@sap.com>; Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@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@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@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@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@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@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@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@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@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@linux.vnet.ibm.com] Sent: Montag, 25. Juni 2018 10:24 To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Doerr, Martin <martin.doerr@sap.com>; hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@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
On 07/17/2018 12:19 PM, Lindenmaier, Goetz wrote:
Hi Gustavo,
the change looks good, Reviewed.
Small nit, don't need a new webrev: + // used in the JVM. Thus mostly (B) a Nesting Overflows or (C) a Footprint 'Overflows' should be singular I think.
Thanks, Goetz. I'll push it to jdk/jdk11 today. Best regards, Gustavo
Best regards, Goetz.
-----Original Message----- From: Gustavo Romero [mailto:gromero@linux.vnet.ibm.com] Sent: Dienstag, 17. Juli 2018 15:44 To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com> Cc: Doerr, Martin <martin.doerr@sap.com>; hotspot-compiler- dev@openjdk.java.net; ppc-aix-port-dev@openjdk.java.net Subject: Re: RFR(s): 8205580: PPC64: RTM: Don't retry lock on abort if abort was intentional
Hi,
Could I get a second review for that change please? It's already reviewed by Martin.
Best regards, Gustavo
On 07/02/2018 11:03 AM, Gustavo Romero wrote:
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@linux.vnet.ibm.com] Sent: Dienstag, 26. Juni 2018 18:01 To: Doerr, Martin <martin.doerr@sap.com>; Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@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@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@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@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@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@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@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@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@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@linux.vnet.ibm.com] Sent: Montag, 25. Juni 2018 10:24 To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>; Doerr, Martin <martin.doerr@sap.com>; hotspot-compiler-dev@openjdk.java.net Cc: ppc-aix-port-dev@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
participants (3)
-
Doerr, Martin
-
Gustavo Romero
-
Lindenmaier, Goetz