-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8349988: Change cgroup version detection logic to not depend on /proc/cgroups #23811
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
8349988: Change cgroup version detection logic to not depend on /proc/cgroups #23811
Conversation
|
👋 Welcome back fitzsim! A progress list of the required criteria for merging this PR into |
|
@fitzsim 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 21 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 (@jerboaa, @ashu-mehra) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@fitzsim Please use |
|
/issue add JDK-8347811 |
|
@fitzsim |
|
The actual changes are easier to see when whitespace is ignored: 39a6463?w=1 |
|
I fixed an existing assert message typo that I noticed while working on the patch, |
Webrevs
|
jerboaa
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.
Thanks for this. It's getting there...
|
/reviewers 2 |
|
/contributor add jerboaa |
|
@fitzsim Syntax:
User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address. |
|
/contributor add @jerboaa |
|
@fitzsim |
Also fix the testCgroupv1SystemdOnly and testCgroupv1NoMounts test cases such that their /proc/cgroups and /proc/self/cgroup contents correspond. This prevents assertion failures these tests were producing when is_cgroupsV2 was replaced with cgroups_v2_enabled.
Do not log enabled controllers that are not relevant to the JDK.
Remove from cgroups v1 branch incorrect log messages about cpuset controller being optional. Add test case for cgroups v1, cpuset disabled.
|
/integrate |
|
@fitzsim This pull request has not yet been marked as ready for integration. |
ashu-mehra
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.
lgtm
jerboaa
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.
Still good.
|
/integrate |
|
Thank you for re-reviewing, @jerboaa and @ashu-mehra. I have issued the |
|
/sponsor |
|
Going to push as commit 9c5ed23.
Your commit was automatically rebased without conflicts. |
|
In case anyone comes across this, I filed it as https://bugs.launchpad.net/ubuntu/+source/linux-meta-hwe-6.14/+bug/2117446 Effectiveness of patch confirmed on Ubuntu 24.04.2 HWE using Docker CE 28.3.2 by git checkout 9c5ed23eac7470f56d498e9c4d3c51c2f80fd571^
bash configure --with-boot-jdk=$HOME/.sdkman/candidates/java/24-tem
make images
docker run -m 1GB --rm -v $(pwd)/build/linux-x86_64-server-release/images/jdk:/jdk ubuntu /jdk/bin/java '-Xlog:os*=trace' -XshowSettings:vm -XX:MaxRAMPercentage=50 -version
git checkout 9c5ed23eac7470f56d498e9c4d3c51c2f80fd571
make images
docker run -m 1GB --rm -v $(pwd)/build/linux-x86_64-server-release/images/jdk:/jdk ubuntu /jdk/bin/java '-Xlog:os*=trace' -XshowSettings:vm -XX:MaxRAMPercentage=50 -versionBefore: shows and max heap size is ½ of physical RAM. After, and max heap size is 512Mb as expected. |
I tend to agree. Note that it's an issue on how the actual kernel is configured. But Ubuntu 24.04 LTS is affected. Noted as such in the bug recently: https://bugs.openjdk.org/browse/JDK-8349988 First step would be JDK 21. JDK 17 I'm not sure. It'll depend how the patch will look like. |
JDK 21 patch is mostly clean (minor context issue). I will do the backport next week, if @fitzsim does not take it over. JDK 17 backport won't be clean, JDK 17 doesn't have JDK-8301479 and JDK-8238161 (NULLs were replaced with nullptr, fopens with os::fopen, both are irrelevant to this patch), JDK-8347129 (backport is underway) and JDK-8261242 which I consider a context conflict, not really a dependency (only the variable name has changed). |
This pull request fixes https://bugs.openjdk.org/browse/JDK-8349988 and https://bugs.openjdk.org/browse/JDK-8347811.
I tested it with:
on:
Red Hat Enterprise Linux 8 (cgroups v1 only):No change in behaviour
Fedora 41 (cgroups v2):More verbose output due to
/sys/fs/cgroup/cgroup.controllersparsing:Fedora 41 (custom kernel with cgroups v1 disabled):Fixes
cgroups v2detection:Alpine Linux v3.21 (unified, aka cgroups v2 only):Fixes
cgroups v2detection:Alpine Linux v3.21 (hybrid):No change in behaviour.
Alpine Linux v3.21 (legacy):No change in behaviour.
Progress
Issues
Reviewers
Contributors
<sgehwolf@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23811/head:pull/23811$ git checkout pull/23811Update a local copy of the PR:
$ git checkout pull/23811$ git pull https://git.openjdk.org/jdk.git pull/23811/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23811View PR using the GUI difftool:
$ git pr show -t 23811Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23811.diff
Using Webrev
Link to Webrev Comment