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

[Kim Barrett]

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>

PretouchTask attempts to parallelize the pretouching.  How well does that work
with the use of MADV_POPULATE_WRITE?

src/hotspot/share/gc/shared/pretouchTask.cpp line 75:

> 73:   // initially always use small pages.
> 74:   page_size = UseTransparentHugePages ? (size_t)os::vm_page_size() : page_size;
> 75: #endif

I never liked this, so happy to see it gone.

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

> 2115: }
> 2116: 
> 2117: void os::pretouch_memory_common(void *first, void *last, size_t page_size) {

Suggest asserting `first` and `last` are `page_size` aligned.  Also maybe assert `first <= last` here too.

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

> 2117: void os::pretouch_memory_common(void *first, void *last, size_t page_size) {
> 2118:   for (char *cur = static_cast<char *>(first); /* break */; cur += page_size) {
> 2119:     Atomic::add(reinterpret_cast<int *>(cur), 0, memory_order_relaxed);

Throughout, HotSpot style is generally to cuddle a ptr-operator with the type,
e.g. `char* foo` rather than `char *foo`, and similarly in the casts.

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 *first, void *last, size_t page_size);

I wish we had a better pattern than a (usually identical) version of a function for each platform, but this
is consistent with how similar things are done elsewhere.  Good that you provide a common helper.  So
okay.

And yes, I see that the existing nearby declarations are inconsistent with what I suggested is usual HotSpot
style regarding the ptr-operator placement.  This file is particularly inconsistent in that respect, sometimes
even in the same declaration.  But the substantial majority are type cuddled.  In this particular case I'd not
object to being consistent with the adjacent code.

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

> 47: public class TestParallelAlwaysPreTouch {
> 48:   public static void main(String[] args) throws Exception {
> 49:     // everything should happen before entry point

This isn't really testing anything beyond not crashing with the given options, and giving some log messages
for manual examination.  It would be better if some examination of the log messages could be performed to
verify expected behavior.

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

PR Review: https://git.openjdk.org/jdk/pull/15781#pullrequestreview-1650014869
PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1340977019
PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1340872840
PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1340872628
PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1340878255
PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1340947249