RFR(s): 8205582: PPC64: RTM: Fix counter for aborts on nested transactions

[Doerr, Martin]

Hi Gustavo,

thanks for the new webrev.

You're right, the two bits should be mutual exclusive, so the sum is equivalent to the logical or in this case.
However, the loop pretends to be generic, but it's not. The "or" only works for mutual exclusive bits.
If you want to keep the code with the loops, I think there should be a comment explaining this.

Please also fix indentation.

Alternatively, the loop could get replaced by some code for each bit. Given that the loop is not really generic, I think this wouldn't be worse.

Besides that, it looks good. Thanks for improving it.

Best regards,
Martin


-----Original Message-----
From: Gustavo Romero [mailto:gromero at linux.vnet.ibm.com] 
Sent: Freitag, 13. Juli 2018 16:56
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): 8205582: PPC64: RTM: Fix counter for aborts on nested transactions

Hi Martin,

On 07/12/2018 12:30 PM, Doerr, Martin wrote:
> I think your new code may increment a counter twice when 2 bits are specified for it. I think this should get fixed.
> Iterating over the RTMLockingCounters in the outer loop and performing several checks before incrementing should fix this, right?

Do you mean increment twice counter #2 (conflict counter) or another counter?
non_trans_cf and trans_cf are mutually exclusive.
You probably spotted a case I'm missing so I ask.


> Besides that, I have some improvement proposals:
> 
> - I think using 3 constant tables is not so good to read. For example, inverting could be encoded in your new 2 dimensional table by using -1, 0 , +1 when using int instead of bool. Would this be better?
> 
> - I think you can get rid of rtm_counters_Reg increment and restoration by computing the abort_offs relative to the original value.

Cool :)

New (interim) webrev:
http://cr.openjdk.java.net/~gromero/8205582/v3/

Thanks.


Best regards,
Gustavo

> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Gustavo Romero [mailto:gromero at linux.vnet.ibm.com]
> Sent: Dienstag, 10. Juli 2018 18:59
> 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): 8205582: PPC64: RTM: Fix counter for aborts on nested transactions
> 
> Dear Martin,
> 
> On 06/25/2018 01:31 PM, Doerr, Martin wrote:
>> I think it would be better to ignore bit 63 in the macroAssembler code and use the definition from the spec in assembler_ppc.hpp.
>> Somebody may want to use the definition for other purposes.
> 
> Done. transactional_level bit is 52 now so bits 52:62 can easily be extracted
> using 'rldicr' in macroAssembler.
> 
> 
>> I wonder if Assembler::tm_trans_cf | Assembler::tm_non_trans_cf would be a better match for x86's description for tm_failure_bit[2]. It's also a little unfortunate to print the same bit twice as tm_failure_bit[4].
> 
> Done. Now both tm_trans_cf and tm_non_trans_cf failures will increment counter 2
> (conflict). Duplicated check code for tm_failure_bit[4] was removed and now
> counter 4 (debug) is mapped to count traps or syscalls caught in TM events,
> which seems a reasonable approximation to the original semantics of the debug
> counter on Intel. Unfortunately I could not confirm on AIX how these two events
> (trap and syscall in TM) will set the failure code, so the counter will never
> track any information on AIX. But with the current proposed change that failure
> code can be easily added in the future.
> 
> I also realized that I used previously a wrong ME operand value in:
> 
> +        // Extract 11 bits
> +        rldicr_(temp_Reg, abort_status, tm_failure_bit[i], 11);
> 
> It should be 10 to extract 11 bits actually, so all extractions must be correct
> now.
> 
> 
> I hope you don't find the array map of failure bits vs counters overkilling.
> 
> Finally, I replaced the comment:
> 
> tm_tabort, // Note: Seems like signal handler sets this, too.
> 
> by:
> 
> tm_tabort, // Signal handler will set this too.
> 
> because we just enable RTM support on Power if 'htm-nosc' is supported, so
> treclaim. on aborting the syscall will indeed always set Abort bit in TEXASR
> afaik. Since debug counter now tracks trap/syscall in TM it's possible to check
> that counter to verify the number of aborts cause by the kernel (if any).
> 
> new webrev: http://cr.openjdk.java.net/~gromero/8205582/v2/
> 
> 
> Best regards,
> Gustavo
> 
>> Best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: Gustavo Romero [mailto:gromero at linux.vnet.ibm.com]
>> Sent: Montag, 25. Juni 2018 10:19
>> 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): 8205582: PPC64: RTM: Fix counter for aborts on nested transactions
>>
>> Hi,
>>
>> Could the following change be reviewed please?
>>
>> bug   : https://bugs.openjdk.java.net/browse/JDK-8205582
>> webrev: http://cr.openjdk.java.net/~gromero/8205582/v1/
>>
>> It fixes the RTM counter for nested aborts (rtm lock aborts type 5) by
>> extracting and checking bits in the Transactional Level field of TEXASR
>> register.
>>
>> It also fixes the memory conflict counter (rtm lock aborts type 2). Power TM
>> status register supports two bits to inform two different types of memory
>> conflict between threads: non-transactional and transactional. According to how
>> the jtreg RTM tests are designed the memory conflict counter counts
>> non-transactional conflicts: on TestPrintPreciseRTMLockingStatistics a RTM lock
>> is held on a static variable while another thread without any synchronization
>> (non-trasactional) tries to modify the same variable. Hence that small
>> adjustment satisfies the TestPrintPreciseRTMLockingStatistics making it pass on
>> Power. The memory conflict counter is not used in any other place besides by the
>> RTM precise statistics (no decision is made by the JVM based on that amount).
>>
>> This change partially fixes some failures in
>> compiler/rtm/print/TestPrintPreciseRTMLockingStatistics.java regarding the
>> nested and memory conflict abort counters. The remaining issue will be fixed by
>> aborting on calling JNI (next RFR).
>>
>>
>> Thank you and best regards,
>> Gustavo
>>
>