RFR: 8262188: Add test to verify trace page sizes logging on Linux

[Stefan Johansson]

On Wed, 24 Feb 2021 10:17:26 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Please review this new test that checks the output from `-Xlog:pagesize` and compares it to what's really used according to `/proc/self/smaps`.
>> 
>> We already have a few tests that check the output from `-Xlog:pagesize`, the difference is that those tests expect some page size given the parameters used. This new test instead check if the values printed out are correct with regards to what is recorded in `/proc/self/maps`.
>> 
>> I wrote the test to see if all our logging was correct, and found those two issues that are now fixed:
>> [JDK-8261029: Code heap page sizes not traced correctly using os::trace_page_sizes](https://bugs.openjdk.java.net/browse/JDK-8261029)
>> [JDK-8261230: GC tracing of page sizes are wrong in a few places](https://bugs.openjdk.java.net/browse/JDK-8261230)
>> 
>> The test now passes and it will be a good way to prevent incorrect page size logging to be added going forward.
>> 
>> **Testing**
>> The new test has been run through mach5 without any problems and also manually on configurations with explicit large pages enabled.
>
> test/hotspot/jtreg/runtime/os/TestTracePageSizes.java line 51:
> 
>> 49:  * @run main/othervm -XX:+AlwaysPreTouch -Xlog:pagesize:ps-%p.log -XX:-SegmentedCodeCache -XX:+UseLargePages TestTracePageSizes
>> 50:  * @run main/othervm -XX:+AlwaysPreTouch -Xlog:pagesize:ps-%p.log -XX:-SegmentedCodeCache -XX:+UseTransparentHugePages TestTracePageSizes
>> 51:  */
> 
> When do you use multiple @run, when multiple @test?

I might be using this wrong, but the way I use it is that for each `@test` I can have a set of `@requires` and then I do a set of runs under these requirements. 

Sometimes specifying explicit tests for the different GCs are considered to expensive but in this case I think it is fine.

> test/hotspot/jtreg/runtime/os/TestTracePageSizes.java line 81:
> 
>> 79:  * @run main/othervm -XX:+AlwaysPreTouch -Xlog:pagesize:ps-%p.log -XX:+UseSerialGC -XX:+UseLargePages TestTracePageSizes
>> 80:  * @run main/othervm -XX:+AlwaysPreTouch -Xlog:pagesize:ps-%p.log -XX:+UseSerialGC -XX:+UseTransparentHugePages TestTracePageSizes
>> 81: */
> 
> I see you don't explicitly test UseSHM. Because the number of tests would be too large?

I don't explicitly test HugeTLBFS either, but yes that is what's being tested when just setting `+UseLargePages` =) Would you prefer if I had one `@run` for `-XX:+UseHugeTLBFS` and one for `-XX:+UseSHM`.

> test/hotspot/jtreg/runtime/os/TestTracePageSizes.java line 102:
> 
>> 100:     // match as little as possible so each "segment" in the file
>> 101:     // will generate a match.
>> 102:     private static void parseSmaps() throws Exception {
> 
> Code may be easier to read if the pattern were defined up here.

I agree, I moved it around and ended up leaving it below, but now with the comment it certainly make sense to have it here.

> test/hotspot/jtreg/runtime/os/TestTracePageSizes.java line 104:
> 
>> 102:     private static void parseSmaps() throws Exception {
>> 103:         Pattern smapsPattern = Pattern.compile(RangeWithPageSize.smapsPatternString, Pattern.DOTALL);
>> 104:         Scanner smapsScanner = new Scanner(new File("/proc/self/smaps"));
> 
> Would that work if mappings are concurrently modified? (I guess it has to otherwise one could not read reliably from /proc at all)

I can't say for sure, not sure exactly how the file is consumed by the Scanner. The segments and the fields in those segments that we really care about should not change and therefore I think we should be fine.

> test/hotspot/jtreg/runtime/os/TestTracePageSizes.java line 168:
> 
>> 166: 
>> 167:         // The test needs to be run with page size logging printed to ps-$pid.log.
>> 168:         Scanner fileScanner = new Scanner(new File("./ps-" + ProcessHandle.current().pid() + ".log"));
> 
> So we scan the log file while it is being written by ourselves? This seems slightly unsafe. Are we sure it has been completely flushed at this point? I'm mainly concerned about things like running this test in a current dir on NFS or somesuch.
> 
> An alternative would be to let the testee write the log file and print out a copy of /proc/self/smaps to stdout, then to scan and compare in a separate process. 
> 
> But if you think this is safe enough, I'm fine with this too.

Not really while it is being written, since all the output from `-Xlog:pagesize` happens before the java test is executed and from what I can tell the log-file should be properly flushed.

As you say doing the scan and compare in a separate process afterwards would make any concurrent modification issue and flush issue go away. But it would also slightly complicate the test and make it depend on the test library. For debugging it is very nice to be able to run the test standalone and that is a bit harder when depending on the test lib.

Until proven unsafe I would prefer to leave it as is.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2696