8334371: [AIX] Beginning with AIX 7.3 TL1 mmap() supports 64K memory pages #19771

JoKern65 · 2024-06-18T13:48:00Z

Beginning with AIX 7.3 TL1 mmap() supports 64K memory pages. As an enhancement, during the initialization of the VM the availability of this new feature is examined. If the 64K pages are supported the VM will use mmap() with 64K pages instead of shmget()/shmat() with 64K pages due to the bad 256M alignment of shmget()/shmat().

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-8334371: [AIX] Beginning with AIX 7.3 TL1 mmap() supports 64K memory pages (Enhancement - P4)

Reviewers

Thomas Stuefe (@tstuefe - Reviewer)
Martin Doerr (@TheRealMDoerr - Reviewer) 🔄 Re-review required (review applies to cd15f133)
Matthias Baesken (@MBaesken - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19771

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-18T13:48:54Z

👋 Welcome back jkern! 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-06-18T13:50:19Z

@JoKern65 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:

8334371: [AIX] Beginning with AIX 7.3 TL1 mmap() supports 64K memory pages

Reviewed-by: stuefe, mbaesken, mdoerr

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 204 new commits pushed to the master branch:

916db07: 8335532: [JVMCI] Export VM_Version::L1_line_size in JVMCI
c0604fb: 8334890: Missing unconditional cross modifying fence in nmethod entry barriers
cf1be87: 8335663: Fix simple -Wzero-as-null-pointer-constant warnings in C2 code
0bb9c76: 8324089: Fix typo in the manual page for "jcmd" (man jcmd)
3e3f83f: 8335385: javac crash on unattributed piece of AST
b20e8c8: 8335397: Improve reliability of TestRecursiveMonitorChurn.java
38a578d: 8334738: os::print_hex_dump should optionally print ASCII
7b894bc: 8332786: When dumping static CDS archives, explicitly assert that we don't use a CDS archive
e01626c: 8335655: ProblemList serviceability/dcmd/vm tests failing after JDK-8322475
3efa93b: 8335588: Fix -Wzero-as-null-pointer-constant warnings in calls to Node ctor
... and 194 more: https://git.openjdk.org/jdk/compare/e681b4e9b3ae24f45d8c6adab4105df39e6b8a92...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-06-18T13:50:43Z

@JoKern65 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-06-18T13:53:35Z

Webrevs

tstuefe

At long last. Cool.

Once 7.3 is the minimum requirement, you can shake lose a lot of code here, all that old SystemV shm coding.

Small remarks, otherwise okay

tstuefe · 2024-06-18T14:11:21Z

src/hotspot/os/aix/os_aix.cpp


+    // Can we use mmap with 64K pages? (Should be available with AIX7.3 TL1)
+    {
+      void* p = mmap(NULL, 1000000, PROT_READ | PROT_WRITE, MAP_ANON_64K | MAP_ANONYMOUS | MAP_SHARED, -1, 0);


Weird size. Why 1mio decimal? Why not 64K?

OK, I will use 64*K

tstuefe · 2024-06-18T14:14:17Z

src/hotspot/os/aix/os_aix.cpp

+    // Can we use mmap with 64K pages? (Should be available with AIX7.3 TL1)
+    {
+      void* p = mmap(NULL, 1000000, PROT_READ | PROT_WRITE, MAP_ANON_64K | MAP_ANONYMOUS | MAP_SHARED, -1, 0);
+      guarantee0(p != (void*) -1); // Should always work.


I probably introduced those guarantee's myself many years ago, but we actually used guarantee very rarely, only for things that absolutely have to work, or the world explodes.

Here, there are a number of reasons why an mmap could fail, and you probably don't want to crash the VM with an guarantee at a customer site. I'd use assert.

And what about the other guarantees in the same function after shmget() or shmat()?

Yes, these are my faults. When I did them originally, I did not know better ( ~20 years ago). In new code, lets prefer assert. Up to you if you want to take the opportunity and clean up old code.

tstuefe · 2024-06-18T14:17:55Z

test/hotspot/gtest/runtime/test_os.cpp

+# ifndef MAP_ANON_64K
+# define MAP_ANON_64K  0x400
+# endif
+#endif


Could you move the AIX-specific files out to something like test_os_aix.cpp, please? (same directory, just use ifdef AIX in the whole file). Its nicer, and precedence is there with the Cgroup tests of linux.

I hope I did it like you meant. Please verify.

tstuefe

Mostly good

tstuefe · 2024-06-19T11:13:56Z

test/hotspot/gtest/runtime/test_os_aix.cpp

+#include "memory/allocation.hpp"
+#include "memory/resourceArea.hpp"
+#include "nmt/memTracker.hpp"
+#include "runtime/frame.inline.hpp"
+#include "runtime/globals.hpp"
+#include "runtime/os.inline.hpp"
+#include "runtime/thread.hpp"
+#include "runtime/threads.hpp"
+#include "utilities/align.hpp"
+#include "utilities/globalDefinitions.hpp"
+#include "utilities/macros.hpp"
+#include "utilities/ostream.hpp"
+#include "unittest.hpp"


Suggested change

#include "memory/allocation.hpp"

#include "memory/resourceArea.hpp"

#include "nmt/memTracker.hpp"

#include "runtime/frame.inline.hpp"

#include "runtime/globals.hpp"

#include "runtime/os.inline.hpp"

#include "runtime/thread.hpp"

#include "runtime/threads.hpp"

#include "utilities/align.hpp"

#include "utilities/globalDefinitions.hpp"

#include "utilities/macros.hpp"

#include "utilities/ostream.hpp"

#include "unittest.hpp"

#include "runtime/os.inline.hpp"

#include "utilities/debug.hpp"

#include "utilities/globalDefinitions.hpp"

#include "unittest.hpp"

I would guess that is all includes you really need here

Yes you're right, I had tested this out beforehand, but was reluctant and not sure if I change something not obvious with this. My test was without utilities/debug.hpp

tstuefe · 2024-06-19T11:15:02Z

src/hotspot/os/aix/os_aix.cpp

+            ::shmdt(p);
+          }
        }
-        ::shmdt(p);


Are these just unrelated cleanups? All of this affects shmget etc, I am not sure what this has to do with large-page mmap.

Forget my question, I now understand the (small) difference

tstuefe · 2024-06-19T11:15:29Z

src/hotspot/os/aix/os_aix.cpp

+        g_multipage_support.can_use_64K_mmap_pages = (64*K == os::Aix::query_pagesize(p));
+        munmap(p, 64*K);
+      }
+    }


