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

Tue Jul 17 13:44:17 UTC 2018

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