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

Gustavo Romero gromero at linux.vnet.ibm.com
Mon Mar 7 15:27:11 UTC 2016 

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

More information about the hotspot-dev mailing list