8346834: Tests failing with -XX:+UseNUMA due to "NUMA support disabled" warning #22948
Conversation
|
👋 Welcome back swati-sha! A progress list of the required criteria for merging this PR into |
|
@swati-sha 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 154 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 (@dholmes-ora, @kstefanj) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@swati-sha 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
|
|
I think this is a bit unfortunate. I saw the comment in the bug around the wording for the flag
I don't fully agree that it's wrong to issue a warning (just because is says "if available"), it would be wrong to issue an error and terminate the process. I see the warning as a way to inform the user that the performance feature they configured the process to use couldn't be used. If this is instead communicated as a log statement on info-level almost nobody will see it. Especially for the newly added case, where we disable NUMA when the cpu and memory nodes mismatch, I think the warning could be helpful to users. Looking at I don't oppose this change but wanted to share my view on it. |
|
Those are good points @kstefanj. This is touching a lot of areas including testing, logging (it would be handy to run tests with GC logs I'd think), as well as the "principle of least surprise" for the user. We'll leave this topic to you and @AlanBateman, @dholmes-ora and others with more experience in the wider issues. We've implemented the message both as a warning and as "info" and are happy to have either one. |
|
Just to reinforce what I've already stated, if I ask for something "if available" I don't expect to get a warning when it is not available - I knew it might not be available. If using NUMA is a potential performance boost then naturally people will want to use it, if it is available. My concern is about the potential impact on end-users who set this in their deployment settings, not the tests that shone the light on the potential problem (for which I'm grateful the tests did in fact fail!). Warning about a NUMA misconfiguration is a different matter - that is a problem that needs someone's attention to fix. |
src/hotspot/os/linux/os_linux.cpp
Outdated
| log_warning(os)("NUMA support disabled: %s", reason); | ||
| log_info(os)("NUMA support disabled: %s", reason); |
There was a problem hiding this comment.
So this is too coarse. Some reasons for disabling NUMA may require a warning because something is actually wrong. I only want to see the new warning for "Only a single NUMA node is available" not be a warning.
|
Are you going to remove com/sun/jdi/ProcessAttachTest.java and com/sun/jdi/ReattachStressTest.java from the ProblemList.txt as part of this? They were excluded due to this issue but there may be changes coming to make these tests more robust. |
If they are listed against this bug id then they must be removed, or else updated to a new bug id, as part of this change. |
|
The changes in this PR means that com/sun/jdi/ProcessAttachTest.java and com/sun/jdi/ReattachStressTest.java should pass again. JDK-8346827 was created for these two because they were causing a lot of noise in our CI, then later lots more tests started to fail and JDK-8346834 were created. My comment was just to say that the 2 JDI tests can be removed from the exclude list too. I think Chris wants to make the tests more robust but that shouldn't prevent the tests running in the mean-time. |
|
@swati-sha can we please get this integrated. Thanks |
|
/integrate |
|
@swati-sha |
|
/sponsor |
|
Going to push as commit afc4529.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@dholmes-ora @swati-sha Pushed as commit afc4529. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi All,
A number of tests launch VMs and read the output of the sub-process. The changes in JDK-8205051 mean the warning message "NUMA support disabled: Only a single NUMA node is available" is printed when running the tests -XX:+UseNUMA on system that only have one node, this breaks several tests. After update in some tests, so far, the failures are with:
java/util/logging/LoggingDeadlock2.java
tools/jar/modularJar/Basic.java
As a fix have changed logging level from "log_warning" to "log_info" when UseNUMA flag is disabled.
Thanks,
Swati Sharma
Intel
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22948/head:pull/22948$ git checkout pull/22948Update a local copy of the PR:
$ git checkout pull/22948$ git pull https://git.openjdk.org/jdk.git pull/22948/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 22948View PR using the GUI difftool:
$ git pr show -t 22948Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22948.diff
Using Webrev
Link to Webrev Comment