RFR(L): 8143125: [aix] Further Developments for AIX

Volker Simonis volker.simonis at gmail.com
Wed Dec 2 10:19:25 UTC 2015

Hi Thomas,

thanks a lot for this nice cleanup. Please find my remaining comments below:

- please update the copyright year
- remove the following comments which reference SAP path or bugs

  45   // See libo4.h (at //bas2/sapjvm/dev/common/libs/libo4/include)
  46   // for details on this API.

  59   // See libo4.h (at //bas2/sapjvm/dev/common/libs/libo4/include)
  60   // for details on this API.

  72   // See also CSN Internal Message 0001182318 2008
  73   //
  74   // See libo4.h (at //bas2/sapjvm/dev/common/libs/libo4/include)
  75   // for details on this API.

- please update the copyright year

- please update the copyright year

- "trc" is defined to the empty string and doesn't seemed to be used anywhere:

  45 #define trc(fmt, ...)

do we still need it?

- why do we need the following define right in-between the includes:

  76 #include "utilities/defaultStream.hpp"
  77 #define PV_8_Compat 0x308000   /* Power PC 8 */
  78 #include "utilities/events.hpp"

it doesn't seem to be used in os_aix.cpp

- you replaced most of the logging with trcVerbose but there are still
tow or three places using fprintf and jio_fprintf. Could you please
replace them by calls to trcVerbose as well.

- I can't see where the kernel thread id is initialized. Shouldn't you
call set_kernel_thread_id() from os::create_thread and
os::create_attached_thread() ?

- in os::commit_memory() are you sure you always have 4K pages:

2342   if (UseExplicitCommit) {
2343     // AIX commits memory on touch. So, touch all pages to be committed.
2344     for (char* p = addr; p < (addr + size); p += SIZE_4K) {
2345       *p = '\0';
2346     }
2347   }

wouldn’t it be better (i.e. potentially faster) to use
os::vm_page_size() instead of SIZE_4K ?

- it doesn't seem we use '_large_page_size'. We statically initialize
it to '0' and later on in os::init() to '-1'. Better remove it and
directly return '0' (or '-1') from os::large_page_size().

libperfstat_aix.{cpp, hpp}
- please update the copyright year
- I want to second Goetz - we really don't need
perfstat_cpu_total_t_52. Please remove it from both
libperfstat_aix.{cpp, hpp}

- please update the copyright year

- I don't see why we need MiscUtils::describe_errno(). Can you please
instead use strerror() to get the error message (as we already do in a
lot of other places, e.g. in os_aix.cpp). That said, I realized that
strerror() is not thread-safe. So we should probably change it to the
POSIX function strerror_r(). But this is not only an AIX problem, so
better do that in a follow up change.

- the same holds true for MiscUtils::sleep_ms(). Why do we need yet
another AIX-specific helper function? Moreover, it is used only once.
Can you please instead use usleep() or sleep() directly, depending on
how long you want to wait. (Just as a side note: your current
implementation of sleep_ms() would sleep for three seconds if called
with 1000ms as argument, and it would call usleep() with an argument
of 1.000.000 which should be avoided according to your comment (which
I couldn't verify in the AIX man-page (see
So please remove MiscUtils::sleep_ms().

There's no need to provide a new webrev, if you check that the your
latest changes will build and run.

Thank you and best regards,

On Thu, Nov 26, 2015 at 9:29 AM, Lindenmaier, Goetz
<goetz.lindenmaier at sap.com> wrote:
> Hi Thomas,
> thanks a lot for doing all these changes.
> Looks good now.
> Could you add the change comment to the patch before you
> Make a webrev next time?  Simplifies sponsoring ;)
> Best regards,
>   Goetz.
> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> Sent: Mittwoch, 25. November 2015 18:01
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Cc: ppc-aix-port-dev at openjdk.java.net; HotSpot Open Source Developers <hotspot-dev at openjdk.java.net>
> Subject: Re: RFR(L): 8143125: [aix] Further Developments for AIX
> Hi Goetz,
> new webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8143125-Further/webrev.01/webrev/
> thank you for reviewing! See remarks inline...
> On Tue, Nov 24, 2015 at 12:38 PM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>> wrote:
> Hi Thomas,
> I looked at your change.  It’s good we get these improvements into
> openJDK.  But I think we should skip some stuff that's not (yet?)
> used there.  I'll sponsor the change. Details:
> libperfstat_aix.cpp:
> What do you need
> static fun_perfstat_partition_total_t g_fun_perfstat_partition_total = NULL;
> static fun_perfstat_wpar_total_t    g_fun_perfstat_wpar_total = NULL;
> static fun_wpar_getcid_t            g_fun_wpar_getcid = NULL;
> for? I think they are never used.
> They are now used (for the respective wrapper functions which in turn are used in os_aix.cpp. Sorry for omitting this code.
> libperfstat_aix.cpp:161
> We support AIX 5.3 with xlC12 only. I think we need not add the older
> #defines to openJDK. Also, the comment should be adapted and not mention xlc8 etc.
> Also, there are datastructures for 5.2 which can be removed.
> Removed the older defines.
> I would for now prefer to leave the AIX 5.2 data structures in this file. I am not sure if we could encounter older libperfstat versions on AIX 5.3 too. I plan to rework this coding to get rid of the duplicate structure definitions, but would prefer to do this in a separate patch.
> Where is this used?
> bool libperfstat::get_wparinfo(wparinfo_t* pwi)
> Do we need it if it's not used?
> It is now used :) in os_aix.cpp.
> libperfstat.hpp:835
> I would remove these prototypes commented out.
> I removed them.
> loadlib_aix.cpp
> Why do you do these changes? Please revert them.
> I reverted them.
> os_aix.hpp:219
> What's this good for?
> get_brk_at_startup()
> get_lowest_allocation_above_brk()
> I removed those prototypes.
> os_aix.hpp:259
> What's this good for?
> parse_qsyslib_path()
> I removed this prototype.
> os_aix.cpp:410
> Why do you move query_multipage_support()?
> I moved this back to its original position.
> os_aix.inline.hpp:164
> Why do you reverse the order of the functions here?
> The old order is the same as in the related files os_linux.inline.hpp etc.
> I reversed the order back to original order.
> Best regards,
>   Goetz.
> Kind Regards, Thomas
>> -----Original Message-----
>> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net<mailto:hotspot-dev-bounces at openjdk.java.net>] On
>> Behalf Of Thomas Stüfe
>> Sent: Freitag, 20. November 2015 11:49
>> To: ppc-aix-port-dev at openjdk.java.net<mailto:ppc-aix-port-dev at openjdk.java.net>; HotSpot Open Source Developers
>> Subject: RFR(L): 8143125: [aix] Further Developments for AIX
>> Hi all,
>> please review and sponsor these AIX only changes. Basically, with this
>> change we bring the OpenJDK AIX hotspot port up to speed with the
>> developments done at SAP in the recent months.
>> For a more detailled number of changes and fixes, please refer to the bug
>> description.
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8143125
>> webrev:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8143125-
>> Further/webrev.00/webrev/index.html
>> Kind Regards, Thomas

More information about the hotspot-dev mailing list