RFR: 8312132: Add tracking of multiple address spaces in NMT [v2]

[Stefan Karlsson]

On Wed, 20 Mar 2024 15:17:52 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Hi,
>> 
>> This PR introduces a new abstraction to NMT, named `MemoryFileTracker`. Today, NMT does not track any memory outside of the virtual memory address space. This means that if you allocated memory in something such as a memory-backed file and use `mmap` to map into that memory, then you'll have trouble reporting this to NMT. This is the situation that ZGC is in, and that is what this patch attempts to fix.
>> 
>> ## `MemoryFileTracker`
>> 
>> The `MemoryFileTracker` adds the ability of adding new virtual memory address spaces to NMT and committing memory to these, the basic API is:
>> 
>> ```c++
>>     static MemoryFile* make_device(const char* descriptive_name);
>>     static void free_device(MemoryFile* device);
>> 
>>     static void allocate_memory(MemoryFile* device, size_t offset, size_t size,
>>                                 MEMFLAGS flag, const NativeCallStack& stack);
>>     static void free_memory(MemoryFile* device, size_t offset, size_t size);
>> 
>> 
>> It is easiest to see how this is used by looking at what ZGC's `ZNMT` class does:
>> 
>> ```c++
>> void ZNMT::reserve(zaddress_unsafe start, size_t size) {
>>   MemTracker::record_virtual_memory_reserve((address)start, size, CALLER_PC, mtJavaHeap);
>> }
>> void ZNMT::commit(zoffset offset, size_t size) {
>>   MemTracker::allocate_memory_in(ZNMT::_device, static_cast<size_t>(offset), size, mtJavaHeap, CALLER_PC);
>> }
>> void ZNMT::uncommit(zoffset offset, size_t size) {
>>   MemTracker::free_memory_in(ZNMT::_device, (size_t)offset, size);
>> }
>> 
>> void ZNMT::map(zaddress_unsafe addr, size_t size, zoffset offset) {
>>   // NMT doesn't track mappings at the moment.
>> }
>> void ZNMT::unmap(zaddress_unsafe addr, size_t size) {
>>   // NMT doesn't track mappings at the moment.
>> }
>> 
>> 
>> As you can see, any mapping between reserved regions and device-allocated memory is not recorded in NMT. This means that in detailed mode you only get reserved regions printed for the reserved memory, the device-allocated memory is reported separately. When performing summary reporting any memory allocated via these devices is added to the corresponding `MEMFLAGS` as `committed` memory. 
>> 
>> This patch is also acting as a base on which we deploy multiple new backend ideas to NMT. These ideas are:
>> 
>> 1. Implement VMA tracking using a balanced binary tree approach. Today's `VirtualMemoryTracker`'s usage of linked lists is slow and brittle, we'd like to move away from it. Our Treap-based approach in this patch gives a performance bo...
>
> Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Include the mutex header

Nice! I went over the ZGC files and requested a few style updates.

src/hotspot/share/gc/z/zInitialize.cpp line 36:

> 34: #include "gc/z/zMarkStackAllocator.hpp"
> 35: #include "gc/z/zNUMA.hpp"
> 36: #include "gc/z/zNMT.hpp"

Sort order

src/hotspot/share/gc/z/zNMT.cpp line 37:

> 35: 
> 36: void ZNMT::reserve(zaddress_unsafe start, size_t size) {
> 37:   MemTracker::record_virtual_memory_reserve((address)start, size, CALLER_PC, mtJavaHeap);

I'd prefer if we scale away the zaddress_unsafe type before casting to a pointer:
Suggestion:

  MemTracker::record_virtual_memory_reserve((address)untype(start), size, CALLER_PC, mtJavaHeap);

src/hotspot/share/gc/z/zNMT.cpp line 40:

> 38: }
> 39: void ZNMT::commit(zoffset offset, size_t size) {
> 40:   MemTracker::allocate_memory_in(ZNMT::_device, static_cast<size_t>(offset), size, mtJavaHeap, CALLER_PC);

Suggestion:

  MemTracker::allocate_memory_in(ZNMT::_device, untype(offset), size, mtJavaHeap, CALLER_PC);

src/hotspot/share/gc/z/zNMT.cpp line 42:

> 40:   MemTracker::allocate_memory_in(ZNMT::_device, static_cast<size_t>(offset), size, mtJavaHeap, CALLER_PC);
> 41: }
> 42: void ZNMT::uncommit(zoffset offset, size_t size) {

Please, add a blankline between the functions.

src/hotspot/share/gc/z/zNMT.cpp line 43:

> 41: }
> 42: void ZNMT::uncommit(zoffset offset, size_t size) {
> 43:   MemTracker::free_memory_in(ZNMT::_device, (size_t)offset, size);

Suggestion:

  MemTracker::free_memory_in(ZNMT::_device, untype(offset), size);

src/hotspot/share/gc/z/zNMT.cpp line 49:

> 47:   // NMT doesn't track mappings at the moment.
> 48: }
> 49: void ZNMT::unmap(zaddress_unsafe addr, size_t size) {

blankline

src/hotspot/share/gc/z/zNMT.hpp line 34:

> 32: #include "utilities/globalDefinitions.hpp"
> 33: #include "utilities/nativeCallStack.hpp"
> 34: #include "nmt/memTracker.hpp"

Sort order

src/hotspot/share/gc/z/zNMT.hpp line 39:

> 37: class ZNMT : public AllStatic {
> 38: private:
> 39:   static MemoryFileTracker::MemoryFile* _device;

Blankline after this line

src/hotspot/share/gc/z/zNMT.hpp line 43:

> 41:   static void init();
> 42:   static void map(zaddress_unsafe addr, size_t size, zoffset offset);
> 43:   static void unmap(zaddress_unsafe addr, size_t size);

ZGC tries to keep the definitions and declarations in the same order in the .hpp and .cpp files. Please, move this accordingly.

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18289#pullrequestreview-1949288929
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1532326193
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1532329588
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1532330918
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1532332161
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1532332658
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1532333077
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1532334535
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1532334938
PR Review Comment: https://git.openjdk.org/jdk/pull/18289#discussion_r1532336445