[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-ppc-aix-port-dev
Subject: Re: RFR(L): 8143125: [aix] Further Developments for AIX
From: Volker Simonis <volker.simonis () gmail ! com>
Date: 2015-12-03 10:03:07
Message-ID: CA+3eh114N0dy7L49Hmh+aKE1jFDiT=m3XxCDmYk1ZLhW84=Ytg () mail ! gmail ! com
[Download RAW message or body]
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@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@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@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@gmail.com]
> > > Sent: Mittwoch, 25. November 2015 18:01
> > > To: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>
> > > Cc: ppc-aix-port-dev@openjdk.java.net; HotSpot Open Source Developers
> > > <hotspot-dev@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@sap.com<mailto:goetz.lindenmaier@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@openjdk.java.net<mailto:hotspot-dev-bounces@openjdk.java.net>]
> > > > On
> > > > Behalf Of Thomas Stüfe
> > > > Sent: Freitag, 20. November 2015 11:49
> > > > To:
> > > > ppc-aix-port-dev@openjdk.java.net<mailto:ppc-aix-port-dev@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
> > >
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic