RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Yumin Qi-3
Hi, Please review
  Usually most OSes are configured with page size of 4K, but some others are configured with 64K. If jdk binary is built on 4K platform and run on different configured platforms, CDS fails to be loaded due to region alignment mismatch:
  Unable to map CDS archive -- os::vm_allocation_granularity() expected: 4096 actual: 65536
  This change uses 64K as region alignment if OS page size is less than 64K. For most of the current OSes, means always use 64K as file map region alignment.
   The archive size will increase about 300K due to the change.
   Tests: tier1-4
              Run MacOS/X64 binary on MacOS/aarch64

   Thanks
   Yumin

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

Commit messages:
 - 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Changes: https://git.openjdk.java.net/jdk/pull/2651/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2651&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8236847
  Stats: 185 lines in 9 files changed: 128 ins; 14 del; 43 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2651.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2651/head:pull/2651

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Ioi Lam-2
On Fri, 19 Feb 2021 18:15:45 GMT, Yumin Qi <[hidden email]> wrote:

> Hi, Please review
>   Usually most OSes are configured with page size of 4K, but some others are configured with 64K. If jdk binary is built on 4K platform and run on different configured platforms, CDS fails to be loaded due to region alignment mismatch:
>   Unable to map CDS archive -- os::vm_allocation_granularity() expected: 4096 actual: 65536
>   This change uses 64K as region alignment if OS page size is less than 64K. For most of the current OSes, means always use 64K as file map region alignment.
>    The archive size will increase about 300K due to the change.
>    Tests: tier1-4
>               Run MacOS/X64 binary on MacOS/aarch64
>
>    Thanks
>    Yumin

Changes requested by iklam (Reviewer).

src/hotspot/share/memory/filemap.cpp line 212:

> 210:   }
> 211:   _version = CURRENT_CDS_ARCHIVE_VERSION;
> 212:   _region_alignment = region_alignment;

I think this should be renamed to `_core_region_alignment` to be consistent with `MetaspaceShared::core_region_alignment()`

src/hotspot/share/memory/metaspaceShared.cpp line 128:

