RFR: 8328944: NMT reports "unknown" memory

Gerard Ziemski gziemski at openjdk.org
Sat Nov 16 02:57:54 UTC 2024 

On Mon, 4 Nov 2024 07:38:24 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> We use mtNone value in several functions default parameters, which may show up in NMT reports.
>> 
>> We address this, by removing the default value and forcing, where possible, for the callers to declare NMT tag that reflects the usage.
>> 
>> Eventually the goal would be not to use mtNone anywhere, but we are not there quite yet.
>> 
>> TODO: update copyrights.
>> 
>> Testing: undergoing MARCH5 tier1-5 ...
>
> src/hotspot/share/cds/metaspaceShared.cpp line 1387:
> 
>> 1385:       total_space_rs = ReservedSpace(total_range_size, base_address_alignment,
>> 1386:                                      os::vm_page_size(), (char*)base_address, mtClass);
>> 1387:     } else {
> 
> All the changes here in `MetaspaceShared::reserve_address_space_for_archives` are unnecessary; we tag the address region already. No need to do this multiple times, and in every case branch. The ReservedSpace objects here are not used for much and mostly ephemeral anyway - they are only used to reserve memory here, but after that have not much meaning, and apart from the class space rs don't even survive the invocation.

ReservedSpace() requires the client to pass in the `mem_tag` and I think it would be a mistake to allow a default `mtNone` tag here just to avoid setting it. It might be ephemeral, but technically we can and should set it if it's known.

> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 249:
> 
>> 247:             _bitmap_bytes_per_slice, bitmap_page_size);
>> 248: 
>> 249:   ReservedSpace bitmap(_bitmap_size, bitmap_page_size, mtGC);
> 
> remove nmt registration below?

Fixed.

> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 269:
> 
>> 267: 
>> 268:   if (ShenandoahVerify) {
>> 269:     ReservedSpace verify_bitmap(_bitmap_size, bitmap_page_size, mtGC);
> 
> remove nmt registration below?

Fixed.

> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 288:
> 
>> 286: 
>> 287:   ReservedSpace aux_bitmap(_bitmap_size, aux_bitmap_page_size, mtGC);
>> 288:   os::trace_page_sizes_for_requested_size("Aux Bitmap",
> 
> remove nmt registration below?

Fixed.

> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 306:
> 
>> 304: 
>> 305:   ReservedSpace region_storage(region_storage_size, region_page_size, mtGC);
>> 306:   os::trace_page_sizes_for_requested_size("Region Storage",
> 
> remove nmt registration below?

Fixed.

> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 340:
> 
>> 338: 
>> 339:     if (_collection_set == nullptr) {
>> 340:       cset_rs = ReservedSpace(cset_size, cset_align, os::vm_page_size(), nullptr, mtGC);
> 
> remove NMT registration in ShenandoahCollectionSet?

Fixed.

> src/hotspot/share/jfr/recorder/storage/jfrVirtualMemory.cpp line 107:
> 
>> 105:   _rs = ReservedSpace(reservation_size_request_bytes,
>> 106:                       os::vm_allocation_granularity(), os::vm_page_size(),
>> 107:                       nullptr, mtTracing);
> 
> remove NMT registraton?

Fixed.

> src/hotspot/share/memory/heap.cpp line 231:
> 
>> 229:   // reserve space for _segmap
>> 230:   ReservedSpace seg_rs(reserved_segments_size, mtCode);
>> 231:   if (!_segmap.initialize(seg_rs, committed_segments_size)) {
> 
> remove NMT registration

Fixed.

> src/hotspot/share/memory/metaspace.cpp line 605:
> 
>> 603:     assert(is_aligned(result, Metaspace::reserve_alignment()), "Alignment too small for metaspace");
>> 604:     rs = ReservedSpace::space_for_range(result, size, Metaspace::reserve_alignment(),
>> 605:                                                       os::vm_page_size(), false, false, mtMetaspace);
> 
> Please revert to mtNone; same reason as given in metaspaceShared.cpp. You can add a comment that tags are adjusted afterward, when the distribution of mtClass and mtClassShared is clear.

Can we use `mtAllocated` used here?

> src/hotspot/share/memory/metaspace.cpp line 746:
> 
>> 744:       }
>> 745:       rs = ReservedSpace(size, Metaspace::reserve_alignment(),
>> 746:                          os::vm_page_size() /* large */, (char*)base, mtMetaspace);
> 
> mtClass, but actually not needed, since we set mtClass below. Again, not necessary to do this for every branch individually we take here when reserving class space memory.

Fixed.

Since ReservedSpace requires `mem_tag` we need to pass it.

> src/hotspot/share/memory/metaspace.cpp line 775:
> 
>> 773: 
>> 774:     // Mark metaspace as such
>> 775:     MemTracker::record_virtual_memory_tag((address)rs.base(), mtMetaspace);
> 
> No, please revert. mtClass is correct here.

Removed `MemTracker::record_virtual_memory_tag()`

> src/hotspot/share/memory/metaspace/testHelpers.cpp line 82:
> 
>> 80:     // have reserve limit -> non-expandable context
>> 81:     _rs = ReservedSpace(reserve_limit * BytesPerWord, Metaspace::reserve_alignment(),
>> 82:                         os::vm_page_size(), nullptr, MemTag::mtMetaspace);
> 
> Give this mtTest

Fixed.

> src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp line 258:
> 
>> 256:   ReservedSpace rs(word_size * BytesPerWord,
>> 257:                    Settings::virtual_space_node_reserve_alignment_words() * BytesPerWord,
>> 258:                    os::vm_page_size(), nullptr, mtMetaspace);
> 
> remove tracking below.
> 
> This is actually wrong, and only works out of accident correctly, since this constructor just happens to be called for mtMetaspace; more correct would be to hand in the tag. This is on my plate, there is a JBS issue somewhere.

Fixed.

> src/hotspot/share/memory/virtualspace.hpp line 50:
> 
>> 48:   // ReservedSpace
>> 49:   ReservedSpace(char* base, size_t size, size_t alignment,
>> 50:                 size_t page_size, bool special, bool executable, MemTag mem_tag);
> 
> The growth in complexity is unfortunate.

I think the cost is worth the fact that we mark the memory with a tag as soon as possible, so that we don't catch NMT with its trousers down.

> src/hotspot/share/memory/virtualspace.hpp line 67:
> 
>> 65: 
>> 66:   void reserve(size_t size, size_t alignment, size_t page_size,
>> 67:                char* requested_address, bool executable, MemTag mem_tag);
> 
> Why do the instance methods get MemTag? Why is MemTag not stored as a (const) attribute of ReservedSpace? 
> 
> But then, we would have double accounting - we'd store the tag both in NMT and in ReservedSpace instances. If we change them post-reservation, we would need to change both. Sigh.
> 
> So we pass in MemTag just for the ReservedSpace to pass it into os::reserve_memory. Seeing how often ReservedSpace is used in situations where the tag is not clear at reservation time, I wonder whether we are not better off letting the user of ReservedSpace register the NMT tag post reservation like they do today.

I like the simplicity of not having to deal with changing the tag afterwards personally. Does anyone else have an opinion here?

> src/hotspot/share/nmt/memReporter.cpp line 83:
> 
>> 81:   if (mem_tag != mtNone) {
>> 82:     out->print("(%s" SIZE_FORMAT "%s type=%u", alloc_type,
>> 83:       amount_in_current_scale(amount), scale, (unsigned)mem_tag);
> 
> No, please revert.

Why? Seeing it as a string makes it more human readable.

Are we thinking that this output is parsed by tools?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844883480
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844883500
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844883510
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844883526
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844883544
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844883562
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844883605
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844883613
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844884091
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844884520
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844884665
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844884842
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844885543
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844888407
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844888178
PR Review Comment: https://git.openjdk.org/jdk/pull/21843#discussion_r1844888695

More information about the shenandoah-dev mailing list