-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8303215: Make thread stacks not use huge pages #14105
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
8303215: Make thread stacks not use huge pages #14105
Conversation
|
👋 Welcome back poonam! A progress list of the required criteria for merging this PR into |
|
@poonamparhar The following label will be automatically applied to this pull request:
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. |
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.
Yes, this is a pragmatic workaround. I believe @theRealAph recommended the same solution.
Did you test this on an aarch64 kernel with 64k pages? I suspect the 2040 default size only works for 4k or 8k pages, for larger page sizes pthread_create would just round up the stack size again to 2MB.
This does not solve the problem when the default small page size is 64K. The stack size gets rounded up to 2048K. JavaThread: non-JavaThread: For 64k page size, changing the stack size to 1920K (2048 - 2*64) would work: |
|
For the 64K page size, the default huge page size is usually 512M. The aarch64 machine with 64K page size that I am testing on has: And on that, the current set of changes behave correctly when I run with -Xss512m. Snippet of pmap output: an additional 64k page gets added to the stack size to disturb the large page alignment. |
|
Does this PR affect only Linux/aarch64? If so, I think it's better to add that into the bug title. |
It affects all platforms, reproducible if you set stack size to 2M and THP to always. Aarch64 has 2M as default though. |
tstuefe
left a comment
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.
Some small nits remain.
tstuefe
left a comment
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.
Looks good. Thanks for fixing.
|
@poonamparhar 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 122 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 |
src/hotspot/os/linux/os_linux.cpp
Outdated
|
|
||
| // 1) Handle the case where we do not want to use huge pages and hence | ||
| // there is no need to scan the OS for related info |
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 comment needs updating as now you do the scan first.
src/hotspot/os/linux/os_linux.cpp
Outdated
| if (stack_size >= os::Linux::default_large_page_size() && is_aligned(stack_size, os::Linux::default_large_page_size())) { | ||
| stack_size += os::vm_page_size(); | ||
| } |
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.
Do we need to watch for a zero default size here so that we don't expand all stacks by a page?
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.
Added a check for zero default page size. Please review.
src/hotspot/os/linux/os_linux.cpp
Outdated
| } | ||
|
|
||
| void os::large_page_init() { | ||
| // Scan default large page size |
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.
Suggestion:
// Always initialize the default large page size even if large pages are not being used.
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.
Changed the comment.
| define_pd_global(intx, VMThreadStackSize, 2048); | ||
|
|
||
| define_pd_global(intx, CompilerThreadStackSize, 2048); | ||
| // set default stack sizes < 2MB so as to prevent stacks from getting |
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.
/set/Set/
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.
Done.
| define_pd_global(intx, CompilerThreadStackSize, 2048); | ||
| // set default stack sizes < 2MB so as to prevent stacks from getting | ||
| // large-page aligned and backed by THPs on systems where 2MB is the | ||
| // default huge page size. For non-JavaThreads, glibc adds an additional |
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.
s/adds/may add/
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.
Done.
dholmes-ora
left a comment
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.
One minor nit but otherwise nothing further from me.
Thanks.
|
@tstuefe Hi Thomas, I have addressed the review comments from David Holmes. Could you please take a look at the updated changes again? Thanks! |
Still good. Thanks. |
|
/integrate |
|
Going to push as commit 59d9d9f.
Your commit was automatically rebased without conflicts. |
|
@poonamparhar Pushed as commit 59d9d9f. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
When a system has Transparent Huge Pages (THP) enabled (/sys/kernel/mm/transparent_hugepage/enabled is set to 'always'), thread stacks can have significantly more resident set size (RSS) than they actually require. This occurs when the stack size is 2MB or larger, which makes the memory range of the stack more likely to be aligned on a Large Page Size boundary (2MB on most systems). This in turn makes the stack eligible to be backed by transparent huge pages resulting in more memory consumption than it would otherwise when standard small pages are used. This issue is more apparent on AArch64 platforms where the default stack size is 2MB.
This fix addresses this issue with the following two main changes:
Example mapping for a JavaThread:
Example Mapping for a non-JavaThread (WatcherThread):
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14105/head:pull/14105$ git checkout pull/14105Update a local copy of the PR:
$ git checkout pull/14105$ git pull https://git.openjdk.org/jdk.git pull/14105/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14105View PR using the GUI difftool:
$ git pr show -t 14105Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14105.diff
Webrev
Link to Webrev Comment