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

Gustavo Romero gromero at linux.vnet.ibm.com
Wed Jul 18 14:38:05 UTC 2018 

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 at linux.vnet.ibm.com]
>> Sent: Dienstag, 17. Juli 2018 15:44
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
>> Cc: Doerr, Martin <martin.doerr at sap.com>; hotspot-compiler-
>> dev at openjdk.java.net; 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,
>>
>> 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
>>>>>
>>>>
>>>
>

More information about the ppc-aix-port-dev mailing list