RFR: 8272807: Permit use of memory concurrent with pretouch [v2]

[Thomas Schatzl]

On Tue, 21 Sep 2021 23:01:08 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Note that this PR replaces the withdrawn https://github.com/openjdk/jdk/pull/5215.
>> 
>> Please review this change which adds os::touch_memory, which is similar to
>> os::pretouch_memory but allows concurrent access to the memory while it is
>> being touched.  This is accomplished by using an atomic add of zero as the
>> operation for touching the memory, ensuring the virtual location is backed
>> by physical memory while not changing any values being read or written by
>> other threads.
>> 
>> While I was there, fixed some other lurking issues in os::pretouch_memory.
>> There was a potential overflow in the iteration that has been fixed.  And if
>> the range arguments weren't page aligned then the last page might not get
>> touched.  The latter was even mentioned in the function's description.  Both
>> of those have been fixed by careful alignment and some extra checks.  The
>> resulting code is a little more complicated, but more robust and complete.
>> 
>> Similarly added TouchTask, which is similar to PretouchTask.  Again here,
>> there is some cleaning up to avoid potential overflows and such.
>> 
>> - The chunk size is computed using the page size after possible adjustment
>> for UseTransparentHugePages.  We want a chunk size that reflects the actual
>> number of touches that will be performed.
>> 
>> - The chunk claim is now done using a CAS that won't exceed the range end.
>> The old atomic-fetch-and-add and check the result, which is performed by
>> each worker thread, could lead to overflow.  The old code has a test for
>> overflow, but since pointer-arithmetic overflow is UB that's not reliable.
>> 
>> - The old calculation of num_chunks for parallel touching could also
>> potentially overflow.
>> 
>> Testing:
>> mach5 tier1-3
>
> Kim Barrett has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Merge branch 'master' into touch_memory
>  - simplify touch_impl, using conditional on bool arg rather than template specialization
>  - touch task
>  - add touch_memory

Other than the comments, the change looks good.

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

> 103:                             size_t page_size,
> 104:                             WorkGang* pretouch_gang) {
> 105:   PretouchTask task{task_name, start, end, page_size};

I would prefer to use the regular braces (also in TouchTask::touch()) for consistency with other code here, but is fine with me. Just unusual to see in Hotspot code.

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

> 1867: 
> 1868: void os::touch_memory(void* start, void* end, size_t page_size) {
> 1869:   check_touch_memory_args(start, end, page_size);

Fwiw, the refactorings look good, thanks for fixing the issues, but due to the amount of code changed (compared to the actual addition of pretouch vs. touch) I would also have preferred this to be a separate change.

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

> 374:   // precondition: start <= end.
> 375:   static void pretouch_memory(void* start, void* end, size_t page_size = vm_page_size());
> 376:   static void touch_memory(void* start, void* end, size_t page_size = vm_page_size());

Imho the naming is really bad. "Pretouch" and "touch" to me do not differ in the strength of the touch which I assume is intended here, but when the action is done. So this is really confusing to me. I am not a linguist, and looking up synonyms did not yield anything good (that is commonly recognized too) though.

Did you consider adding a parameter to the `os::pretouch` method instead of two so confusingly named ones (something like `destructive` to indicate that one actually changes the memory that is touched)?

Maybe this allows flattening the `BasicTouchTask` hierarchy to just pass along that flag too, potentially saving a lot of code implementing just that (i.e. the only difference of the two child classes is that they call the respective touch method) unless I'm mistaken.

There may still be two different subclasses or helper methods if desired. But maybe there is a reason to have the subclasses in some follow-up?

E.g. one option (which may be too much change here since it takes information hiding even further, something for an extra CR maybe) could be to only have a public `PretouchHelper` class in the .hpp file with one (or two) methods and move all implementation detail into the .cpp file.

Like

class PretouchHelper {
    static void pretouch(const char* task_name, char* start_address, char* end_address,
                       size_t page_size, WorkGang* pretouch_gang, bool destructive = false); /* safe but slow */

OR

    static void pretouch(const char* task_name, char* start_address, char* end_address,
                       size_t page_size, WorkGang* pretouch_gang);
    static void touch(const char* task_name, char* start_address, char* end_address,
                       size_t page_size, WorkGang* pretouch_gang);
}


and implement these methods as needed.

As mentioned, due to naming issues I would prefer the version with the bool parameter.

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

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