Skip to content
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.

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

8312180: (bf) MappedMemoryUtils passes incorrect arguments to msync (aix) #14904

Closed
wants to merge 5 commits into from

Conversation

backwaterred
Copy link
Contributor

@backwaterred backwaterred commented Jul 17, 2023

Calls to msync on AIX require the length to be a multiple of a specific pagesize which may not be the same as the one returned by Bits.pageSize(), as explained in the JBS issue description.

EINVAL The addr argument is not a multiple of the page size as returned by the sysconf subroutine using the _SC_PAGE_SIZE value for the Name parameter, ...
[emphasis added by me]

By adding this as platform dependant code, the correct value of page size is used on AIX. Other Unix platforms should see no change by calling sysconf instead of Bits.pagesize. Windows is unchanged.


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-8312180: (bf) MappedMemoryUtils passes incorrect arguments to msync (aix) (Bug - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14904

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 17, 2023

👋 Welcome back tsteele! 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
Copy link

openjdk bot commented Jul 17, 2023

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

  • nio

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.

@openjdk openjdk bot added the nio nio-dev@openjdk.org label Jul 17, 2023
@backwaterred backwaterred changed the title 8312180: [AIX] MappedMemoryUtils passes incorrect address size and length to msync 8312180: (bf) MappedMemoryUtils passes incorrect address size and length to msync (aix) Jul 17, 2023
@backwaterred backwaterred changed the title 8312180: (bf) MappedMemoryUtils passes incorrect address size and length to msync (aix) 8312180: (bf) MappedMemoryUtils passes incorrect arguments to msync (aix) Jul 17, 2023
@backwaterred backwaterred marked this pull request as ready for review July 17, 2023 22:52
@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 17, 2023
@mlbridge
Copy link

mlbridge bot commented Jul 17, 2023

Webrevs

@backwaterred
Copy link
Contributor Author

backwaterred commented Jul 17, 2023

The pre-test failures appearing on some architectures may be using old header files and need to be re-run. I encountered a similar issue internally, and fixed it with make clean. In any case, I don't think the signature can be wrong on linux-x[86|64], but right on linux-x64-hs* since they use the same source.

// is aware of. If no implementation is provided, return Bits.pageSize instead.
private static int pageSize() {
int ps = pageSize0();
if (ps > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

If sysconf(_SC_PAGESIZE) returns a value different from Bits::pageSize on other Unix platforms, this could cause an unexpected change in behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. However, my thinking was that on systems with only one pagesize the size returned by sysconf is still the correct size to return. There is no need to pop back to the VM and trigger another call (to Bits.pageSize).

Copy link
Member

Choose a reason for hiding this comment

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

The risk is still non-zero. Calling Bits::pageSize does not pop back to the VM, it just returns the already injected constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The risk is still non-zero.

Fair point. I'll modify it to keep the same value a[s] before, and only change AIX.

@AlanBateman
Copy link
Contributor

This change impacts all platforms and I think needs more information. How many page sizes are you dealing with on AIX, what does Unsafe::pageSize return vs. sysconf(_SC_PAGESIZE).

@backwaterred
Copy link
Contributor Author

@bplb and @AlanBateman there's a bit more info in the JBS issue. I encourage you to take another look there too.

How many page sizes are you dealing with on AIX[?]

Just two!

what does Unsafe::pageSize return vs. sysconf(_SC_PAGESIZE)[?]

It boils down to Dynamic Variable Page Sizes, which is an AIX feature. The larger page size returned by Unsafe::pageSize is the system page size (by default 64 KiB). The page size required by msync (and returned by sysconf(_SC_PAGESIZE) is the fine-grained page size (by default 4 KiB).

@tstuefe
Copy link
Member

tstuefe commented Jul 18, 2023

what does Unsafe::pageSize return vs. sysconf(_SC_PAGESIZE)[?]

It boils down to Dynamic Variable Page Sizes, which is an AIX feature. The larger page size returned by Unsafe::pageSize is the system page size (by default 64 KiB). The page size required by msync (and returned by sysconf(_SC_PAGESIZE) is the fine-grained page size (by default 4 KiB).

A bit of background: At the start of the JVM port to AIX (before OpenJDK) we wanted to use 64 KB pages, but the "LargePage" semantics used by the JVM were too rigid and too badly abstracted to be usable.

So we instead "fake" a system page size of 64 KB instead of 4 KB by, basically, letting os::vm_page_size() return 64 KB. This works surprisingly well in most cases due to some peculiarities of AIX memory management. The real 4 KB system page size is only observable in a select few corner cases, one of which is this msync coding.

@AlanBateman
Copy link
Contributor

It boils down to Dynamic Variable Page Sizes, which is an AIX feature. The larger page size returned by Unsafe::pageSize is the system page size (by default 64 KiB). The page size required by msync (and returned by sysconf(_SC_PAGESIZE) is the fine-grained page size (by default 4 KiB).

sysconf is usually documented to return values that don't change during the lifetime of the process. Does this AIX feature mean that sysconf(_SC_PAGESIZE) may initially return 4k but 64k some time later? If so, what does this mean for MappedByteBuffer.load/isLoaded/force and the equivalent methods on MemorySegment as they use the page size to compute the base address and offset. I see this bug report was because a MemorySegment test is failing but it hints that there may be long standing issues with MappedByteBuffer too.

@AlanBateman
Copy link
Contributor

AlanBateman commented Jul 18, 2023

A bit of background: At the start of the JVM port to AIX (before OpenJDK) we wanted to use 64 KB pages, but the "LargePage" semantics used by the JVM were too rigid and too badly abstracted to be usable.

So we instead "fake" a system page size of 64 KB instead of 4 KB by, basically, letting os::vm_page_size() return 64 KB. This works surprisingly well in most cases due to some peculiarities of AIX memory management. The real 4 KB system page size is only observable in a select few corner cases, one of which is this msync coding.

Does this mean that the injected value for Unsafe.PAGE_SIZE is 64k on AIX? Should it be changed to the real page size?

@tstuefe
Copy link
Member

tstuefe commented Jul 18, 2023

A bit of background: At the start of the JVM port to AIX (before OpenJDK) we wanted to use 64 KB pages, but the "LargePage" semantics used by the JVM were too rigid and too badly abstracted to be usable.
So we instead "fake" a system page size of 64 KB instead of 4 KB by, basically, letting os::vm_page_size() return 64 KB. This works surprisingly well in most cases due to some peculiarities of AIX memory management. The real 4 KB system page size is only observable in a select few corner cases, one of which is this msync coding.

Does this mean that the injected value for Unsafe.PAGE_SIZE is 64k on AIX?

Yes :-(

@backwaterred Try changing UnsafeConstant injection such that we inject the real system page size of 4K instead of using os::vm_page_size() here:

_page_size = (int)os::vm_page_size();
. Then, the JDK would see the real 4 KB whereas hotspot would continue to use 64 KB internally. Not sure how well that separation would work, but it may be worth a try.

Mid- to long-term, we could think about reverting that decision and going back to real 4 KB. Essentially, rip out all coding in hotspot guarded by -XX:+Use64KPages. We would then see a performance drop because of the increased TLB usage. But maybe today's AIX machines are faster and can compensate for that. A future improvement could be adding real "hotspot-style" large page support using real 1 GB huge pages (which are supported on AIX but we never bothered).

@AlanBateman As a defense of our old coding: We did this long before OpenJDK existed, and at that time, there was no way to contribute code back to Sun. Today I may do things differently. But back then, we had to aim for a minimally invasive patch, and this lying-about-pagesize worked well. There may be hidden issues like this we have not discovered yet.

@AlanBateman
Copy link
Contributor

@AlanBateman As a defense of our old coding: We did this long before OpenJDK existed, and at that time, there was no way to contribute code back to Sun. Today I may do things differently. But back then, we had to aim for a minimally invasive patch, and this lying-about-pagesize worked well. There may be hidden issues like this we have not discovered yet.

No problem, it's good to get the history and get this issue to the right place. I was initially concerned we were into a scenario where the page size configuration could change and thus impact the base address and offset for operations like load and force.

@backwaterred backwaterred marked this pull request as draft July 19, 2023 19:02
@openjdk openjdk bot removed the rfr Pull request is ready for review label Jul 19, 2023
@backwaterred
Copy link
Contributor Author

Thanks for clarifying @tstuefe. It seems the issue was not Dynamic Variable Pages as I had thought. Next time, I guess I should start by assuming that the vm is lying to me 😃.

@backwaterred Try changing UnsafeConstant injection such that we inject the real system page size of 4K instead of using os::vm_page_size()

I am currently testing this.

@backwaterred
Copy link
Contributor Author

Closing in favour of Thomas' suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nio nio-dev@openjdk.org
4 participants