Skip to content

8356228: NMT does not record reserved memory base address correctly #25719

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+5 −1
Open

8356228: NMT does not record reserved memory base address correctly #25719

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/hotspot/share/cds/archiveBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
#include "memory/memoryReserver.hpp"
#include "memory/memRegion.hpp"
#include "memory/resourceArea.hpp"
#include "nmt/memTracker.hpp"
#include "oops/compressedKlass.inline.hpp"
#include "oops/instanceKlass.hpp"
#include "oops/methodCounters.hpp"
Expand Down Expand Up @@ -339,7 +340,10 @@ address ArchiveBuilder::reserve_buffer() {
aot_log_error(aot)("Failed to reserve %zu bytes of output buffer.", buffer_size);
MetaspaceShared::unrecoverable_writing_error();
}

// Once the memory is reserved, NMT can track it as mtClassShared.
// During an aligned reserve, some extra sub-regions of the memory region may be released due to alignment requirements.
// NMT will ignore releasing sub-regions (i.e., contained in other regions) that have mtClassShared tags.
MemTracker::record_virtual_memory_tag(rs, mtClassShared);
Comment on lines +343 to +346
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to capture this logic in a test, so we can always check for this behavior?

Where in the code do we release and check for the special case of mtClassShared?

I'm not 100% sure I understand why we have to first reserve with mtNone and only then change it to mtClassShared. Why can't we reserve with mtClassShared right from the start?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to capture this logic in a test, so we can always check for this behavior?

Work on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where in the code do we release and check for the special case of mtClassShared?

In virtualMemoryTracker.cpp, remove_released_region(adddress, size) in an if block as this:

if (reserved_rgn->mem_tag() == mtClassShared) {
    if (reserved_rgn->contain_region(addr, size)) {
      // This is an unmapped CDS region, which is part of the reserved shared
      // memory region.
      // See special handling in VirtualMemoryTracker::add_reserved_region also.
      return true;
    }

    if (size > reserved_rgn->size()) {
      // This is from release the whole region spanning from archive space to class space,
      // so we release them altogether.
      ReservedMemoryRegion class_rgn(addr + reserved_rgn->size(),
                                     (size - reserved_rgn->size()));
      ReservedMemoryRegion* cls_rgn = _reserved_regions->find(class_rgn);
      assert(cls_rgn != nullptr, "Class space region  not recorded?");
      assert(cls_rgn->mem_tag() == mtClass, "Must be class mem tag");
      remove_released_region(reserved_rgn);
      remove_released_region(cls_rgn);
      return true;
    }
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure I understand why we have to first reserve with mtNone and only then change it to mtClassShared. Why can't we reserve with mtClassShared right from the start?

The sequence is as follows:
All the reserve/release operations here are from os:: namespace and therefore will call NMT tracking API
1- CDS requests for reserving S bytes at address Base with alignment A and memTag M.
2- Due to OS alignment limitations regarding A, new base XBase to be used with new size XS. Read X as extra here.
3- Do instead, reserve XS bytes at XBase.
4- If succeeds, memory would be like <--s1---><----S----><----s2--->, where XS = s1 + S + s2 and XBase + s1 == Base
5- Release extra parts s1 and s2.
6- request at step 1 above is fulfilled now.

If at step 1, the memTag M is mtClassShared , the releases at step 5 will be ignored by NMT. Which means that there are regions in NMT lists that are actually released (at os level) but NMT takes them still as reserved. Therefore, further reservations by os at those regions (e.g., for stack) will intersect with existing NMT regions and the NMT assertions fail. So the memTag is to be mtNone to bypass/avoid the NMT ignore branch.

If we want to track the new memory region as belongs to CDS, we need to set the memTag of the region to mtClassShared with the corresponding MemTracker API.(as is done here in this PR)

// buffer_bottom is the lowest address of the 2 core regions (rw, ro) when
// we are copying the class metadata into the buffer.
address buffer_bottom = (address)rs.base();
Expand Down