Why MAP_SHARED?

Sorry Thomas, I oversaw your question.
I followed the the code of reserve_mmaped_memory(), where MAP_SHARED is used with the following comment:

// Note: MAP_SHARED (instead of MAP_PRIVATE) needed to be able to // later use msync(MS_INVALIDATE) (see os::uncommit_memory). int flags = MAP_ANONYMOUS | MAP_SHARED;

I would like to have set the same flags for this test as used later on in real life.

tstuefe · 2024-06-19T11:17:25Z

src/hotspot/os/aix/os_aix.cpp

-  // 4k           yes                                 64k (treat 4k stacks as 64k)    different loader than java and standard settings
+  // LDR_CNTRL    can_use_64K_pages_dynamically(mmap or shm)       what we do                      remarks
+  // 4K           no                                               4K                              old systems (aix 5.2) or new systems with AME activated
+  // 4k           yes                                              64k (treat 4k stacks as 64k)    different loader than java and standardsettings


standard settings (blank)

In case this was not clear: I meant the typo. So, "s/standardsettings/standard settings" :)

tstuefe · 2024-06-19T15:46:00Z

src/hotspot/os/aix/os_aix.cpp

+// sys/mman.h defines MAP_ANON_64K beginning with AIX7.3 TL1
+#ifndef MAP_ANON_64K
+#define MAP_ANON_64K  0x400
+#endif


