-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
JDK-8313319: [linux] mmap should use MAP_FIXED_NOREPLACE if available #15170
Conversation
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
9276e14
to
4f9eec8
Compare
4f9eec8
to
bd17fe2
Compare
Windows x64 GHA errors unrelated |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Thanks for this and looks good to me. Two small nits.
// Backward compatibility: Older kernels will ignore the unknown flag; so mmap will behave | ||
// as in mode (a). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this in case of a libc that's more up to date than the kernel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The libc has little to do with it. mmap is a system call, and the flags should come from mman.h, which should be a kernel header.
But I am not sure whether the flags we use when including mman.h are actually directly from the kernel, because the glibc contains the same flags and talks about "kernel header is not namespace clean", whatever that means.
Bottomline, on the build machine we either have the flag - from libc or kernel - or we don't, in which case we define it ourselves. At runtime, the kernel either understands the flag or it don't, in the latter case it shears the flag off and then behaves as if the flag had not been given.
See also man 2 mmap:
Note that older kernels which do not recognize the MAP_FIXED_NOREPLACE flag will typically (upon detecting a collision with a preexisting mapping) fall back to a "non-MAP_FIXED" type of behavior: they will return an address that is different from the requested address. Therefore, backward-compatible software should check the returned address against the requested address.
ASSERT_EQ(p2, (char*)nullptr); // should have failed | ||
os::release_memory(p1, M); | ||
} else { | ||
tty->print_cr("Skipped."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you succesfully seen this printed? AFAIK Gtest only prints stderr by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it gets printed.
@tstuefe 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:
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 16 new commits pushed to the
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 |
Thanks Johan! |
Friendly ping. |
Anybody else? Need a second review. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable. I'd like to take it for a spin through our CI though. Thanks.
Thank you, David! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing was okay.
One typo left but otherwise seems fine to me.
Thanks
src/hotspot/os/linux/os_linux.cpp
Outdated
// to map there and nowhere else. Therefore we will unmap the block again, which means we | ||
// just executed a needless mmap->munmap cycle. | ||
// Since Linux 4.17, the kernel offers MAP_FIXED_NOREPLACE. With this flag, if a pre- | ||
// existing mapping exists, the kernel will not map at an alternative point but instead | ||
// return an error. We can therefore save that unnessassary mmap-munmap cycle. | ||
// return an error. We can therefore save that unnecassary mmap-munmap cycle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary is still mis-spelt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, David, for review and testing. I fixed the typo.
/integrate |
Going to push as commit 3699666.
Your commit was automatically rebased without conflicts. |
os::attempt_reserve_memory_at() is used to map memory at a specific address that we fancy. It is not used to replace an existing mapping (hence we omit MAP_FIXED).
But it may result in the kernel creating a mapping at a different address if our wish address is blocked; we then have to unmap again. That mmap-munmap cycle is unnecessary.
MAP_FIXED_NOREPLACE exists since Linux 4.17 and prevents that unnecessary cycle . We should use that if available. We need to tiptoe around a bit to deal with building on old machines/running on old kernels, though.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15170/head:pull/15170
$ git checkout pull/15170
Update a local copy of the PR:
$ git checkout pull/15170
$ git pull https://git.openjdk.org/jdk.git pull/15170/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15170
View PR using the GUI difftool:
$ git pr show -t 15170
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15170.diff
Webrev
Link to Webrev Comment