RFR(M): 8144847: PPC64: Update Transactional Memory and Atomic::cmpxchg code

Thu Dec 10 16:52:12 UTC 2015

Looks fine.

Thanks, Thomas

On Thu, Dec 10, 2015 at 5:30 PM, Doerr, Martin <martin.doerr at sap.com> wrote:

> Hi Thomas,
>
>
>
> thanks for the review.
>
>
>
> Current as400 implementations configure the processor in a way that
> Transactional Memory can’t be used (which is already detected).
>
> But, it is safer to check if we’re really on AIX before we activate
> Transactional Memory.
>
>
>
> I made the changes in this new webrev:
>
> http://cr.openjdk.java.net/~mdoerr/8144847_ppc_updates/webrev.02/
>
>
>
> Best regards,
>
>   Martin
>
>
>
> *From:* Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> *Sent:* Donnerstag, 10. Dezember 2015 16:37
> *To:* Doerr, Martin <martin.doerr at sap.com>
> *Cc:* Lindenmaier, Goetz <goetz.lindenmaier at sap.com>;
> hotspot-compiler-dev at openjdk.java.net
>
> *Subject:* Re: RFR(M): 8144847: PPC64: Update Transactional Memory and
> Atomic::cmpxchg code
>
>
>
> Hi Martin,
>
>
>
> thanks for working in my requests!
>
>
>
> here some more nits:
>
>
>
>
> http://cr.openjdk.java.net/~mdoerr/8144847_ppc_updates/webrev.01/src/cpu/ppc/vm/vm_version_ppc.cpp.udiff.html
>
>
>
> +    if (os::Aix::os_version() >= 0x0701031e) { // at least AIX 7.1.3.30
>
>
>
> Do we support RTM on PASE? if not, do we need to exclude it?
>
>
>
>
> http://cr.openjdk.java.net/~mdoerr/8144847_ppc_updates/webrev.01/src/os/aix/vm/os_aix.cpp.udiff.html
>
>
>
> +    _os_version = (major << 24) | (minor << 16);
>
> +    char ver_str[20] = {0};
>
> +    char *name_str = "unknown OS";
>
>
>
> please make name_str a const char*;
>
>
>
> -      if (_os_version < 0x0503) {
>
> +      // Determine detailed AIX version: Version, Release, Modification,
> Fix Level.
>
> +      odmWrapper::determine_os_kernel_version(&_os_version);
>
> +      if (os_version_short() < 0x0503) {
>
>
>
> Lets do the kernel version number query only if needed, so inside the "if
> (os_version_short() < 0x0503) {"
>
>
>
>
> http://cr.openjdk.java.net/~mdoerr/8144847_ppc_updates/webrev.01/src/os/aix/vm/os_aix.hpp.udiff.html
>
>
>
> -  // -1 = uninitialized, otherwise 16 bit number:
>
> +  // 0 = uninitialized, otherwise 16 bit number:
>
>    //  lower 8 bit - minor version
>
>    //  higher 8 bit - major version
>
>    //  For AIX, e.g. 0x0601 for AIX 6.1
>
>    //  for OS/400 e.g. 0x0504 for OS/400 V5R4
>
> -  static int _os_version;
>
> -
>
> -  // 4 Byte kernel version: Version, Release, Tech Level, Service Pack.
>
> -  static unsigned int _os_kernel_version;
>
> +  static uint32_t _os_version;
>
>
>
> comment needs to be adapted.
>
>
>
> ---------
>
>
>
> I did not look closely at the RTM-related changes, just at the AIX side,
> so this is not a full review.
>
>
>
> Kind Regards, Thomas
>
>
>
> On Thu, Dec 10, 2015 at 4:06 PM, Doerr, Martin <martin.doerr at sap.com>
> wrote:
>
> Hi,
>
>
>
> this new webrev applies to hs-rt:
>
> http://cr.openjdk.java.net/~mdoerr/8144847_ppc_updates/webrev.01/
>
>
>
> It only touches PPC64 files.
>
>
>
> I have made the changes requested by Thomas.
>
> I only had to remove a minor interpreter variable name change. The
> remainder fits to hs-rt.
>
>
>
> Please have a look.
>
>
>
> Best regards,
>
>   Martin
>
>
>
>
>
> *From:* Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> *Sent:* Mittwoch, 9. Dezember 2015 08:28
>
>
> *To:* Doerr, Martin <martin.doerr at sap.com>
> *Cc:* hotspot-compiler-dev at openjdk.java.net
> *Subject:* Re: RFR(M): 8144847: PPC64: Update Transactional Memory and
> Atomic::cmpxchg code
>
>
>
> Hi Martin,
>
>
>
> You could split the os kernel detection from the RTM change and submit the
> former to hs-rt now.
>
>
>
> Kind regards, Thomas
>
>
>
> On Tue, Dec 8, 2015 at 3:08 PM, Doerr, Martin <martin.doerr at sap.com>
> wrote:
>
> Hi Thomas,
>
>
>
> thanks for the hint. There are changes in hs-comp and hs-rt which would
> cause trouble with my change at the moment. I’ll wait until they get merged
> and create a new webrev which hopefully applies to both repositories.
>
>
>
> Best regards,
>
>   Martin
>
>
>
> *From:* Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> *Sent:* Dienstag, 8. Dezember 2015 09:22
> *To:* Doerr, Martin <martin.doerr at sap.com>
> *Cc:* hotspot-compiler-dev at openjdk.java.net
> *Subject:* Re: RFR(M): 8144847: PPC64: Update Transactional Memory and
> Atomic::cmpxchg code
>
>
>
> Hi Martin,
>
>
>
> thanks for this addition :)
>
>
>
> It may make a lot of sense to rebase this change to hs-rt, because
> os_aix.cpp is quite different there after
> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/ce87b1141c12. Otherwise
> we may have problems later applying your change atop of my change.
>
>
>
> -------------
>
>
>
> About the AIX kernel version recognition: I know we talked about this, but
> I have second thoughts now. I guess I did not think it really through
> before, sorry.
>
> So, now I have a change request:
>
>
>
> Instead of introducing os::Aix::os_kernel_version
> (version,release,techlevel,sp) beside the already existing
> os::Aix::os_version (version,release) I would prefer just one parameter,
> os_version, end enriching this by techlevel and sp. So, exactly what you
> did for os_kernel_version.
>
>
>
> Basically, as a prototype:
>
>
>
> // -1 = uninitialized, otherwise 32 bit number:
>
> //  0xVVRRTTSS
>
> //  VV - major version
>
> //  RR - minor version
>
> //  TT - tech level, if known, 0 otherwise
>
> //  SS - service pack, if known, 0 otherwise
>
> static uint32_t os_version ();
>
>
>
> Then please change the few users of os::Aix::os_version() to now expect a
> 32bit unsigned number. As far as I see there are only 3 callsites.
>
>
>
> -------------------
>
>
>
> Other small nitpicks:
>
>
>
> - in libodm_aix.cpp, please use trcVerbose() instead of if (Verbose)
> tty->.. . Please include misc_aix.hpp for trcVerbose(). We will change all
> those tracecalls to Unified logging in the near future and this would help
> me finding all trace occurrences.
>
>
>
> - please move ~dynamicOdm() and odmWrapper::clean_wrapper() from
> libodm_aix.hpp to libodm_aix.cpp and accordingly remove the includes
> dlfcn.h and stdlib.h from libodm_aix.hpp.
>
>
>
> - I probably would change "static unsigned int
> determine_os_kernel_version(int major_aix_version, int minor_aix_version);"
> to " "static bool fill_in_os_kernel_version(unsigned int* p_os_version);",
> but that is just a matter of taste.
>
>
>
> Kind Regards, Thomas
>
>
>
>
>
> On Mon, Dec 7, 2015 at 6:10 PM, Doerr, Martin <martin.doerr at sap.com>
> wrote:
>
> Hi,
>
>
>
> I have created a webrev for further PPC64 updates:
>
> AIX supports Transactional Memory with a certain kernel patch level. Add a
> detection for it and make UseRTMLocking usable on AIX.
> In addition, implement Atomic::cmpxchg for jbyte.
>
>
>
> The webrev is here:
>
> http://cr.openjdk.java.net/~mdoerr/8144847_ppc_updates/webrev.00/
>
>
>
> Please review.
>
>
>
> Best regards,
>
>   Martin
>
>
>
>
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20151210/b3428b77/attachment-0001.html>