Suggested change

// sys/mman.h defines MAP_ANON_64K beginning with AIX7.3 TL1

#ifndef MAP_ANON_64K

#define MAP_ANON_64K 0x400

#endif

// sys/mman.h defines MAP_ANON_64K beginning with AIX7.3 TL1

#ifndef MAP_ANON_64K

#define MAP_ANON_64K 0x400

#else

STATIC_ASSERT(MAP_ANON_64K == 0x400);

#endif

That adds a test, for when you do have the MAP_ANON_64K constant, that your assumption about its value (0x400) is correct. Needs utilities/debug.hpp, but you should include that anyway.

tstuefe · 2024-06-19T15:46:59Z

src/hotspot/os/aix/os_aix.cpp

  size_t textpsize;           // default text page size (LDR_CNTRL STACKPSIZE)
  bool can_use_64K_pages;     // True if we can alloc 64K pages dynamically with Sys V shm.
  bool can_use_16M_pages;     // True if we can alloc 16M pages dynamically with Sys V shm.
+  bool can_use_64K_mmap_pages;// True if we can alloc 64K pages dynamically with mmap.


nit: blank after semicolon

tstuefe · 2024-06-19T15:49:11Z

src/hotspot/os/aix/os_aix.cpp

-        if (real_pagesize != pagesize) {
-          log_warning(pagesize)("real page size (" SIZE_FORMAT_X ") differs.", real_pagesize);
+      assert(shmid != -1, "shmget failed");
+      if (shmid != -1) {


Ah, I see what you do here, you now want to handle shmid == -1 for the release build, because I wanted you to change guarantee->assert? Okay. That is very thorough.

tstuefe · 2024-06-19T15:50:20Z

src/hotspot/os/aix/os_aix.cpp

+            ::shmdt(p);
+          }
        }
-        ::shmdt(p);


Forget my question, I now understand the (small) difference

tstuefe

Okay

TheRealMDoerr

LGTM. Only a few minor nits. Are you planning to change

jdk/src/hotspot/share/runtime/os.cpp

Line 1924 in 747e1e4

AIX_ONLY(os::vm_page_size() == 4*K ? 4*K : 256*M)

later?

TheRealMDoerr · 2024-07-01T10:15:55Z

src/hotspot/os/aix/os_aix.cpp

 #include "services/attachListener.hpp"
 #include "services/runtimeService.hpp"
 #include "signals_posix.hpp"
+#include "utilities/debug.hpp"


Should be sorted alphabetically.

TheRealMDoerr · 2024-07-01T10:16:18Z

src/hotspot/os/aix/os_aix.cpp

 #include <sys/mman.h>
+// sys/mman.h defines MAP_ANON_64K beginning with AIX7.3 TL1
+#ifndef MAP_ANON_64K
+  #define MAP_ANON_64K  0x400


Extra whitespace.

TheRealMDoerr · 2024-07-01T10:29:01Z

test/hotspot/gtest/runtime/test_os_aix.cpp

+#include <sys/mman.h>
+// sys/mman.h defines MAP_ANON_64K beginning with AIX7.3 TL1
+#ifndef MAP_ANON_64K
+  #define MAP_ANON_64K  0x400


Extra whitespace.

TheRealMDoerr · 2024-07-01T14:02:44Z

src/hotspot/share/runtime/os.cpp

  // The logic here is dual to the one in pd_reserve_memory in os_aix.cpp
  const size_t system_allocation_granularity =
-    AIX_ONLY(os::vm_page_size() == 4*K ? 4*K : 256*M)
+    AIX_ONLY(os::vm_page_size() == 4*K ? 4*K : os::Aix::supports_64K_mmap_pages() ? 64*K : 256*M)


