Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v8]
On Tue, 18 Jan 2022 15:23:42 GMT, Thomas Stuefe wrote: >> Tyler Steele has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains five commits: >> >> - Merge branch 'master' into JDK-8203290 >> - Clean up & testing >> >>- Run JFR tests in test/jdk/jdk/jfr >>- Fix issues found from above test suite >>- Remove unecessary jtreg & gtest tests >>- Remove ineffective `qpic=large` >>- TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java >> - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290 >> - 8203290: Implements Java Flight Recorder on AIX >> >> - changes build system to allow jfr feature on aix >> - implements NetworkPerformance, CPUPerformance, and SystemProcess >> interfaces from os_perf.hpp >> - implements jfr sanity tests >> - 8203290: Implements Java Flight Recorder on AIX >> >> - changes build system to allow jfr feature on aix >> - implements NetworkPerformance, CPUPerformance, and SystemProcess >> interfaces from os_perf.hpp >> - implements jfr sanity tests > > make/autoconf/flags-cflags.m4 line 421: > >> 419: # so for debug we build with '-qpic=large -bbigtoc'. >> 420: DEBUG_CFLAGS_JVM="-qpic=large" >> 421: fi > > Why this removal? Note that getting the TOC not to explode on AIX has been an > ongoing struggle, see this string of associated JBS issues: > > https://bugs.openjdk.java.net/browse/JDK-8184344 > https://bugs.openjdk.java.net/browse/JDK-8171408 > https://bugs.openjdk.java.net/browse/JDK-8196488 > https://bugs.openjdk.java.net/browse/JDK-8204935 Hmm, that is strange. I initially thought my code had caused the TOC overflow, and added -qpic=large to address this. After realizing that my code was not solely responsible, I (tried) to back out of my changes without removing anything I had not added myself. It seems I should review the changes to this file one more time to ensure that I didn't get a bit carried away with the removal process. Good catch. - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v8]
On Wed, 12 Jan 2022 15:18:46 GMT, Tyler Steele wrote: >> Just in time for the holidays I have completed an implementation of the JFR >> functionality for AIX. As a side note, this is my first submission to >> OpenJDK >> >> ### Implementation notes and alternatives considered >> >> After modifying the build system to allow the --enable-jvm-feature-jfr to >> work on AIX, my task was to implement the interfaces from os_perf.hpp. The >> os_perf_aix.cpp implementation that existed was, I believe, a copy of the >> Linux implementation. A review of the code in that file showed that >> NetworkInterface, CPUPerformanceInterface, and SystemProcessInterface would >> require modification to work on AIX. Using the Linux implementation as a >> guide, I initially expected to use files from the aix procfs like >> /proc//psinfo, and /proc//status in a manner similar to the Linux >> implementation. However, I ended up using libperfstat for all functionality >> required by the interfaces. >> >> ### Testing >> >> Testing for JFR seems to be quite light in the repo both before and after >> this change. In addition to performing manual testing, I have added some >> basic sanity checks that ensure events can be created and committed (using >> jtreg), and performs some basic checks on the results of the interface >> member functions (using gtest). >> >> ### More notes >> >> I've sent an email to the JFR group about a TOC overflow warning I >> encountered while building (for the release server target). I believe the >> fix is to pass -qpic=large when using the xlc toolchain, but my >> modifications to flags-cflags.m4 have not been successful in removing this >> warning. > > Tyler Steele has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into JDK-8203290 > - Clean up & testing > >- Run JFR tests in test/jdk/jdk/jfr >- Fix issues found from above test suite >- Remove unecessary jtreg & gtest tests >- Remove ineffective `qpic=large` >- TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java > - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290 > - 8203290: Implements Java Flight Recorder on AIX > > - changes build system to allow jfr feature on aix > - implements NetworkPerformance, CPUPerformance, and SystemProcess > interfaces from os_perf.hpp > - implements jfr sanity tests > - 8203290: Implements Java Flight Recorder on AIX > > - changes build system to allow jfr feature on aix > - implements NetworkPerformance, CPUPerformance, and SystemProcess > interfaces from os_perf.hpp > - implements jfr sanity tests Hi Tyler, nice to see this effort. I'm curious, are you associated with IBM? This is not a full review, but I can take a closer look over the next days if necessary (but looks like you already got your full set of reviewers). About ThreadCrashProtection, we unified that into os_posix.cpp, so it should work out of the box on any Posix system. Its the usual sigsetjmp/siglongjmp game, covering SIGBUS and SIGSEGV. I am not aware of any AIX peculiarities which would prevent this from working on AIX. Cheers, Thomas make/autoconf/flags-cflags.m4 line 421: > 419: # so for debug we build with '-qpic=large -bbigtoc'. > 420: DEBUG_CFLAGS_JVM="-qpic=large" > 421: fi Why this removal? Note that getting the TOC not to explode on AIX has been an ongoing struggle, see this string of associated JBS issues: https://bugs.openjdk.java.net/browse/JDK-8184344 https://bugs.openjdk.java.net/browse/JDK-8171408 https://bugs.openjdk.java.net/browse/JDK-8196488 https://bugs.openjdk.java.net/browse/JDK-8204935 - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v8]
On Wed, 12 Jan 2022 15:18:46 GMT, Tyler Steele wrote:
>> Just in time for the holidays I have completed an implementation of the JFR
>> functionality for AIX. As a side note, this is my first submission to
>> OpenJDK
>>
>> ### Implementation notes and alternatives considered
>>
>> After modifying the build system to allow the --enable-jvm-feature-jfr to
>> work on AIX, my task was to implement the interfaces from os_perf.hpp. The
>> os_perf_aix.cpp implementation that existed was, I believe, a copy of the
>> Linux implementation. A review of the code in that file showed that
>> NetworkInterface, CPUPerformanceInterface, and SystemProcessInterface would
>> require modification to work on AIX. Using the Linux implementation as a
>> guide, I initially expected to use files from the aix procfs like
>> /proc//psinfo, and /proc//status in a manner similar to the Linux
>> implementation. However, I ended up using libperfstat for all functionality
>> required by the interfaces.
>>
>> ### Testing
>>
>> Testing for JFR seems to be quite light in the repo both before and after
>> this change. In addition to performing manual testing, I have added some
>> basic sanity checks that ensure events can be created and committed (using
>> jtreg), and performs some basic checks on the results of the interface
>> member functions (using gtest).
>>
>> ### More notes
>>
>> I've sent an email to the JFR group about a TOC overflow warning I
>> encountered while building (for the release server target). I believe the
>> fix is to pass -qpic=large when using the xlc toolchain, but my
>> modifications to flags-cflags.m4 have not been successful in removing this
>> warning.
>
> Tyler Steele has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains five commits:
>
> - Merge branch 'master' into JDK-8203290
> - Clean up & testing
>
>- Run JFR tests in test/jdk/jdk/jfr
>- Fix issues found from above test suite
>- Remove unecessary jtreg & gtest tests
>- Remove ineffective `qpic=large`
>- TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java
> - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290
> - 8203290: Implements Java Flight Recorder on AIX
>
> - changes build system to allow jfr feature on aix
> - implements NetworkPerformance, CPUPerformance, and SystemProcess
> interfaces from os_perf.hpp
> - implements jfr sanity tests
> - 8203290: Implements Java Flight Recorder on AIX
>
> - changes build system to allow jfr feature on aix
> - implements NetworkPerformance, CPUPerformance, and SystemProcess
> interfaces from os_perf.hpp
> - implements jfr sanity tests
You could test it like this:
In JfrThreadSampler.cpp, you have the OSThreadSampler::protected_task()
function:
Insert the following to crash the system:
void OSThreadSampler::protected_task(const os::SuspendedThreadTaskContext&
context) {
+int* null_ptr = nullptr;
+*null_ptr = 5;
...
If crash protection is enabled and active, for release builds, you will not get
a system crash. Instead, you will get this output (in debug builds you will get
a crash as expected):
[10.740s][error][jfr] Thread method sampler crashed
[10.786s][error][jfr] Thread method sampler crashed
[10.798s][error][jfr] Thread method sampler crashed
[10.810s][error][jfr] Thread method sampler crashed
[10.822s][error][jfr] Thread method sampler crashed
[10.834s][error][jfr] Thread method sampler crashed
[10.847s][error][jfr] Thread method sampler crashed
[10.858s][error][jfr] Thread method sampler crashed
...
-
PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v8]
On Wed, 12 Jan 2022 19:29:48 GMT, Markus Grönlund wrote: > Speaking of the thread sampler, to ensure stability, you also need a working > implementation of os::ThreadCrashProtection. I can see there is the one in > os_posix.cpp, and maybe that is the one already in use? Or does there need to > be a specialization in os_aix.cpp? Please see > https://bugs.openjdk.java.net/browse/JDK-8279077 for more details about crash > protection. Thanks for bringing this to my attention @mgronlun. I spent some time looking over crash protection, and I believe the implementation in os_posix is brought in for >= jdk16. If I understand the project history (especially relating to the issue you linked), this would not necessarily be true for the project before jdk 16, so it will need to be considered if this change is backported. Is there a way to be sure that ThreadCrashProtection is present on AIX? I have forced a segfault, and it appears to behave the same on Linux x86 as it does on AIX Power. - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v8]
On 15/01/2022 2:13 am, Tyler Steele wrote: To follow up on an item mentioned above so that it is documented here: I reviewed the functionality from `src/hotspot/os/aix/libperfstat_aix` and found it didn't fit well with the needs of this pr. If it were extended in the future to include the ability to call at least `libperfstat_process` and `libperfstat_cpu` (in addition to `libperfstat_cpu_total`), then using it here would be a reasonable choice. There may be some other functionality required, but those stand out to me as the big ones. Wouldn't adding the needed functionality to the existing libperfstat_aix be preferable here? David - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v8]
On Wed, 12 Jan 2022 15:18:46 GMT, Tyler Steele wrote: >> Just in time for the holidays I have completed an implementation of the JFR >> functionality for AIX. As a side note, this is my first submission to >> OpenJDK >> >> ### Implementation notes and alternatives considered >> >> After modifying the build system to allow the --enable-jvm-feature-jfr to >> work on AIX, my task was to implement the interfaces from os_perf.hpp. The >> os_perf_aix.cpp implementation that existed was, I believe, a copy of the >> Linux implementation. A review of the code in that file showed that >> NetworkInterface, CPUPerformanceInterface, and SystemProcessInterface would >> require modification to work on AIX. Using the Linux implementation as a >> guide, I initially expected to use files from the aix procfs like >> /proc//psinfo, and /proc//status in a manner similar to the Linux >> implementation. However, I ended up using libperfstat for all functionality >> required by the interfaces. >> >> ### Testing >> >> Testing for JFR seems to be quite light in the repo both before and after >> this change. In addition to performing manual testing, I have added some >> basic sanity checks that ensure events can be created and committed (using >> jtreg), and performs some basic checks on the results of the interface >> member functions (using gtest). >> >> ### More notes >> >> I've sent an email to the JFR group about a TOC overflow warning I >> encountered while building (for the release server target). I believe the >> fix is to pass -qpic=large when using the xlc toolchain, but my >> modifications to flags-cflags.m4 have not been successful in removing this >> warning. > > Tyler Steele has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into JDK-8203290 > - Clean up & testing > >- Run JFR tests in test/jdk/jdk/jfr >- Fix issues found from above test suite >- Remove unecessary jtreg & gtest tests >- Remove ineffective `qpic=large` >- TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java > - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290 > - 8203290: Implements Java Flight Recorder on AIX > > - changes build system to allow jfr feature on aix > - implements NetworkPerformance, CPUPerformance, and SystemProcess > interfaces from os_perf.hpp > - implements jfr sanity tests > - 8203290: Implements Java Flight Recorder on AIX > > - changes build system to allow jfr feature on aix > - implements NetworkPerformance, CPUPerformance, and SystemProcess > interfaces from os_perf.hpp > - implements jfr sanity tests Thanks! That clarifies some things. The output for that test is currently empty, so it seems that the Native Libs are not being included in the recording. - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v8]
On Wed, 12 Jan 2022 15:18:46 GMT, Tyler Steele wrote: >> Just in time for the holidays I have completed an implementation of the JFR >> functionality for AIX. As a side note, this is my first submission to >> OpenJDK >> >> ### Implementation notes and alternatives considered >> >> After modifying the build system to allow the --enable-jvm-feature-jfr to >> work on AIX, my task was to implement the interfaces from os_perf.hpp. The >> os_perf_aix.cpp implementation that existed was, I believe, a copy of the >> Linux implementation. A review of the code in that file showed that >> NetworkInterface, CPUPerformanceInterface, and SystemProcessInterface would >> require modification to work on AIX. Using the Linux implementation as a >> guide, I initially expected to use files from the aix procfs like >> /proc//psinfo, and /proc//status in a manner similar to the Linux >> implementation. However, I ended up using libperfstat for all functionality >> required by the interfaces. >> >> ### Testing >> >> Testing for JFR seems to be quite light in the repo both before and after >> this change. In addition to performing manual testing, I have added some >> basic sanity checks that ensure events can be created and committed (using >> jtreg), and performs some basic checks on the results of the interface >> member functions (using gtest). >> >> ### More notes >> >> I've sent an email to the JFR group about a TOC overflow warning I >> encountered while building (for the release server target). I believe the >> fix is to pass -qpic=large when using the xlc toolchain, but my >> modifications to flags-cflags.m4 have not been successful in removing this >> warning. > > Tyler Steele has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into JDK-8203290 > - Clean up & testing > >- Run JFR tests in test/jdk/jdk/jfr >- Fix issues found from above test suite >- Remove unecessary jtreg & gtest tests >- Remove ineffective `qpic=large` >- TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java > - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290 > - 8203290: Implements Java Flight Recorder on AIX > > - changes build system to allow jfr feature on aix > - implements NetworkPerformance, CPUPerformance, and SystemProcess > interfaces from os_perf.hpp > - implements jfr sanity tests > - 8203290: Implements Java Flight Recorder on AIX > > - changes build system to allow jfr feature on aix > - implements NetworkPerformance, CPUPerformance, and SystemProcess > interfaces from os_perf.hpp > - implements jfr sanity tests To follow up on an item mentioned above so that it is documented here: I reviewed the functionality from `src/hotspot/os/aix/libperfstat_aix` and found it didn't fit well with the needs of this pr. If it were extended in the future to include the ability to call at least `libperfstat_process` and `libperfstat_cpu` (in addition to `libperfstat_cpu_total`), then using it here would be a reasonable choice. There may be some other functionality required, but those stand out to me as the big ones. - PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v8]
On Wed, 12 Jan 2022 15:18:46 GMT, Tyler Steele wrote: >> Just in time for the holidays I have completed an implementation of the JFR >> functionality for AIX. As a side note, this is my first submission to >> OpenJDK >> >> ### Implementation notes and alternatives considered >> >> After modifying the build system to allow the --enable-jvm-feature-jfr to >> work on AIX, my task was to implement the interfaces from os_perf.hpp. The >> os_perf_aix.cpp implementation that existed was, I believe, a copy of the >> Linux implementation. A review of the code in that file showed that >> NetworkInterface, CPUPerformanceInterface, and SystemProcessInterface would >> require modification to work on AIX. Using the Linux implementation as a >> guide, I initially expected to use files from the aix procfs like >> /proc//psinfo, and /proc//status in a manner similar to the Linux >> implementation. However, I ended up using libperfstat for all functionality >> required by the interfaces. >> >> ### Testing >> >> Testing for JFR seems to be quite light in the repo both before and after >> this change. In addition to performing manual testing, I have added some >> basic sanity checks that ensure events can be created and committed (using >> jtreg), and performs some basic checks on the results of the interface >> member functions (using gtest). >> >> ### More notes >> >> I've sent an email to the JFR group about a TOC overflow warning I >> encountered while building (for the release server target). I believe the >> fix is to pass -qpic=large when using the xlc toolchain, but my >> modifications to flags-cflags.m4 have not been successful in removing this >> warning. > > Tyler Steele has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains five commits: > > - Merge branch 'master' into JDK-8203290 > - Clean up & testing > >- Run JFR tests in test/jdk/jdk/jfr >- Fix issues found from above test suite >- Remove unecessary jtreg & gtest tests >- Remove ineffective `qpic=large` >- TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java > - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290 > - 8203290: Implements Java Flight Recorder on AIX > > - changes build system to allow jfr feature on aix > - implements NetworkPerformance, CPUPerformance, and SystemProcess > interfaces from os_perf.hpp > - implements jfr sanity tests > - 8203290: Implements Java Flight Recorder on AIX > > - changes build system to allow jfr feature on aix > - implements NetworkPerformance, CPUPerformance, and SystemProcess > interfaces from os_perf.hpp > - implements jfr sanity tests Hi Tyler, Good work in getting JFR ported - thank you. Most of the work revolved around implementing the platform-specific os_perf* functionality. I had assumed you would need more work concerning the thread sampler, i.e. JFR method sampling, but it seems that os/posix/signals_posix.cpp can be used as-is (for suspend / resume) and that pd_get_top_frame_for_profiling() already have implementations - that's good in that case. Speaking of the thread sampler, to ensure stability, you also need a working implementation of os::ThreadCrashProtection. I can see there is the one in os_posix.cpp, and maybe that is the one already in use? Or does there need to be a specialization in os_aix.cpp? Please see https://bugs.openjdk.java.net/browse/JDK-8279077 for more details about crash protection. I can't comment on the os_perf_aix.cpp changes from a ppc/aix viewpoint - I think Martin has taken care of that already. I am ok with this from a JFR standpoint. Thanks again Markus - Marked as reviewed by mgronlun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6885
Re: RFR: 8203290: [PPC64, s390] Check functionality of JDK-8199712 (Flight Recorder) [v8]
> Just in time for the holidays I have completed an implementation of the JFR > functionality for AIX. As a side note, this is my first submission to OpenJDK > > > ### Implementation notes and alternatives considered > > After modifying the build system to allow the --enable-jvm-feature-jfr to > work on AIX, my task was to implement the interfaces from os_perf.hpp. The > os_perf_aix.cpp implementation that existed was, I believe, a copy of the > Linux implementation. A review of the code in that file showed that > NetworkInterface, CPUPerformanceInterface, and SystemProcessInterface would > require modification to work on AIX. Using the Linux implementation as a > guide, I initially expected to use files from the aix procfs like > /proc//psinfo, and /proc//status in a manner similar to the Linux > implementation. However, I ended up using libperfstat for all functionality > required by the interfaces. > > ### Testing > > Testing for JFR seems to be quite light in the repo both before and after > this change. In addition to performing manual testing, I have added some > basic sanity checks that ensure events can be created and committed (using > jtreg), and performs some basic checks on the results of the interface member > functions (using gtest). > > ### More notes > > I've sent an email to the JFR group about a TOC overflow warning I > encountered while building (for the release server target). I believe the fix > is to pass -qpic=large when using the xlc toolchain, but my modifications to > flags-cflags.m4 have not been successful in removing this warning. Tyler Steele has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits: - Merge branch 'master' into JDK-8203290 - Clean up & testing - Run JFR tests in test/jdk/jdk/jfr - Fix issues found from above test suite - Remove unecessary jtreg & gtest tests - Remove ineffective `qpic=large` - TODO: jdk/jfr/event/runtime/TestNativeLibrariesEvent.java - Merge branch 'JDK-8203290' of github.com:backwaterred/jdk into JDK-8203290 - 8203290: Implements Java Flight Recorder on AIX - changes build system to allow jfr feature on aix - implements NetworkPerformance, CPUPerformance, and SystemProcess interfaces from os_perf.hpp - implements jfr sanity tests - 8203290: Implements Java Flight Recorder on AIX - changes build system to allow jfr feature on aix - implements NetworkPerformance, CPUPerformance, and SystemProcess interfaces from os_perf.hpp - implements jfr sanity tests - Changes: https://git.openjdk.java.net/jdk/pull/6885/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6885=07 Stats: 831 lines in 5 files changed: 151 ins; 514 del; 166 mod Patch: https://git.openjdk.java.net/jdk/pull/6885.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6885/head:pull/6885 PR: https://git.openjdk.java.net/jdk/pull/6885
