8340631: assert(reserved_rgn->contain_region(base_addr, size)) failed: Reserved CDS region should contain this mapping region #22743

matias9927 · 2024-12-13T19:51:47Z

What testing with UseCompressedClassPointers set to false, this assert is hit due to memory being released twice during the failure path of CDS archive mapping. This patch makes it so the RW and RO regions are only released once at the end in MetaspaceShared::release_reserved_spaces().

This patch hides unmap_region as the method should not be called on regions that were reserved. Instead, the region is skipped and we verify that it is indeed in the reserved space. Verified` with tier 1-4 tests.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8340631: assert(reserved_rgn->contain_region(base_addr, size)) failed: Reserved CDS region should contain this mapping region (Bug - P4)

Reviewers

Ioi Lam (@iklam - Reviewer)
Johan Sjölen (@jdksjolen - Reviewer) Review applies to 4402f98b
Stefan Karlsson (@stefank - Reviewer) Review applies to 4402f98b

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22743/head:pull/22743
$ git checkout pull/22743

Update a local copy of the PR:
$ git checkout pull/22743
$ git pull https://git.openjdk.org/jdk.git pull/22743/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22743

View PR using the GUI difftool:
$ git pr show -t 22743

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22743.diff

Using Webrev

Link to Webrev Comment

…: Reserved CDS region should contain this mapping region

bridgekeeper · 2024-12-13T19:52:42Z

👋 Welcome back matsaave! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-12-13T19:53:17Z

@matias9927 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8340631: assert(reserved_rgn->contain_region(base_addr, size)) failed: Reserved CDS region should contain this mapping region

Reviewed-by: iklam, jsjolen, stefank

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 25 new commits pushed to the master branch:

9f3c5f9: 8344907: NullPointerException in Win32ShellFolder2.getSystemIcon when "icon" is null
22f70a7: 8321413: IllegalArgumentException: Code length outside the allowed range while creating a jlink image
3bfa952: 8283795: Add TLSv1.3 and CNSA 1.0 algorithms to implementation requirements
1ef77cd: 8347171: (dc) java/nio/channels/DatagramChannel/InterruptibleOrNot.java fails with virtual thread factory
72f1114: 8346705: SNI not sent with Java 22+ using java.net.http.HttpClient.Builder#sslParameters
cb9a98b: 8347141: Several javac tests compile with an unnecessary -Xlint:-path flag
f9b1133: 8346880: [aix] java/lang/ProcessHandle/InfoTest.java still fails: "reported cputime less than expected"
9c72ded: 8346036: Unnecessary Hashtable usage in javax.swing.text.html.parser.Entity
3024a73: 8345782: Refining the cases that libjsig deprecation warning is issued
2801bc6: 8346460: NotifyFramePop should return JVMTI_ERROR_DUPLICATE
... and 15 more: https://git.openjdk.org/jdk/compare/ea49537726db6530f0ddcc04d9938df3d6d18250...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-12-13T19:53:53Z

@matias9927 The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-12-13T19:59:09Z

Webrevs

iklam

LGTM

jdksjolen

Looks good to me.

stefank

Nice solution for fixing the difference between Windows and the rest of the platforms!

I've added one nit below that you can ignore if you want to. I've also added a request/wish for an assert.

stefank · 2024-12-16T09:07:11Z

src/hotspot/share/cds/filemap.cpp

+    // If the region is inside an active ReservedSpace, its memory and address space will be
+    // freed when the ReservedSpace is released.
+    if (!rs.is_reserved()) {
+      unmap_region(idx);
+    } else {
+      // Treat this region as if it has been unmapped
+      region_at(idx)->set_mapped_base(nullptr);
+    }


Personal taste, but I tend to think that the code becomes easier to read if you remove the negation in if/else statements.

Suggested change

// If the region is inside an active ReservedSpace, its memory and address space will be

// freed when the ReservedSpace is released.

if (!rs.is_reserved()) {

unmap_region(idx);

} else {

// Treat this region as if it has been unmapped

region_at(idx)->set_mapped_base(nullptr);

}

// If the region is inside an active ReservedSpace, its memory and address space will be

// freed when the ReservedSpace is released.

if (rs.is_reserved()) {

// Treat this region as if it has been unmapped

region_at(idx)->set_mapped_base(nullptr);

} else {

unmap_region(idx);

}

It would also be good to have an assert to check that the unmapped region actually is within the provided ReservedSpace.

matias9927 · 2024-12-17T18:58:56Z

Thank you for the reviews and discussion @iklam @jdksjolen and @stefank!
/integrate

openjdk · 2024-12-17T19:00:29Z

@matias9927 This pull request has not yet been marked as ready for integration.

stefank · 2024-12-19T09:13:26Z

src/hotspot/share/cds/filemap.cpp

+      char* mapped_base = r->mapped_base();
+      size_t size = r->used_aligned();
+
+      assert(rs.is_reserved(), "must be");


This is already checked in the if-statement.

stefank · 2024-12-20T13:34:18Z

I would like to propose that we push the assert into unmap_region so that we don't have to duplicate the logic to decide if we should assert or unmap. Please take a look at this version of the fix and see what you think about it:
master...stefank:jdk:pull/22743

iklam · 2025-01-02T06:39:17Z

I would like to propose that we push the assert into unmap_region so that we don't have to duplicate the logic to decide if we should assert or unmap. Please take a look at this version of the fix and see what you think about it: master...stefank:jdk:pull/22743

I think this is better.

Stefan's change also reveals a new bug -- unmap_stand_alone_region(MetaspaceShared::hp) shouldn't be called by FileMapInfo::map_heap_region_impl(), as it will unreserve a block of memory inside the reserved heap region. This call should be removed.

This failure should be very very rare: The bitmap region is very small. If the process can't even map that, it's on the verge of dying due to OOM anyway. That's why we haven't caught this in the wild, but we should fix it nonetheless.

iklam · 2025-01-08T23:48:57Z

src/hotspot/share/cds/filemap.cpp

+        // This region was mapped inside a ReservedSpace. Its memory will be freed when the ReservedSpace
+        // is released. Zero it so that we don't accidentally read its content.
+        log_info(cds)("Region #%d (%s) is in a reserved space, it will be freed when the space is released", i, shared_region_name[i]);
+        r->set_mapped_base(nullptr);


There's no need for this line as it will be set to nullptr below line line 2450.

iklam · 2025-01-08T23:49:09Z

src/hotspot/share/cds/metaspaceShared.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2025, Oracle and/or its affiliates. All rights reserved.


This is the only change in this file.

src/hotspot/share/cds/metaspaceShared.cpp

iklam · 2025-01-09T16:00:54Z

src/hotspot/share/cds/filemap.cpp

@@ -1889,7 +1889,7 @@ MapArchiveResult FileMapInfo::map_region(int i, intx addr_delta, char* mapped_ba
  FileMapRegion* r = region_at(i);
  size_t size = r->used_aligned();
  char *requested_addr = mapped_base_address + r->mapping_offset();
-  assert(r->mapped_base() == nullptr, "must be not mapped yet");
+  assert(!is_mapped(), "must be not mapped yet");
  assert(requested_addr != nullptr, "must be specified");

  r->set_mapped_from_file(false);


Need to add r->set_in_reserved_space(false); here, as the r can be used twice (first map at requested location; if that fails, map at random location).

Does that belong here or should it be placed in init()?

FileMapRegion::init() is called once, during dump time. r->set_in_reserved_space(false) should be called there as well.

During run time, FileMapInfo::map_region() can be called twice, on the same index i, so the _in_reserved_space needs to be reset at the beginning of each call. That would be similar to what we do with _mapped_from_file.

iklam

LGTM!

matias9927 · 2025-01-09T22:06:00Z

Thanks for the reviews and discussion @iklam, @stefank. and @jdksjolen!
/integrate

openjdk · 2025-01-09T22:06:38Z

Going to push as commit 931914a.
Since your change was applied there have been 28 commits pushed to the master branch:

665c39c: 8347375: Extra
tag in robot specification
b335ea9: 8347122: Add missing @serial tags to module java.desktop
df28cec: 8345144: Robot does not specify all causes of IllegalThreadStateException
9f3c5f9: 8344907: NullPointerException in Win32ShellFolder2.getSystemIcon when "icon" is null
22f70a7: 8321413: IllegalArgumentException: Code length outside the allowed range while creating a jlink image
3bfa952: 8283795: Add TLSv1.3 and CNSA 1.0 algorithms to implementation requirements
1ef77cd: 8347171: (dc) java/nio/channels/DatagramChannel/InterruptibleOrNot.java fails with virtual thread factory
72f1114: 8346705: SNI not sent with Java 22+ using java.net.http.HttpClient.Builder#sslParameters
cb9a98b: 8347141: Several javac tests compile with an unnecessary -Xlint:-path flag
f9b1133: 8346880: [aix] java/lang/ProcessHandle/InfoTest.java still fails: "reported cputime less than expected"
... and 18 more: https://git.openjdk.org/jdk/compare/ea49537726db6530f0ddcc04d9938df3d6d18250...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-09T22:06:46Z

@matias9927 Pushed as commit 931914a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

matias9927 added 3 commits December 12, 2024 19:49

8340631: assert(reserved_rgn->contain_region(base_addr, size)) failed…

5a52976

…: Reserved CDS region should contain this mapping region

Reverted UseCompressedClassPointers

75bfb69

Fixed assert

72f71bd

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Dec 13, 2024

matias9927 marked this pull request as ready for review December 13, 2024 19:55

openjdk bot added the rfr Pull request is ready for review label Dec 13, 2024

iklam approved these changes Dec 16, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 16, 2024

jdksjolen approved these changes Dec 16, 2024

View reviewed changes

stefank approved these changes Dec 16, 2024

View reviewed changes

matias9927 added 2 commits December 16, 2024 19:22

Merge branch 'master' into cds_mapping_8340631

4402f98

Stefan comment

b589625

openjdk bot removed the ready Pull request is ready to be integrated label Dec 17, 2024

Added assert and made unmap_region private

3411970

stefank reviewed Dec 19, 2024

View reviewed changes

matias9927 added 2 commits December 19, 2024 15:52

Removed unneeded assert and caught edge case

fb8b310

Corrected edge case

f370362

matias9927 added 5 commits January 6, 2025 20:57

Stefan and Ioi comments

5866e70

New implementation

3cde55e

Merge branch 'master' into cds_mapping_8340631

05175aa

Fixed assert

bf1a4a2

Replaced memset

516373b

Merge branch 'master' into cds_mapping_8340631

be5e095

iklam suggested changes Jan 8, 2025

View reviewed changes

matias9927 added 2 commits January 9, 2025 00:23

Ioi suggestions

aa36910

Fixed copyright

65db5d0

iklam reviewed Jan 9, 2025

View reviewed changes

src/hotspot/share/cds/metaspaceShared.cpp Outdated Show resolved Hide resolved

Removed comment

5b9b32d

iklam reviewed Jan 9, 2025

View reviewed changes

Reset _in_reserved_space

0b2d3f9

iklam approved these changes Jan 9, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 9, 2025

openjdk bot added the integrated Pull request has been integrated label Jan 9, 2025

openjdk bot closed this Jan 9, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8340631: assert(reserved_rgn->contain_region(base_addr, size)) failed: Reserved CDS region should contain this mapping region #22743

8340631: assert(reserved_rgn->contain_region(base_addr, size)) failed: Reserved CDS region should contain this mapping region #22743

matias9927 commented Dec 13, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 13, 2024

openjdk bot commented Dec 13, 2024 •

edited

Loading

openjdk bot commented Dec 13, 2024

mlbridge bot commented Dec 13, 2024 •

edited

Loading

iklam left a comment

jdksjolen left a comment

stefank left a comment •

edited

Loading

stefank Dec 16, 2024

matias9927 commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

stefank Dec 19, 2024

stefank commented Dec 20, 2024

iklam commented Jan 2, 2025

iklam Jan 8, 2025

iklam Jan 8, 2025

iklam Jan 9, 2025

matias9927 Jan 9, 2025

iklam Jan 9, 2025

iklam left a comment

matias9927 commented Jan 9, 2025

openjdk bot commented Jan 9, 2025

openjdk bot commented Jan 9, 2025

8340631: assert(reserved_rgn->contain_region(base_addr, size)) failed: Reserved CDS region should contain this mapping region #22743

8340631: assert(reserved_rgn->contain_region(base_addr, size)) failed: Reserved CDS region should contain this mapping region #22743

Conversation

matias9927 commented Dec 13, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 13, 2024

openjdk bot commented Dec 13, 2024 • edited Loading

openjdk bot commented Dec 13, 2024

mlbridge bot commented Dec 13, 2024 • edited Loading

Webrevs

iklam left a comment

Choose a reason for hiding this comment

jdksjolen left a comment

Choose a reason for hiding this comment

stefank left a comment • edited Loading

Choose a reason for hiding this comment

stefank Dec 16, 2024

Choose a reason for hiding this comment

matias9927 commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

stefank Dec 19, 2024

Choose a reason for hiding this comment

stefank commented Dec 20, 2024

iklam commented Jan 2, 2025

iklam Jan 8, 2025

Choose a reason for hiding this comment

iklam Jan 8, 2025

Choose a reason for hiding this comment

iklam Jan 9, 2025

Choose a reason for hiding this comment

matias9927 Jan 9, 2025

Choose a reason for hiding this comment

iklam Jan 9, 2025

Choose a reason for hiding this comment

iklam left a comment

Choose a reason for hiding this comment

matias9927 commented Jan 9, 2025

openjdk bot commented Jan 9, 2025

openjdk bot commented Jan 9, 2025

matias9927 commented Dec 13, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 13, 2024 •

edited

Loading

mlbridge bot commented Dec 13, 2024 •

edited

Loading

stefank left a comment •

edited

Loading