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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Tue Jul 17 15:19:05 UTC 2018 

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