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

[Gustavo Romero]

Thanks a lot Vladimir.

Best regards,
Gustavo

On 07-03-2016 14:43, Vladimir Kozlov wrote:
> Looks good. I will push it.
> 
> Thanks,
> Vladimir
> 
> On 3/7/16 8:00 AM, 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
>