RFR(M) 8150353: PPC64LE: Support RTM on linux

[Gustavo Romero]

Thank you Martin for all the guidelines, comments and reviews.

Best regards,
Gustavo


On 07-03-2016 13:00, Doerr, Martin wrote:
> Hi,
> 
> looks good. Thanks for making the requested changes.
> 
> The change modifies os_linux and shared C2 code.
> Can somebody from Oracle review and sponsor, please?
> 
> The webrev is here:
> http://cr.openjdk.java.net/~mdoerr/8150353_RTM_linux_ppc64/webrev.00/
> 
> It was tested in jdk9/dev/hotspot, but it applies to jdk9/hs-comp/hotspot as well.
> 
> Best regards,
> Martin
> 
> -----Original Message-----
> From: Gustavo Romero [mailto:gromero at linux.vnet.ibm.com] 
> Sent: Montag, 7. März 2016 16:27
> To: Doerr, Martin <martin.doerr at sap.com>; Vladimir Kozlov <vladimir.kozlov at oracle.com>; Thomas Stüfe <thomas.stuefe at gmail.com>
> Cc: hotspot-dev at openjdk.java.net; Breno Leitao <brenohl at br.ibm.com>
> Subject: Re: RFR(M) 8150353: PPC64LE: Support RTM on linux
> 
> Hi Martin
> 
> Sure, AIX is now included as well.
> 
> Could you please host and review this updated version?
> 
> Webrev:
> http://81.de.7a9f.ip4.static.sl-reverse.com./8150353/webrev_v3.zip
> 
> src/cpu/ppc/vm/globalDefinitions_ppc.hpp:
> 
> I merged the defines as Thomas suggested.
> 
> 
> src/cpu/ppc/vm/vm_version_ppc.cpp:
> 
> Thomas, sorry for the cryptic comment about the kernel change. It refers
> to Linux kernel source tree and not to OpenJDK. I've improved the
> comment to reflect it.
> 
> 
> Regarding MAX_inst_size:
> I assumed that the ScratchBufferBlob issue is present only on PPC64
> and, as Vladmir and Martin suggested, I made MAX_inst_size a platform
> specific value (changed only for PPC64 - AIX and Linux). Vladimir,
> thanks for doing additional analysis on x64 architecture.
> 
> 
> Regarding Linux version encoding:
> Keeping the values x.y.z together the same form it was done on AIX is
> more convenient, as Martin said, and besides that it's the way Linux
> kernel itself handles the Linux kernel versions. Hence it defines
> macro KERNEL_VERSION used whenever kernel version check/comparison is
> necessary and it does exactly what we are doing for AIX and Linux on
> PPC64:
> 
> #define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))
> 
> So I think it's not a problem to assume true that each value in x.y.z
> is always less then 256 and also never something like 3.2 (x.y). To the
> best of my knowledge there is no rigid standard for that but by looking
> at mainline kernel versions we can trust this format too. However, as
> any distro can change it at will (although very unlikely), making it
> more robust in the sense Thomas and Martin commented is a good idea. I
> did it and checked if the kernel version format is what we expect
> (x.y.z format) and for x, y, and z less then 256.
> 
> Thomas, in case of a version is unknown I decided for bailing out
> instead of just letting the VM continue with RTM disabled since it's the
> behavior, for instance, when the CPU doesn't support Transactional
> Memory and other similar conditions in the code around. But as method
> os_version_is_known() exists (instead of assert()'s in os_linux.cpp),
> it's possible to change this behavior easily in vm_version_ppc.cpp.
> 
> 
> Regarding Hotspot jtreg tests:
> No regression was observed. More 16 tests are passing now on Linux
> (PP64 LE):
> 
> Passed: compiler/rtm/cli/TestPrintPreciseRTMLockingStatisticsOptionOnSupportedConfig.java
> Passed: compiler/rtm/cli/TestRTMAbortRatioOptionOnSupportedConfig.java
> Passed: compiler/rtm/cli/TestRTMTotalCountIncrRateOptionOnSupportedConfig.java
> Passed: compiler/rtm/cli/TestUseRTMDeoptOptionOnSupportedConfig.java
> Passed: compiler/rtm/cli/TestUseRTMForStackLocksOptionOnSupportedConfig.java
> Passed: compiler/rtm/cli/TestUseRTMLockingOptionOnSupportedConfig.java
> Passed: compiler/rtm/cli/TestUseRTMLockingOptionWithBiasedLocking.java
> Passed: compiler/rtm/locking/TestRTMAbortRatio.java
> Passed: compiler/rtm/locking/TestRTMAfterNonRTMDeopt.java
> Passed: compiler/rtm/locking/TestRTMTotalCountIncrRate.java
> Passed: compiler/rtm/locking/TestUseRTMDeopt.java
> Passed: compiler/rtm/locking/TestUseRTMForInflatedLocks.java
> Passed: compiler/rtm/locking/TestUseRTMForStackLocks.java
> Passed: compiler/rtm/method_options/TestNoRTMLockElidingOption.java
> Passed: compiler/rtm/method_options/TestUseRTMLockElidingOption.java
> Passed: gc/ergonomics/TestDynamicNumberOfGCThreads.java
> 
> 
> Kind regards,
> Gustavo
> 
> On 07-03-2016 07:17, Doerr, Martin wrote:
>> Hi Gustavo,
>>
>> I’m back from vacation and I can host the webrev. Thanks for doing this change. It’s quite some work to get it into openjdk.
>>
>> May I ask you to increase the scratch buffer size for AIX as well (e.g. make it only dependent on PPC64, not on the OS in compile.hpp)?
>> We have the same problem there and we should try to make only one shared code change. We will have to ask someone from Oracle to push it (e.g. Vladimir Kozlov).
>>
>> Best regards,
>> Martin
>>
>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
>> Sent: Montag, 7. März 2016 07:48
>> To: Doerr, Martin <martin.doerr at sap.com>
>> Subject: Fwd: Re: RFR(M) 8150353: PPC64LE: Support RTM on linux
>>
>>
>> Hi Martin, kannst Du übernehmen? Bin erst morgen wieder da.
>>
>> Gruß Thomas
>> ---------- Forwarded message ----------
>> From: "Gustavo Romero" <gromero at linux.vnet.ibm.com<mailto:gromero at linux.vnet.ibm.com>>
>> Date: Mar 6, 2016 23:27
>> Subject: Re: RFR(M) 8150353: PPC64LE: Support RTM on linux
>> To: "Thomas Stüfe" <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
>> Cc: "Doerr, Martin" <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>, "hotspot-dev at openjdk.java.net<mailto:hotspot-dev at openjdk.java.net>" <hotspot-dev at openjdk.java.net<mailto:hotspot-dev at openjdk.java.net>>, <brenohl at br.ibm.com<mailto:brenohl at br.ibm.com>>
>>
>> Hi Thomas
>>
>> Thanks for offering the hosting. :-)
>>
>> Thomas, could you please host this updated version that incorporates the
>> comments made by you, Martin, and Vladimir (Martin also kindly offered
>> the hosting but probably is yet out of office - but I'm not sure).
>>
>> Webrev updated version:
>> http://81.de.7a9f.ip4.static.sl-reverse.com./8150353/webrev.zip
>>
>> Thanks a lot.
>>
>> Best regards,
>> Gustavo
>>
>> On 25-02-2016 07:57, Thomas Stüfe wrote:
>>> Hi Gustavo,
>>>
>>> I can host this for you, if you have no access to cr.openjdk.java.net<http://cr.openjdk.java.net>. Just
>>> send me the patch file.
>>>
>>> About your change:
>>>
>>> src/cpu/ppc/vm/globalDefinitions_ppc.hpp
>>>
>>> small nit: Could be probably be merged with the paragraph above where we do
>>> the same thing for AIX, but I do not have strong emotions.
>>>
>>>
>>> src/cpu/ppc/vm/vm_version_ppc.cpp
>>>
>>> +    // Please, refer to commit 4b4fadba057c1af7689fc8fa182b13baL7
>>> Does this refer to an OpenJDK change? If yes, could you please instead
>>> mention the OpenJDK bug number instead?
>>>
>>>
>>> src/os/linux/vm/os_linux.cpp
>>>
>>> os::Linux::initialize_os_info()
>>>
>>> Please make this code more robust:
>>> - Check the return value of sscanf (must be 3, otherwise your assumption
>>> about the version format was wrong)
>>> - Could this happen: "3.2" ? If yes, could you please handle it too?
>>> - Please handle overflow - if any one of minor/fix is > 256, something
>>> sensible should happen (for major, this probably indicates an error).
>>> Possibly cap out at 256?
>>>
>>> If version cannot be read successfully, the VM should not abort imho but
>>> behave gracefully. Worst that should happen is that RTM gets deactivated.
>>>
>>>
>>> Kind Regards, Thomas
>>>
>>>
>>> On Thu, Feb 25, 2016 at 9:21 AM, David Holmes <david.holmes at oracle.com<mailto:david.holmes at oracle.com>>
>>> wrote:
>>>
>>>> On 25/02/2016 6:11 PM, Gustavo Romero wrote:
>>>>
>>>>> Hi David,
>>>>>
>>>>> OK, I'll fix that.
>>>>>
>>>>> Should I re-send the RFR with the right URL and abandon this one?
>>>>>
>>>>
>>>> I think you can just post the new URL to this one.
>>>>
>>>> In case a contribution is not so small, even so is it fine to include it
>>>>> inline?
>>>>>
>>>>
>>>> Well it's preferable, for larger contributions, to find someone to host
>>>> the webrev for you.
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>
>>>> Thank you.
>>>>>
>>>>> Regards,
>>>>> Gustavo
>>>>>
>>>>> On 25-02-2016 04:54, David Holmes wrote:
>>>>>
>>>>>> Hi Gustavo,
>>>>>>
>>>>>> Just a point of order. All contributions to the OpenJDK must be made
>>>>>> through OpenJDK infrastructure. So you must either get someone to host your
>>>>>> webrev on cr.openjdk.java.net<http://cr.openjdk.java.net>, or include it inline in
>>>>>> email (attachments tend to get stripped).
>>>>>>
>>>>>> Sorry for the inconvenience.
>>>>>>
>>>>>> David
>>>>>>
>>>>>> On 25/02/2016 5:50 AM, Gustavo Romero wrote:
>>>>>>
>>>>>>> Hi Martin,
>>>>>>>
>>>>>>> Both little and big endian Linux kernel contain the syscall change, so
>>>>>>> I did not include:
>>>>>>>
>>>>>>> #if defined(COMPILER2) && (defined(AIX) || defined(VM_LITTLE_ENDIAN)
>>>>>>>
>>>>>>> in globalDefinitions_ppc.hpp.
>>>>>>>
>>>>>>> Please, could you review the following change?
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8150353
>>>>>>> Webrev (hotspot): http://81.de.7a9f.ip4.static.sl-reverse.com/webrev/
>>>>>>>
>>>>>>> Summary:
>>>>>>>
>>>>>>> * Enable RTM support for Linux on PPC64 (LE and BE).
>>>>>>> * Fix C2 compiler buffer size issue.
>>>>>>>
>>>>>>> Thank you.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Gustavo
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>