RFR(L): 8143125: [aix] Further Developments for AIX
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Dec 3 09:07:12 UTC 2015
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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/attachments/20151203/cad5af61/attachment-0001.html>
More information about the ppc-aix-port-dev
mailing list