RFR(L): 8143125: [aix] Further Developments for AIX
Volker Simonis
volker.simonis at gmail.com
Thu Dec 3 10:03:07 UTC 2015
Great - I think the change is now ready to push :)
Thanks,
Volker
On Thu, Dec 3, 2015 at 10:07 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> Hi Volker,
>
> thank you for all that work! See my comments inline.
>
> On Wed, Dec 2, 2015 at 11:19 AM, Volker Simonis <volker.simonis at gmail.com>
> wrote:
>>
>> Hi Thomas,
>>
>> thanks a lot for this nice cleanup. Please find my remaining comments
>> below:
>>
>> libo4.hpp
>> =======
>> - 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.
>>
>> libo4.cpp
>> =======
>> - please update the copyright year
>>
>> vmStructs_aix_ppc.hpp
>> =================
>> - please update the copyright year
>>
>>
>
> All fixed.
>
>>
>> misc_aix.hpp
>> ==========
>> - "trc" is defined to the empty string and doesn't seemed to be used
>> anywhere:
>>
>> 45 #define trc(fmt, ...)
>>
>> do we still need it?
>>
>
> I removed it.
>
>>
>> os_aix.cpp
>> ========
>> - 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
>>
>
> Fixed.
>
>>
>> - 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.
>>
>
> Fixed. All those trcVerbose calls are ugly crutches; I plan to transform all
> these trcVerbose calls to the new Unified Logging in the near future, just
> to get a feel for it.
>
>>
>> - 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() ?
>>
>
> Good catch, fixed.
>
>>
>> - 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 ?
>>
>
> Unfortunately, I cannot be sure. It may be possible to find out, but it
> seems easier to just do 4K paged touching. Once the page is paged in, it
> should not cost much. Note also that this is a development option and
> normally off.
>
>>
>> - 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().
>>
>
> True. But I still left that in, because the other platforms have the same
> code in them (eg bsd and solaris, which both don't seem to support large
> pages either). This is worth a cleanup, but should be done cross platform in
> a different patch.
>
>>
>> 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}
>>
>
> Ok, I removed the AIX 5.2 structure definitions. As I explained before, I
> was afraid of cutting off old, unpatched AIX 5.3 releases as well, if they
> still happen to carry an old libperfstat. But you guys seem to really want
> this removed, so I removed it.
>
>>
>> loadlib_aix.cpp
>> ==================
>> - please update the copyright year
>
>
> Fixed
>>
>>
>> misc_aix.{hpp,cpp}
>> ==============
>> - 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.
>>
>
> Ok, I removed MiscUtils::desribe_errno().
>
> I really do not like strerror(), though:
> - it is not threadsafe, which is quite unnecessary
> - strerror_r is threadsafe but may fail and requires error handling if done
> correctly
> - it gives lenghty, localized (!) error descriptions, which clog up log
> files and are less easy to grep for.
> - it carries the risk of not printing anything at all if the locale is set
> weird, e.g. "????" for japanese LC_MESSAGES. Just google for "strerror
> useless"
>
> All in all I think strerror is not good for the task of printing errno
> values into log files which are to be consumed by programmers, not users.
>
> But for now I removed describe_errno and replaced it with printing the
> numeric value of the errno. Maybe we can have this strerror discussion in
> the mailing list, see what others think.
>
>>
>> - 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
>>
>> https://www-01.ibm.com/support/knowledgecenter/ssw_aix_53/com.ibm.aix.basetechref/doc/basetrf2/sleep.htm))).
>> So please remove MiscUtils::sleep_ms().
>>
>
> Ok, I did remove this and replaced the one call with plain usleep.
>
> The error was embarrassing :) thanks for catching that.
>
> As for the "1.000.000" restriction, that comes from Linux.
> MiscUtils::sleep_ms is used crossplatform in the SAP JVM.
>
>
>>
>> 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,
>> Volker
>>
>
> Thanks again!
>
> ..Thomas
>
>
>>
>>
>> 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 ppc-aix-port-dev
mailing list