8324781: runtime/Thread/TestAlwaysPreTouchStacks.java failed with Expected a higher ratio between stack committed and reserved #18592
Conversation
|
/issue add 8325218 |
|
👋 Welcome back limingliu-ampere! A progress list of the required criteria for merging this PR into |
|
@limingliu-ampere 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 625 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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@stefank, @jdksjolen, @tstuefe) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@limingliu-ampere |
|
@limingliu-ampere 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
|
src/hotspot/os/linux/os_linux.cpp
Outdated
| page_size = os::vm_page_size(); | ||
| } else if (err == 0) { | ||
| page_size = 0; | ||
| } | ||
| if (UseMadvPopulateWrite) { | ||
| log_debug(gc, os)("Called madvise(" PTR_FORMAT ", " SIZE_FORMAT ", %d):" | ||
| " error='%s' (errno=%d), when THPMode::always=%d and" | ||
| " UseTransparentHugePages=%d", | ||
| p2i(first), len, MADV_POPULATE_WRITE, os::strerror(err), | ||
| err, (int)(HugePages::thp_mode() == THPMode::always), | ||
| (int)UseTransparentHugePages); |
There was a problem hiding this comment.
This function has become quite messy: It checks UseMadvPopulateWrite tree times. It mutates the input variable. It logs even when no failure occurred (This might be a bug).
I'd much prefer if we could straighten out the code into something like this (I've not tried to compile this):
size_t os::pd_pretouch_memory(void* first, void* last, size_t page_size) {
if (HugePages::thp_mode() != THPMode::always && !UseTransparentHugePages) {
// No THP. Use the platform-independent pretouch memory code.
return page_size;
}
if (!UseMadvPopulateWrite) {
// Use small pages with the platform-independent pretouch memory code.
// When using THP we need to always pre-touch using small pages as the
// OS will initially always use small pages.
return os::vm_page_size();
}
const size_t len = pointer_delta(last, first, sizeof(char)) + page_size;
if (::madvise(first, len, MADV_POPULATE_WRITE) != -1) {
// Succeeded
// 0 signals that the platform-independent pretouch memory code should not run.
return 0;
}
// madvise failed
int err = errno;
log_debug(gc, os)("Called madvise(" PTR_FORMAT ", " SIZE_FORMAT ", %d):"
" error='%s' (errno=%d), when THPMode::always=%d and"
" UseTransparentHugePages=%d",
p2i(first), len, MADV_POPULATE_WRITE, os::strerror(err),
err, (int)(HugePages::thp_mode() == THPMode::always),
(int)UseTransparentHugePages);
if (err == EINVAL) {
// Not supported. When using THP we need to always pre-touch using
// small pages as the OS will initially always use small pages.
return os::vm_page_size();
}
assert(false, "Unexpected error='%s' (errno=%d)",os::strerror(err), err);
return page_size;
}
It is unclear to me why a failure with EINVAL should return small pages but other failures should return another page size. Why would we get EINVAL if we already performed the correct checks when setting up UseMadvPopulateWrite? Or is the problem that we skip performing the necessary checks if the user forcefully sets UseMadvPopulateWrite on the command line? Maybe we shouldn't allow that?
There was a problem hiding this comment.
It is unclear to me why a failure with EINVAL should return small pages but other failures should return another page size.
Thanks to point it out. It is a mistake.
src/hotspot/os/linux/os_linux.cpp
Outdated
| FLAG_SET_DEFAULT(UseMadvPopulateWrite, (::madvise(0, 0, MADV_POPULATE_WRITE) == 0)); | ||
| } | ||
| } | ||
| log_debug(gc, os)("UseMadvPopulateWrite=%d", (int)(UseMadvPopulateWrite ? 1 : 0)); |
There was a problem hiding this comment.
Is this temporary debugging code? We have -XX:+PrintFlagsFinal to check the resulting flag values.
src/hotspot/os/linux/os_linux.cpp
Outdated
|
|
||
| // Check the availability of MADV_POPULATE_WRITE. | ||
| FLAG_SET_DEFAULT(UseMadvPopulateWrite, (::madvise(0, 0, MADV_POPULATE_WRITE) == 0)); | ||
| Linux::version_init(); |
There was a problem hiding this comment.
check_pax() and Linux::version_init() are unrelated things. Please add a blankline between them.
src/hotspot/os/linux/os_linux.cpp
Outdated
| @@ -4821,6 +4822,20 @@ jint os::init_2(void) { | |||
| } | |||
| } | |||
|
|
|||
| // Check the availability of MADV_POPULATE_WRITE. | |||
| if (FLAG_IS_DEFAULT(UseMadvPopulateWrite) && UseMadvPopulateWrite) { | |||
There was a problem hiding this comment.
Should we really allow the user to forcefully set this and trigger a path that is not going to be correct on old kernels?
… turns on UseMadvPopulteWrite when not supported
src/hotspot/os/linux/os_linux.cpp
Outdated
| if (!FLAG_IS_DEFAULT(UseMadvPopulateWrite) && !supportMadvPopulateWrite) | ||
| warning("Platform is supposed not to support MADV_POPULATE_WRITE, " | ||
| "disabling using it to pretouch (-XX:-UseMadvPopulateWrite)"); |
There was a problem hiding this comment.
I don't see the code that disables UseMadvPopulateWrite. Is it missing?
The warning message is a bit awkward. Could it be changed to something like: "Platform does not support MADV_POPULATE_WRITE, disabling using it to pretouch memory (-XX:-UseMadvPopulateWrite)"
Could you also make sure to use {} even for single statement if blocks?
src/hotspot/os/linux/os_linux.cpp
Outdated
| long os::Linux::_release_major = -1; | ||
| long os::Linux::_release_minor = -1; |
There was a problem hiding this comment.
Shouldn't this be named something like _kernel_release_*?
src/hotspot/os/linux/os_linux.cpp
Outdated
| *major = -1; | ||
| *minor = -1; | ||
|
|
||
| void os::Linux::version_init() { |
There was a problem hiding this comment.
Shouldn't this be named something like kernel_release_init?
src/hotspot/os/linux/os_linux.cpp
Outdated
| long* set_v = major; | ||
| while (*minor == -1 && walker != nullptr) { | ||
| long* set_v = &_release_major; | ||
| while (_release_minor == -1 && walker != nullptr) { | ||
| if (isdigit(walker[0])) { | ||
| *set_v = strtol(walker, &walker, 10); | ||
| set_v = minor; | ||
| set_v = &_release_minor; | ||
| } else { | ||
| ++walker; | ||
| } |
There was a problem hiding this comment.
I can't tell if this code is correct or not.
- When is walker ever nullptr?
- Don't wee need error handling for strtol?
Maybe this is all pre-existing issues, but I'd like to get someone else to review and sign off on this code.
There was a problem hiding this comment.
Not typical code for me either.
Quoting my man pages:
If endptr is not NULL, strtol() stores the address of the first
invalid character in *endptr. If there were no digits at all,
strtol() stores the original value of nptr in *endptr (and
returns 0). In particular, if *nptr is not \[aq]\0\[aq] but
**endptr is \[aq]\0\[aq] on return, the entire string is valid.
Given a string such as 2.6.5, each strtol will put walker at a new . and as minor is always set the condition in the loop will become false. The walker will never be null. Looking at the documentation, we don't need error-handling for the strtol call (won't over/underflow and will be a valid version revision).
There was a problem hiding this comment.
Basic fix: change walker != nullptr to walker[0] != '\0' and add a test case for this.
src/hotspot/os/linux/os_linux.hpp
Outdated
| static void kernel_version(long* major, long* minor) { | ||
| *major = _release_major; | ||
| *minor = _release_minor; | ||
| } |
There was a problem hiding this comment.
Do we really wan this function now, or could this be just plain getters?:
static long kernel_release_major() {
return _kernel_release_major;
}
static long kernel_release_minor() {
return _kernel_release_minor;
}
There was a problem hiding this comment.
systemMemoryBarrier_linux.cpp uses this function. If the interface was changed here, the patch would involve some unrelated things.
There was a problem hiding this comment.
Right. But you are changing the entire kernel_version() implementation and associated functions. Maybe that should be done as an initial, separate change then? Splitting this out into two RFE:s might make it easier to review as well.
There was a problem hiding this comment.
If it requires an initial change, I will prefer discussing the interface here. On the one hand, there is just one additional place to call the function, so the change would not be big. On the other hand, I'm not a author yet.
There was a problem hiding this comment.
Hmm. Thinking about this a bit more. Why do we even need to make the changes to the kernel release parsing code? It is so that we don't have to parse it more than once, right? If that's the case I don't think that's important for this PR. You could just revert all those changes, get this PR reviewed and integrated, and then, if you want, propose a new PR that makes the kernel release parsing changes. I think that would be the smoothest path forward.
|
/reviewers 2 reviewer |
src/hotspot/os/linux/os_linux.cpp
Outdated
| struct utsname buffer; | ||
| int ret = uname(&buffer); | ||
| if (ret != 0) { | ||
| log_warning(os)("uname(2) failed to get kernel version: %s", | ||
| os::errno_name(ret)); | ||
| } | ||
|
|
||
| long major = -1, minor = -1; | ||
| bool supportMadvPopulateWrite = | ||
| (ret == 0 && | ||
| (sscanf(buffer.release, "%ld.%ld", &major, &minor) == 2) && | ||
| (major > 5 || (major == 5 && minor >= 14)) && |
There was a problem hiding this comment.
Why isn't this using the os::Linux::kernel_version() function?
There was a problem hiding this comment.
Just not get the idea. Previous discussions mentioned the problems of kernel_version, so I used an alternative method. Using kernel_version here is fine with me.
There was a problem hiding this comment.
FYI: @jdksjolen is going to fix kernel_version with #18697.
src/hotspot/os/linux/os_linux.cpp
Outdated
| Linux::kernel_version(&major, &minor); | ||
| bool supportMadvPopulateWrite = | ||
| ((major > 5 || (major == 5 && minor >= 14)) && | ||
| (::madvise(0, 0, MADV_POPULATE_WRITE) == 0)); |
There was a problem hiding this comment.
Shouldn't this be checking MADV_POPULATE_WRITE_value?
I still can't get my head around the possible compile time values versus the potential runtime interpretation of the MADV_POPULATE_WRITE value.
There was a problem hiding this comment.
For example, Linux 5.4-UEK does not support populating pages, and MADV_POPULATE_WRITE is not defined there. However, the kernel internally interprets 23 as MADV_DONEXEC, while 23 is a value used by upstream kernels as MADV_POPULATE_WRITE. MADV_POPULATE_WRITE_value is used by hotspot internally, and used for compiling JDK properly when MADV_POPULATE_WRITE is not defined. MADV_POPULATE_WRITE_value was suggested there: #15781 (comment)
There was a problem hiding this comment.
Additionally, if MADV_POPULATE_WRITE is defined but not the same as MADV_POPULATE_WRITE_value, there will be a build failure, so hotspot is guaranteed to use 23 here regardless of compile time or runtime. 23 may not be interpreted as MADV_POPULATE_WRITE by a downstream kernel. A known case is a downstream of pre-5.14 kernel, so the version of kernel is checked here. This patch does not have tricks on advice numbers.
There was a problem hiding this comment.
Okay the comment:
// Some downstream kernels recognize MADV_POPULATE_WRITE_value as another
made me think we needed to check the "value" version but I see now they are already guaranteed to be the same.
|
/label add hotspot,hotspot-gc |
openjdk
bot
added
hotspot
|
@limingliu-ampere The |
|
Meanwhile, I am warming to the current approach. I understand that this it avoids referring to individual downstream vendors, which I agree may be brittle. My main concern is to prevent future flag mismatches. Therefore, my proposal is to do what this patch does, but in a more generic way. Essentially, encoding that for certain flags, we cannot rely on older kernel correctly ignoring them. But we assume that downstream kernel vendors will at least fix conflicts when they merge in flags from mainline. We sacrifice the ability to benefit from vendor-specific backports, but that is the compromise. The flags I'd like to guard for now are:
If the vendor keeps up its routine of just shifting the proprietary flags to the end of the numerical MADV range for each new mainline flag, we will continue to have problems and this list may grow. The mechanism could be very close to what @limingliu-ampere does now, only a tad more generic. E.g.: and then maybe wrap the madvise call with something like this: in addition to something like this in initialization: Do you like this, does this make sense? |
|
FYI, oracle/linux-uek#23 has recently fixed the madv flag conflict issue, at the three concerned branches: uek7/u2: oracle/linux-uek@6064722 |
- UEK has fixed the compatibility with upstream kernels; - Pretouch behaviors will be covered by other tests.
|
Hi, @tstuefe. Could you please take a look? The patch had been limited to testcases, as there were already fixes in UEK and you created a ticket to cover pretouch. |
|
/integrate |
|
@limingliu-ampere |
|
/sponsor |
|
Going to push as commit 31e8deb.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@tstuefe @limingliu-ampere Pushed as commit 31e8deb. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
/backport jdk23u |
|
@limingliu-ampere To use the |
|
I don't think @limingliu-ampere has enough rights to backport this fix. @tstuefe is this something that should be backported to JDK 23? |
Backporting this is fine and low risk |
|
/backport :jdk23 |
|
@stefank the backport was successfully created on the branch backport-stefank-31e8deba-jdk23 in my personal fork of openjdk/jdk. To create a pull request with this backport targeting openjdk/jdk:jdk23, just click the following link: The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:
If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk: |
The testcase failed on Oracle CI since JDK-8315923. The root cause is that Oracle CI runs Linux-5.4.17-UEK where the value of MADV_POPULATE_WRITE (23) is used as MADV_DONTEXEC which is not supported by upstream. This PR solves the testcase failure by checking versions of kernels first, and checking the availability of MADV_POPULATE_WRITE when they are not older than 5.14.
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18592/head:pull/18592$ git checkout pull/18592Update a local copy of the PR:
$ git checkout pull/18592$ git pull https://git.openjdk.org/jdk.git pull/18592/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18592View PR using the GUI difftool:
$ git pr show -t 18592Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18592.diff
Webrev
Link to Webrev Comment