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

Tue Jun 26 16:01:07 UTC 2018

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
> 