> 126: void MetaspaceShared::init_alignments() {
> 127:   assert(_core_region_alignment == 0, "call this only once");
> 128:   _core_region_alignment = (size_t)os::vm_allocation_granularity();

The above assignment should be moved inside the `if (DumpSharedSpaces) { ...` block.

src/hotspot/share/memory/metaspaceShared.cpp line 142:

> 140:     assert(FileMapInfo::current_info() != NULL, "Call init_alignments() after base archive is opened");
> 141:     _core_region_alignment = FileMapInfo::current_info()->region_alignment();
> 142:     assert(_core_region_alignment == 64*K, "CDS always use 64K region alignment");

This should be `_core_region_alignment >= 64*K, "CDS must use minimum 64K region alignment"`. and moved below the `else` block.

This makes the code compatible with future OSes that may use >64KB for `os::vm_allocation_granularity()`.

src/hotspot/share/memory/metaspaceShared.cpp line 1351:

> 1349:
> 1350:   mapinfo->set_is_mapped(false);
> 1351:   assert(mapinfo->region_alignment() == 64*K, "Should always use 64K alignment");

Should be >= 64K

test/hotspot/jtreg/runtime/cds/appcds/SharedRegionAlignmentTest.java line 28:

> 26:  * @requires vm.cds
> 27:  * @requires vm.gc != "Z"
> 28:  * @summary Testing handling of CDS region alignment

I think the summary should be more specific, such as "Make sure CDS core region alignment is at least 64KB".

Also, I think we should add

* @comment ZGC may exit the VM if -XX:+UseLargePages is specified but
*          unavailable. Since this test is independent of the actual GC type, let's
*          disable it if ZGC is used.

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Thomas Stuefe
On Tue, 23 Feb 2021 02:51:01 GMT, Ioi Lam <[hidden email]> wrote:

>> Hi, Please review
>>   Usually most OSes are configured with page size of 4K, but some others are configured with 64K. If jdk binary is built on 4K platform and run on different configured platforms, CDS fails to be loaded due to region alignment mismatch:
>>   Unable to map CDS archive -- os::vm_allocation_granularity() expected: 4096 actual: 65536
>>   This change uses 64K as region alignment if OS page size is less than 64K. For most of the current OSes, means always use 64K as file map region alignment.
>>    The archive size will increase about 300K due to the change.
>>    Tests: tier1-4
>>               Run MacOS/X64 binary on MacOS/aarch64
>>
>>    Thanks
>>    Yumin
>
> Changes requested by iklam (Reviewer).

Should we only use this on platforms where one can have different base page sizes *on the same platform*? Since the only one of our platforms I know where this could even be an issue are aarch, and possibly AIX.

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Andrew Haley-2
In reply to this post by Yumin Qi-3
On Fri, 19 Feb 2021 18:15:45 GMT, Yumin Qi <[hidden email]> wrote:

> Hi, Please review
>   Usually most OSes are configured with page size of 4K, but some others are configured with 64K. If jdk binary is built on 4K platform and run on different configured platforms, CDS fails to be loaded due to region alignment mismatch:
>   Unable to map CDS archive -- os::vm_allocation_granularity() expected: 4096 actual: 65536
>   This change uses 64K as region alignment if OS page size is less than 64K. For most of the current OSes, means always use 64K as file map region alignment.
>    The archive size will increase about 300K due to the change.
>    Tests: tier1-4
>               Run MacOS/X64 binary on MacOS/aarch64
>
>    Thanks
>    Yumin

src/hotspot/share/memory/archiveBuilder.hpp line 230:

> 228: public:
> 229:   // Use this when you allocate space outside of ArchiveBuilder::dump_{rw,ro}_region. These are usually for misc tables
> 230:   // that are allocated in the RO space.

This line is too long.

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Andrew Haley-2
In reply to this post by Thomas Stuefe
On Tue, 23 Feb 2021 07:27:54 GMT, Thomas Stuefe <[hidden email]> wrote:

>> Changes requested by iklam (Reviewer).
>
> Should we only use this on platforms where one can have different base page sizes *on the same platform*? Since the only one of our platforms I know where this could even be an issue are aarch, and possibly AIX.

It doesn't make sense always to use 64k alignment, as most builds are for a single platform. By all means, it can be a useful option, but in general we want to minimize file sizes.

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Yumin Qi-3
On Tue, 23 Feb 2021 09:30:11 GMT, Andrew Haley <[hidden email]> wrote:

>> Should we only use this on platforms where one can have different base page sizes *on the same platform*? Since the only one of our platforms I know where this could even be an issue are aarch, and possibly AIX.
>
> It doesn't make sense always to use 64k alignment, as most builds are for a single platform. By all means, it can be a useful option, but in general we want to minimize file sizes.

> Should we only use this on platforms where one can have different base page sizes _on the same platform_? Since the only one of our platforms I know where this could even be an issue are aarch, and possibly AIX.

@tstuefe @theRealAph @iklam
Thanks for review! Currently linux-aarch64, macos-aarch64(I don't know AIX) can be configured with 4K/64K page size. To make one build compatible with both, this change takes the 64K (or bigger) as the default region alignment. It is hard to make it optional I think. Will check the possibility of more options.

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Thomas Stuefe
In reply to this post by Thomas Stuefe
On Tue, 23 Feb 2021 07:27:54 GMT, Thomas Stuefe <[hidden email]> wrote:

>> Changes requested by iklam (Reviewer).
>
> Should we only use this on platforms where one can have different base page sizes *on the same platform*? Since the only one of our platforms I know where this could even be an issue are aarch, and possibly AIX.

> @tstuefe @theRealAph @iklam
> Thanks for review! Currently linux-aarch64, macos-aarch64(I don't know AIX) can be configured with 4K/64K page size. To make one build compatible with both, this change takes the 64K (or bigger) as the default region alignment. It is hard to make it optional I think. Will check the possibility of more options.

Hi Yumin,

What I meant was to leave `_core_region_alignment` at allocation granularity for all platforms but aarch64. Since we do not move the archive across platforms, and all other platforms only have one base page size, so on those platforms you never run into build page size being different than runtime page size. Or am I overlooking something?

Wrt to AIX, I looked at it, I think you can disregard it for now.

Thanks, Thomas

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Ioi Lam-2
On Tue, 23 Feb 2021 16:02:13 GMT, Thomas Stuefe <[hidden email]> wrote:

> > @tstuefe @theRealAph @iklam
> > Thanks for review! Currently linux-aarch64, macos-aarch64(I don't know AIX) can be configured with 4K/64K page size. To make one build compatible with both, this change takes the 64K (or bigger) as the default region alignment. It is hard to make it optional I think. Will check the possibility of more options.
>
> Hi Yumin,
>
> What I meant was to leave `_core_region_alignment` at allocation granularity for all platforms but aarch64. Since we do not move the archive across platforms, and all other platforms only have one base page size, so on those platforms you never run into build page size being different than runtime page size. Or am I overlooking something?
>
> Wrt to AIX, I looked at it, I think you can disregard it for now.
>
> Thanks, Thomas

How about changing the code to

  if (DumpSharedSpaces) {
    _core_region_alignment = (size_t)os::vm_allocation_granularity();
#if (defined(LINUX) && defined(AARCH64)) || defined(__APPLE__)
    // If you create a CDS archive on one machine, and use it on another, and the two
    // machines have different page sizes, make sure the archive can be used
    // on both machines.
    //
    // (a) Linux/aarch64 can be configured to have either 4KB or 64KB page sizes.
    // (b) macOS/x64 uses 4KB, but macOS/aarch64 uses 64KB (note: you can run an x64 JDK
    //     on an M1-based MacBook using Rosetta).
    if (_core_region_alignment < 64*K) {
      log_info(cds)("Force core region alignment to 64K");
      _core_region_alignment = 64*K;
    }
#endif
  } else {

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Thomas Stuefe
On Tue, 23 Feb 2021 17:09:37 GMT, Ioi Lam <[hidden email]> wrote:

> How about changing the code to
>
> ```
>   if (DumpSharedSpaces) {
>     _core_region_alignment = (size_t)os::vm_allocation_granularity();
> #if (defined(LINUX) && defined(AARCH64)) || defined(__APPLE__)
>     // If you create a CDS archive on one machine, and use it on another, and the two
>     // machines have different page sizes, make sure the archive can be used
>     // on both machines.
>     //
>     // (a) Linux/aarch64 can be configured to have either 4KB or 64KB page sizes.
>     // (b) macOS/x64 uses 4KB, but macOS/aarch64 uses 64KB (note: you can run an x64 JDK
>     //     on an M1-based MacBook using Rosetta).
>     if (_core_region_alignment < 64*K) {
>       log_info(cds)("Force core region alignment to 64K");
>       _core_region_alignment = 64*K;
>     }
> #endif
>   } else {
> ```

Yes. Good comments too.

I guess on Windows this does not matter, since allocation granularity is 64k anyway?

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Yumin Qi-3
On Tue, 23 Feb 2021 17:30:20 GMT, Thomas Stuefe <[hidden email]> wrote:

> > How about changing the code to
> > ```
> >   if (DumpSharedSpaces) {
> >     _core_region_alignment = (size_t)os::vm_allocation_granularity();
> > #if (defined(LINUX) && defined(AARCH64)) || defined(__APPLE__)
> >     // If you create a CDS archive on one machine, and use it on another, and the two
> >     // machines have different page sizes, make sure the archive can be used
> >     // on both machines.
> >     //
> >     // (a) Linux/aarch64 can be configured to have either 4KB or 64KB page sizes.
> >     // (b) macOS/x64 uses 4KB, but macOS/aarch64 uses 64KB (note: you can run an x64 JDK
> >     //     on an M1-based MacBook using Rosetta).
> >     if (_core_region_alignment < 64*K) {
> >       log_info(cds)("Force core region alignment to 64K");
> >       _core_region_alignment = 64*K;
> >     }
> > #endif
> >   } else {
> > ```
>
> Yes. Good comments too.
>
> I guess on Windows this does not matter, since allocation granularity is 64k anyway?

Is?
`#if (defined(LINUX) || defined(__APPLE__)) &&  defined(AARCH64)
or
#if (defined(LINUX) && defined(AARCH64)) || defined(__APPLE__)`

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Ioi Lam-2
On Tue, 23 Feb 2021 17:32:44 GMT, Yumin Qi <[hidden email]> wrote:

> Is?
> `#if (defined(LINUX) || defined(__APPLE__)) && defined(AARCH64) or #if (defined(LINUX) && defined(AARCH64)) || defined(__APPLE__)`

Actually, I think we should use

#if (defined(LINUX) && defined(AARCH64)) || (defined(__APPLE__) && defined(AMD64))`

For the macOS case, we are running an AMD64 binary inside Rosetta.

We don't need to worry about `(defined(__APPLE__) && defined(AARCH64))` because we cannot run M1 binaries on AMD64.

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

Ioi Lam-2
In reply to this post by Yumin Qi-3
On Tue, 23 Feb 2021 17:32:44 GMT, Yumin Qi <[hidden email]> wrote:

> I guess on Windows this does not matter, since allocation granularity is 64k anyway?

I haven't heard of requirements to run Windows binaries across CPUs (in the style of Rosetta), so I guess we don't need to worry about it for now.

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages [v2]

Yumin Qi-3
In reply to this post by Yumin Qi-3
> Hi, Please review
>   Usually most OSes are configured with page size of 4K, but some others are configured with 64K. If jdk binary is built on 4K platform and run on different configured platforms, CDS fails to be loaded due to region alignment mismatch:
>   Unable to map CDS archive -- os::vm_allocation_granularity() expected: 4096 actual: 65536
>   This change uses 64K as region alignment if OS page size is less than 64K. For most of the current OSes, means always use 64K as file map region alignment.
>    The archive size will increase about 300K due to the change.
>    Tests: tier1-4
>               Run MacOS/X64 binary on MacOS/aarch64
>
>    Thanks
>    Yumin

Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:

  Make 64K core region alignment only for specific platforms, also fixed comments as suggestions.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2651/files
  - new: https://git.openjdk.java.net/jdk/pull/2651/files/b5f2e9f5..2c2cc0e0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2651&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2651&range=00-01

  Stats: 34 lines in 6 files changed: 10 ins; 4 del; 20 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2651.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2651/head:pull/2651

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages

David Holmes
In reply to this post by Ioi Lam-2
On 24/02/2021 3:50 am, Ioi Lam wrote:

> On Tue, 23 Feb 2021 17:32:44 GMT, Yumin Qi <[hidden email]> wrote:
>
>> Is?
>> `#if (defined(LINUX) || defined(__APPLE__)) && defined(AARCH64) or #if (defined(LINUX) && defined(AARCH64)) || defined(__APPLE__)`
>
> Actually, I think we should use
>
> #if (defined(LINUX) && defined(AARCH64)) || (defined(__APPLE__) && defined(AMD64))`
>
> For the macOS case, we are running an AMD64 binary inside Rosetta.
>
> We don't need to worry about `(defined(__APPLE__) && defined(AARCH64))` because we cannot run M1 binaries on AMD64.

I'm confused. Is this about running binaries on different architectures
or running binaries on machines with different page sizes? Or both? If
the M1 machines support 4K or 64K then they would need this change - no?

Regardless I'd rather see this hidden by some abstraction that captures
whether a given platform supports multiple sizes rather than these
explicit ifdefs.

Thanks,
David

> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/2651
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages [v2]

Ioi Lam-2
In reply to this post by Ioi Lam-2
On Tue, 23 Feb 2021 17:50:42 GMT, Ioi Lam <[hidden email]> wrote:

>>> > How about changing the code to
>>> > ```
>>> >   if (DumpSharedSpaces) {
>>> >     _core_region_alignment = (size_t)os::vm_allocation_granularity();
>>> > #if (defined(LINUX) && defined(AARCH64)) || defined(__APPLE__)
>>> >     // If you create a CDS archive on one machine, and use it on another, and the two
>>> >     // machines have different page sizes, make sure the archive can be used
>>> >     // on both machines.
>>> >     //
>>> >     // (a) Linux/aarch64 can be configured to have either 4KB or 64KB page sizes.
>>> >     // (b) macOS/x64 uses 4KB, but macOS/aarch64 uses 64KB (note: you can run an x64 JDK
>>> >     //     on an M1-based MacBook using Rosetta).
>>> >     if (_core_region_alignment < 64*K) {
>>> >       log_info(cds)("Force core region alignment to 64K");
>>> >       _core_region_alignment = 64*K;
>>> >     }
>>> > #endif
>>> >   } else {
>>> > ```
>>>
>>> Yes. Good comments too.
>>>
>>> I guess on Windows this does not matter, since allocation granularity is 64k anyway?
>>
>> Is?
>> `#if (defined(LINUX) || defined(__APPLE__)) &&  defined(AARCH64)
>> or
>> #if (defined(LINUX) && defined(AARCH64)) || defined(__APPLE__)`
>
>> I guess on Windows this does not matter, since allocation granularity is 64k anyway?
>
> I haven't heard of requirements to run Windows binaries across CPUs (in the style of Rosetta), so I guess we don't need to worry about it for now.

> _Mailing list message from [David Holmes](mailto:[hidden email]) on [hotspot-runtime-dev](mailto:[hidden email]):_
>
> On 24/02/2021 3:50 am, Ioi Lam wrote:
>
> > On Tue, 23 Feb 2021 17:32:44 GMT, Yumin Qi <minqi at openjdk.org> wrote:
> > > Is?
> > > `#if (defined(LINUX) || defined(__APPLE__)) && defined(AARCH64) or #if (defined(LINUX) && defined(AARCH64)) || defined(__APPLE__)`
> >
> >
> > Actually, I think we should use
> > #if (defined(LINUX) && defined(AARCH64)) || (defined(__APPLE__) && defined(AMD64))`
> > For the macOS case, we are running an AMD64 binary inside Rosetta.
> > We don't need to worry about `(defined(__APPLE__) && defined(AARCH64))` because we cannot run M1 binaries on AMD64.
>
> I'm confused. Is this about running binaries on different architectures
> or running binaries on machines with different page sizes? Or both?

The latter.

The macOS case is logically running on the same architecture (AMD64), except one of them is a *real* AMD64, and the second one is emulated AMD64 on M1 silicon. As far as the JVM knows, it runs on AMD64.

> If
> the M1 machines support 4K or 64K then they would need this change - no?

No, M1 macs always use 64KB pages. You cannot execute macOS/aarch64 binaries on macOS/AMD.

> Regardless I'd rather see this hidden by some abstraction that captures
> whether a given platform supports multiple sizes rather than these
> explicit ifdefs.

How about a function like:

    os::cds_core_region_alignment()

This function would return 64KB in the affected cases, and os::vm_allocation_granularity() otherwise.
 
> Thanks,
> David

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages [v2]

David Holmes
On 24/02/2021 10:47 am, Ioi Lam wrote:

> On Tue, 23 Feb 2021 17:50:42 GMT, Ioi Lam <[hidden email]> wrote:
>> _Mailing list message from [David Holmes](mailto:[hidden email]) on [hotspot-runtime-dev](mailto:[hidden email]):_
>>
>> On 24/02/2021 3:50 am, Ioi Lam wrote:
>>
>>> On Tue, 23 Feb 2021 17:32:44 GMT, Yumin Qi <minqi at openjdk.org> wrote:
>>>> Is?
>>>> `#if (defined(LINUX) || defined(__APPLE__)) && defined(AARCH64) or #if (defined(LINUX) && defined(AARCH64)) || defined(__APPLE__)`
>>>
>>>
>>> Actually, I think we should use
>>> #if (defined(LINUX) && defined(AARCH64)) || (defined(__APPLE__) && defined(AMD64))`
>>> For the macOS case, we are running an AMD64 binary inside Rosetta.
>>> We don't need to worry about `(defined(__APPLE__) && defined(AARCH64))` because we cannot run M1 binaries on AMD64.
>>
>> I'm confused. Is this about running binaries on different architectures
>> or running binaries on machines with different page sizes? Or both?
>
> The latter.
>
> The macOS case is logically running on the same architecture (AMD64), except one of them is a *real* AMD64, and the second one is emulated AMD64 on M1 silicon. As far as the JVM knows, it runs on AMD64.

Yuck! So we have to use 64K alignment on x86_64 in case it gets run on a M1.

>> If
>> the M1 machines support 4K or 64K then they would need this change - no?
>
> No, M1 macs always use 64KB pages. You cannot execute macOS/aarch64 binaries on macOS/AMD.

At least M1 always being 64KB simplifies things.

>> Regardless I'd rather see this hidden by some abstraction that captures
>> whether a given platform supports multiple sizes rather than these
>> explicit ifdefs.
>
> How about a function like:
>
>      os::cds_core_region_alignment()
>
> This function would return 64KB in the affected cases, and os::vm_allocation_granularity() otherwise.

That's fine for runtime. I was actually thinking about a build-time
abstraction like NEEDS_64K_MINIMUM_ALIGNMENT which would be defined by
those platforms that need this.

Thanks,
David

>> Thanks,
>> David
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/2651
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages [v2]

Nick Gasson
In reply to this post by Yumin Qi-3
On Tue, 23 Feb 2021 21:41:07 GMT, Yumin Qi <[hidden email]> wrote:

>> Hi, Please review
>>   Usually most OSes are configured with page size of 4K, but some others are configured with 64K. If jdk binary is built on 4K platform and run on different configured platforms, CDS fails to be loaded due to region alignment mismatch:
>>   Unable to map CDS archive -- os::vm_allocation_granularity() expected: 4096 actual: 65536
>>   This change uses 64K as region alignment if OS page size is less than 64K. For most of the current OSes, means always use 64K as file map region alignment.
>>    The archive size will increase about 300K due to the change.
>>    Tests: tier1-4
>>               Run MacOS/X64 binary on MacOS/aarch64
>>
>>    Thanks
>>    Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>
>   Make 64K core region alignment only for specific platforms, also fixed comments as suggestions.

src/hotspot/share/memory/metaspaceShared.cpp line 137:

> 135:     // (a) Linux/aarch64 can be configured to have either 4KB or 64KB page sizes.
> 136:     // (b) macOS/x64 uses 4KB, but macOS/aarch64 uses 64KB (note: you can run a x64 JDK
> 137:     //     on a M1-based MacBook using Rosetta).

I think the page size on Apple silicon Macs is 16KB not 64KB:

$ sysctl vm.pagesize
vm.pagesize: 16384

This is also the value reported by `sun.misc.Unsafe.pageSize()` with a native JDK. Although some Linux distributions use a 64KB page size so rounding up is probably the right thing to do.

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages [v2]

Yumin Qi-3
On Wed, 24 Feb 2021 02:19:05 GMT, Nick Gasson <[hidden email]> wrote:

>> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Make 64K core region alignment only for specific platforms, also fixed comments as suggestions.
>
> src/hotspot/share/memory/metaspaceShared.cpp line 137:
>
>> 135:     // (a) Linux/aarch64 can be configured to have either 4KB or 64KB page sizes.
>> 136:     // (b) macOS/x64 uses 4KB, but macOS/aarch64 uses 64KB (note: you can run a x64 JDK
>> 137:     //     on a M1-based MacBook using Rosetta).
>
> I think the page size on Apple silicon Macs is 16KB not 64KB:
>
> $ sysctl vm.pagesize
> vm.pagesize: 16384
>
> This is also the value reported by `sun.misc.Unsafe.pageSize()` with a native JDK. Although some Linux distributions use a 64KB page size so rounding up is probably the right thing to do.

@nick-arm You are right --- on M1 it is 16384. Some linux-aarch64 configured 64K page size.

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

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages [v2]

Thomas Stuefe
In reply to this post by Yumin Qi-3
On Tue, 23 Feb 2021 21:41:07 GMT, Yumin Qi <[hidden email]> wrote:

>> Hi, Please review
>>   Usually most OSes are configured with page size of 4K, but some others are configured with 64K. If jdk binary is built on 4K platform and run on different configured platforms, CDS fails to be loaded due to region alignment mismatch:
>>   Unable to map CDS archive -- os::vm_allocation_granularity() expected: 4096 actual: 65536
>>   This change uses 64K as region alignment if OS page size is less than 64K. For most of the current OSes, means always use 64K as file map region alignment.
>>    The archive size will increase about 300K due to the change.
>>    Tests: tier1-4
>>               Run MacOS/X64 binary on MacOS/aarch64
>>
>>    Thanks
>>    Yumin
>
> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>
>   Make 64K core region alignment only for specific platforms, also fixed comments as suggestions.

As a general remark, do you think it would make sense to move all cds related files into an own sub directory? This would make identifying easier especially for casual code readers.

- Not to do with your patch, but `FileMapInfo::set_current_info()` is unused, _current_info is modified directly. Either remove the function or use it?

----

Page sizes:

I may miss something here, but could we not simply use a constant always, independent on whether we are at dump- or runtime? I don't see why this is still dependent on DumpSharedSpaces. I see that at runtime we use the alignment set down in the archive. But how we could have a different alignment between runtime and dumptime now?

Then, we could get rid of `void MetaspaceShared::init_alignments()` and return the hard coded value directly from MetaspaceShared::core_region_alignment(). The advantage would be that in my IDE I get shown the value right away instead of having to hunt down the setter.

Then, you calculate the alignment based on os::vm_allocation_granularity(). I wonder whether it would be simpler and safer to just hard code the literal values here, combined with an assert maybe. Mainly because allocation granularity just exists for the sake of Windows, where it hides a system API, but in it was never clear whether this always will return the same value across all windows versions on the same platform. Since using hard coded values is simpler too, why not just do that? (same argument goes for os::vm_page_size() though I have never seen that value change for a platform).

Finally, if all we want is to prevent on MacOS use of small pages for archives, why do we have to condition it with the CPU at all? If on ARM page size is 16K, and on x86 we want it to be 16K to be able to load archives x86 generated archives on arm, why not just set it to 16K always?

So this would mean something like this:

  static size_t core_region_alignment() {
    const size_t ret =  
#if (defined(LINUX) && defined(AARCH64)) || (defined(WINDOWS))
        64 * K
#elif defined(__APPLE__)
        16 * K
#else
        4 * K
#endif
        ;
        assert(ret >= os::vm_allocation_granularity(), "sanity");
        return ret;
  }

----

and remove _core_region_alignment and init_alignments(), as well as use of FileMapInfo::core_region_alignment() for anything other than asserting that it is the same as MetaspaceShared::core_region_alignment().

--

Apart from that, I like this patch. The new naming for the alignment is a better description for what you want to align here.

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

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2651
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages [v2]

Andrew Haley-2
On Wed, 24 Feb 2021 06:02:19 GMT, Thomas Stuefe <[hidden email]> wrote:

>> Yumin Qi has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Make 64K core region alignment only for specific platforms, also fixed comments as suggestions.
>
> As a general remark, do you think it would make sense to move all cds related files into an own sub directory? This would make identifying easier especially for casual code readers.
>
> - Not to do with your patch, but `FileMapInfo::set_current_info()` is unused, _current_info is modified directly. Either remove the function or use it?
>
> ----
>
> Page sizes:
>
> I may miss something here, but could we not simply use a constant always, independent on whether we are at dump- or runtime? I don't see why this is still dependent on DumpSharedSpaces. I see that at runtime we use the alignment set down in the archive. But how we could have a different alignment between runtime and dumptime now?
>
> Then, we could get rid of `void MetaspaceShared::init_alignments()` and return the hard coded value directly from MetaspaceShared::core_region_alignment(). The advantage would be that in my IDE I get shown the value right away instead of having to hunt down the setter.
>
> Then, you calculate the alignment based on os::vm_allocation_granularity(). I wonder whether it would be simpler and safer to just hard code the literal values here, combined with an assert maybe. Mainly because allocation granularity just exists for the sake of Windows, where it hides a system API, but in it was never clear whether this always will return the same value across all windows versions on the same platform. Since using hard coded values is simpler too, why not just do that? (same argument goes for os::vm_page_size() though I have never seen that value change for a platform).
>
> Finally, if all we want is to prevent on MacOS use of small pages for archives, why do we have to condition it with the CPU at all? If on ARM page size is 16K, and on x86 we want it to be 16K to be able to load archives x86 generated archives on arm, why not just set it to 16K always?
>
> So this would mean something like this:
>
>   static size_t core_region_alignment() {
>     const size_t ret =  
> #if (defined(LINUX) && defined(AARCH64)) || (defined(WINDOWS))
>         64 * K
> #elif defined(__APPLE__)
>         16 * K
> #else
>         4 * K
> #endif
>         ;
>         assert(ret >= os::vm_allocation_granularity(), "sanity");
>         return ret;
>   }
>
> ----
>
> and remove _core_region_alignment and init_alignments(), as well as use of FileMapInfo::core_region_alignment() for anything other than asserting that it is the same as MetaspaceShared::core_region_alignment().
>
> --
>
> Apart from that, I like this patch. The new naming for the alignment is a better description for what you want to align here.

> Thanks for review! Currently linux-aarch64, macos-aarch64(I don't know AIX) can be configured with 4K/64K page size. To make one build compatible with both, this change takes the 64K (or bigger) as the default region alignment. It is hard to make it optional I think. Will check the possibility of more options.

It's not even slightly hard to make it optional. It should be a build-time configure argument. In many cases there is no need to make one build compatible with both 64k and 4k. If you'd like the default to be 64k I'm fine with that, but it should be possible to build with 4k alignment.

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

PR: https://git.openjdk.java.net/jdk/pull/2651
12