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

[Liming Liu]

On Tue, 19 Sep 2023 00:02:12 GMT, Peter B. Kessler <pbk 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.

The code is moved from the Linux-specific code in pretouchTask.cpp. I think it would be fine 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.

Thanks to point out the risk of overflow. I think it originates from the call to `align_up` earlier, and the following assertion would guarantee the computation of `cur + page_size` if overflow did not happen before. So I took back the original logic of inclusive ranges here, and renamed the parameters from `start` and `end` to `first` and `last`. The suggestion on style is also adopted.

> 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.

I placed it near the similar platform-dependent functions. It should be fine not to treat it special.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1329831774
PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1329819641
PR Review Comment: https://git.openjdk.org/jdk/pull/15781#discussion_r1329826784