RFR: 8315923: pretouch_memory by atomic-add-0 fragments huge pages unexpectedly

Wed Sep 27 12:04:46 UTC 2023

On Mon, 18 Sep 2023 07:37:26 GMT, Liming Liu <duke at openjdk.org> wrote:

> As described at [JDK-8315923](https://bugs.openjdk.org/browse/JDK-8315923), this patch uses madvise with MADV_POPULATE_WRITE to pretouch memory when supported (since kernel 5.14).
> 
> Ran the newly added jtreg test on 64c Neoverse-N1 machines with kernel 4.18, 5.13 and 6.1, and observed that transparent huge pages formed right after pretouch on kernel 6.1. Recorded the time spent on the test in *seconds* with `VERBOSE=time` as the table below, and got that the patch takes improvements when the system call is supported, while does not hurt if not supported:
> 
> <table>
>   <tr>
>     <th>Kernel</th>
>     <th colspan="2"><tt>-XX:-TransparentHugePages</tt></th>
>     <th colspan="2"><tt>-XX:+TransparentHugePages</tt></th>
>   </tr>
>   <tr><td></td><td>Unpatched</td><td>Patched</td><td>Unpatched</td><td>Patched</td></tr>
>   <tr><td>4.18</td><td>11.30</td><td>11.30</td><td>0.25</td><td>0.25</td></tr>
>   <tr><td>5.13</td><td>0.22</td><td>0.22</td><td>3.42</td><td>3.42</td></tr>
>   <tr><td>6.1</td><td>0.27</td><td>0.33</td><td>3.54</td><td>0.33</td></tr>
> </table>

src/hotspot/os/linux/os_linux.cpp line 2914:

> 2912:       // will initially always use small pages.
> 2913:       page_size = UseTransparentHugePages ? (size_t)os::vm_page_size() : page_size;
> 2914:       pretouch_memory_fallback(start, end, page_size);

This assignment should be to a new variable named `pretouch_page_size` since it will only be used by `pretouch_memory_fallback`, and otherwise modifies the function parameter and also shadows the variable from `class os`.  Declaring it `const` would get you extra points from readers, but I accept that is not the style here.

src/hotspot/share/runtime/os.cpp line 2122:

> 2120:     Atomic::add(reinterpret_cast<int *>(cur), 0, memory_order_relaxed);
> 2121:     cur += page_size;
> 2122:   } while (cur < end);

The previous code was careful to touch only as far as `align_down(static_cast<char*>(end) - 1, page_size)` to avoid the case where the region to be pre-touched ended at the end of the address space.  (There is a comment about that.)  Your code might compute `cur + page_size` and wrap around to the beginnng of the address space.  You do not want to debug that, or have someone curse you for making them debug it.

Your code might find that `cur < end` is never true if `end == 0` because you do not do the `-1` and the `align_down` and the comparison is unslgned.

A style comment: I usually write loops like the old line 2113 as `for ( ; /* break *; cur += page_size) {` to warn the reader to watch for a `break` from the loop.

src/hotspot/share/runtime/os.hpp line 226:

> 224:   static void   pd_free_memory(char *addr, size_t bytes, size_t alignment_hint);
> 225:   static void   pd_realign_memory(char *addr, size_t bytes, size_t alignment_hint);
> 226:   static void   pd_pretouch_memory(void *start, void *end, size_t page_size);

Should this be next to the declaration of `os::pretouch_memory` to warn the reader that there might be platform-dependent implementations that have to be considered?  If you move this method, I would move `pretouch_memory_fallback` also.

src/hotspot/share/runtime/os.hpp line 229:

> 227: 
> 228:   // Fallback to this if OS needs no specific treatment
> 229:   static void   pretouch_memory_fallback(void *start, void *end, size_t page_size);

Would `pretouch_memory_common` be a better name for this method?  The name should reflect why the method exists, and I think of this as common code to avoid having a copy of the implementation in each platform-specific class.  You don't "fall back" to this implementation: on most platforms, this is the only implementation.  If `class os` had virtual methods, this would be the base class implementation.  

(Not a big deal.)

test/hotspot/jtreg/gc/parallel/TestParallelAlwaysPreTouch.java line 31:

> 29:  *
> 30:  * @run main/othervm -XX:+UseParallelGC -XX:ParallelGCThreads=${os.processors}
> 31:  *                   -Xlog:startuptime,pagesize,gc+heap=debug

I don't think there is anything ParallelGC-specific about the issue.  If so, you could leave off the `Use...GC` argument (fewer arguments are better), and maybe add a comment in the test that the issue is present in both G1GC and ParallelGC.  Extra points for testing any other collectors that support `-XX:+AlwaysPreTouch`.

test/hotspot/jtreg/gc/parallel/TestParallelAlwaysPreTouch.java line 33:

> 31:  *                   -Xlog:startuptime,pagesize,gc+heap=debug
> 32:  *                   -Xms24G -Xmx24G -XX:+AlwaysPreTouch
> 33:  *                   -XX:-UseTransparentHugePages

I would move the `-XX:-UseTransparentHugePages` and `-XX:+UseTransparentHugePages` to be the first argument on their command lines, to emphasize the only argument that is changing between the two runs.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1329403622
PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1329369688
PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1329389651
PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1329395341
PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1329427162
PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1329425068