Please add braces or rewrite it. Does
AIX_ONLY( (!os::Aix::supports_64K_mmap_pages() && os::vm_page_size() == 64*K) ? 256*M : ) os::vm_allocation_granularity() work?

TheRealMDoerr

Thanks! LGTM. Maybe @tstuefe can take a look at the latest changes, too.

tstuefe

Very good.

JoKern65 · 2024-07-04T11:19:13Z

/integrate

openjdk · 2024-07-04T11:20:57Z

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

916db07: 8335532: [JVMCI] Export VM_Version::L1_line_size in JVMCI
c0604fb: 8334890: Missing unconditional cross modifying fence in nmethod entry barriers
cf1be87: 8335663: Fix simple -Wzero-as-null-pointer-constant warnings in C2 code
0bb9c76: 8324089: Fix typo in the manual page for "jcmd" (man jcmd)
3e3f83f: 8335385: javac crash on unattributed piece of AST
b20e8c8: 8335397: Improve reliability of TestRecursiveMonitorChurn.java
38a578d: 8334738: os::print_hex_dump should optionally print ASCII
7b894bc: 8332786: When dumping static CDS archives, explicitly assert that we don't use a CDS archive
e01626c: 8335655: ProblemList serviceability/dcmd/vm tests failing after JDK-8322475
3efa93b: 8335588: Fix -Wzero-as-null-pointer-constant warnings in calls to Node ctor
... and 194 more: https://git.openjdk.org/jdk/compare/e681b4e9b3ae24f45d8c6adab4105df39e6b8a92...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-07-04T11:21:02Z

@JoKern65 Pushed as commit ced9906.

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

JDK-8334371

379d51a

openjdk bot changed the title ~~JDK-8334371: [AIX] Beginning with AIX 7.3 TL1 mmap() supports 64K memory pages~~ 8334371: [AIX] Beginning with AIX 7.3 TL1 mmap() supports 64K memory pages Jun 18, 2024

openjdk bot added the rfr Pull request is ready for review label Jun 18, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jun 18, 2024

tstuefe reviewed Jun 18, 2024

View reviewed changes

following Thomas proposals

7ec50dc

tstuefe reviewed Jun 19, 2024

View reviewed changes

delete not needed header files

19246c4

tstuefe reviewed Jun 19, 2024

View reviewed changes

minor changes

027843e

tstuefe approved these changes Jun 29, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 29, 2024

TheRealMDoerr reviewed Jul 1, 2024

View reviewed changes

following Martins proposals

ec42f92

TheRealMDoerr reviewed Jul 1, 2024

View reviewed changes

cosmetic changes

cd15f13

TheRealMDoerr approved these changes Jul 1, 2024

View reviewed changes

also call to protect_memory now possible

4a6ecf0

openjdk bot removed the ready Pull request is ready to be integrated label Jul 3, 2024

tstuefe approved these changes Jul 4, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 4, 2024

MBaesken approved these changes Jul 4, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 4, 2024

openjdk bot closed this Jul 4, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 4, 2024

8334371: [AIX] Beginning with AIX 7.3 TL1 mmap() supports 64K memory pages #19771

8334371: [AIX] Beginning with AIX 7.3 TL1 mmap() supports 64K memory pages #19771

Uh oh!

Conversation

JoKern65 commented Jun 18, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jun 18, 2024

Uh oh!

openjdk bot commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 18, 2024

Uh oh!

mlbridge bot commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstuefe Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstuefe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoKern65 Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstuefe Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

JoKern65 commented Jun 18, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 18, 2024 •

edited

Loading

mlbridge bot commented Jun 18, 2024 •

edited

Loading

tstuefe Jun 18, 2024 •

edited

Loading

tstuefe left a comment •

edited

Loading

JoKern65 Jun 19, 2024 •

edited

Loading

tstuefe Jun 19, 2024 •

edited

Loading