Navigation Menu

Skip to content
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

8284950: CgroupV1 detection code should consider memory.swappiness #8285

Closed
wants to merge 20 commits into from

Conversation

xpbob
Copy link
Contributor

@xpbob xpbob commented Apr 18, 2022

set memory.swappiness to 0,swap space will not be used
determine the value of memory.swappiness
https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt

    Memory Limit: 50.00M
    Memory Soft Limit: Unlimited
    Memory & Swap Limit: 100.00M
    Maximum Processes Limit: 4194305 

=>

    Memory Limit: 50.00M
    Memory Soft Limit: Unlimited
    Memory & Swap Limit: 50.00M
    Maximum Processes Limit: 4194305 

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (1 review required, with at least 1 reviewer)

Issue

  • JDK-8284950: CgroupV1 detection code should consider memory.swappiness

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8285/head:pull/8285
$ git checkout pull/8285

Update a local copy of the PR:
$ git checkout pull/8285
$ git pull https://git.openjdk.java.net/jdk pull/8285/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8285

View PR using the GUI difftool:
$ git pr show -t 8285

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8285.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 18, 2022

👋 Welcome back xpbob! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 18, 2022
@openjdk
Copy link

openjdk bot commented Apr 18, 2022

@xpbob The following label will be automatically applied to this pull request:

  • core-libs

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.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Apr 18, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 18, 2022

return retval > 0;
long memswBytes = getLongValue(controller, "memory.memsw.limit_in_bytes");
long swappiness = getLongValue(controller, "memory.swappiness");
return (memswBytes > 0 && swappiness > 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also need to be changed in the test?

oldVal = metrics.getMemoryAndSwapLimit();
newVal = getLongValueFromFile(Controller.MEMORY, "memory.memsw.limit_in_bytes");
newVal = newVal > unlimited_minimum ? CgroupSubsystem.LONG_RETVAL_UNLIMITED : newVal;
if (!CgroupMetricsTester.compareWithErrorMargin(oldVal, newVal)) {
fail(Controller.MEMORY, "memory.memsw.limit_in_bytes", oldVal, newVal);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also corresponding code in HotSpot:

jlong CgroupV1Subsystem::memory_and_swap_limit_in_bytes() {
GET_CONTAINER_INFO(julong, _memory->controller(), "/memory.memsw.limit_in_bytes",
"Memory and Swap Limit is: " JULONG_FORMAT, JULONG_FORMAT, memswlimit);
if (memswlimit >= _unlimited_memory) {
log_trace(os, container)("Non-Hierarchical Memory and Swap Limit is: Unlimited");
CgroupV1MemoryController* mem_controller = reinterpret_cast<CgroupV1MemoryController*>(_memory->controller());
if (mem_controller->is_hierarchical()) {
const char* matchline = "hierarchical_memsw_limit";
const char* format = "%s " JULONG_FORMAT;
GET_CONTAINER_INFO_LINE(julong, _memory->controller(), "/memory.stat", matchline,
"Hierarchical Memory and Swap Limit is : " JULONG_FORMAT, format, hier_memlimit)
if (hier_memlimit >= _unlimited_memory) {
log_trace(os, container)("Hierarchical Memory and Swap Limit is: Unlimited");
} else {
return (jlong)hier_memlimit;
}
}
return (jlong)-1;
} else {
return (jlong)memswlimit;
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also need to be changed in the test?

oldVal = metrics.getMemoryAndSwapLimit();
newVal = getLongValueFromFile(Controller.MEMORY, "memory.memsw.limit_in_bytes");
newVal = newVal > unlimited_minimum ? CgroupSubsystem.LONG_RETVAL_UNLIMITED : newVal;
if (!CgroupMetricsTester.compareWithErrorMargin(oldVal, newVal)) {
fail(Controller.MEMORY, "memory.memsw.limit_in_bytes", oldVal, newVal);
}

This condition returns false,The following code is skipped

if (metrics.getMemoryAndSwapLimit() > metrics.getMemoryLimit()) {

@iklam
Copy link
Member

iklam commented Apr 20, 2022

Both the PR and the bug report are not clear about exactly what the problem is.

Could you provide a test case that demonstrates the relationship between memory.memsw.limit_in_bytes and memory.swappiness? It will be best if you can upload a shell script into the bug report so we know the exact steps to reproduce the problem.

Ultimately we should add a new jtreg test case.

# scenario 1
memory.memsw.limit_in_bytes = 1000M
memory.limit_in_bytes = 50M
memory.swappiness = 1

-> "java -Xms60m -XX:+AlwaysPreTouch -version" can successfully complete

# scenario 2
memory.memsw.limit_in_bytes = 1000M
memory.limit_in_bytes = 50M
memory.swappiness = 0

-> "java -Xms60m -XX:+AlwaysPreTouch -version" will be killed by cgroups

Related to your other PR (#8256), I think CgroupV1Subsystem::memory_and_swap_limit_in_bytes() also need to be changed so that it will return 50M in scenario 2.

@iklam
Copy link
Member

iklam commented Apr 20, 2022

I changed the JBS issue summary to "CgroupV1 detection code should consider memory.swappiness"

@xpbob xpbob changed the title 8284950: Swappiness disables swap space usage 8284950: CgroupV1 detection code should consider memory.swappiness Apr 20, 2022
Comment on lines 162 to 164
opts.addDockerOpts("--memory-swappiness", "0");
} else {
opts.addDockerOpts("--memory-swappiness", "60");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this breaks on a cgroups v2 system as --memory-swappiness is not supported there. I'd prefer if this wouldn't piggy back on the existing test, but actually assert that swap is properly reported as the same as the memory limit if --memory-swappiness=0. Also, this test only verifies the Java (core-libs) change, not the hotspot change. That would have to be done via some TestMisc variant which uses print_container_info().

@xpbob
Copy link
Contributor Author

xpbob commented Apr 27, 2022

Thanks for review.
@iklam @jerboaa
I added test to TestMisc with print_container_info and -XshowSettings:system

@xpbob
Copy link
Contributor Author

xpbob commented Apr 27, 2022

I refer to the content of this test

.addDockerOpts("--memory", dockerMemLimit, "--memory-swappiness", "0", "--memory-swap", dockerMemLimit);

@@ -57,6 +57,7 @@ public static void main(String[] args) throws Exception {
testIsContainerized();
testPrintContainerInfo();
testPrintContainerInfoActiveProcessorCount();
testPrintContainerMemoryInfo("100M", "150M");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again. This test runs unconditionally. --memory-swappiness is not supported in cgroups v2. Thus, the test will fail on a cgroups v2 system. You need to only run this test on a cgroups v1 system. Have a look at test/lib/jdk/test/lib/containers/cgroup/MetricsTester.java how could could detect this and only run on cgroupv1 providers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my cgroups v2 system (i.e. it's using 200m memory + 200m swap):

$ sudo docker run --rm -ti --memory-swappiness=0 --memory=200m fedora:35
WARNING: Your kernel does not support memory swappiness capabilities or the cgroup is not mounted. Memory swappiness discarded.
[root@fb3182c8e23c /]# cat /sys/fs/cgroup/memory.max 
209715200
[root@fb3182c8e23c /]# cat /sys/fs/cgroup/memory.swap.max 
209715200

@openjdk openjdk bot removed the rfr Pull request is ready for review label May 3, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label May 3, 2022
@xpbob
Copy link
Contributor Author

xpbob commented May 3, 2022

Thanks for review.
@jerboaa
I added new test cases to run only in cgroupv1 environment

@xpbob
Copy link
Contributor Author

xpbob commented May 3, 2022

Thanks for review.
@jerboaa
The code has been updated

Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks much better. Thanks!

Common.addWhiteBoxOpts(opts);

OutputAnalyzer out = Common.run(opts);
System.out.println(out.getOutput());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like debug output?

System.out.println("TEST PASSED!!!");
return;
}
if ("cgroupv1".equals(metrics.getProvider())) {

This comment was marked as outdated.

@xpbob
Copy link
Contributor Author

xpbob commented May 4, 2022

Thanks for review.
@jerboaa
The code has been updated

Comment on lines 148 to 154
GET_CONTAINER_INFO(julong, _memory->controller(), "/memory.swappiness",
"Swappiness is: " JULONG_FORMAT, JULONG_FORMAT, swappiness);
if (swappiness == 0) {
jlong memlimit = read_memory_limit_in_bytes();
log_trace(os, container)("Memory and Swap Limit has been reset to " JULONG_FORMAT " because swappiness is 0", memlimit);
return memlimit;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It now occurs to me that we'd need the same treatment of this on line 143. Perhaps extract a function and call it in both places?

@xpbob
Copy link
Contributor Author

xpbob commented May 12, 2022

Thanks for review.
@jerboaa
I recently had a minor operation, so I couldn't get back on time.
Read values in different places, I added a method read_mem_swappiness
Is that possible?

Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I think the only thing missing is a test for the JDK side. Perhaps write one using the OperatingSystemMXBean's getTotalSwapSpaceSize() method within a container with --memory=200m --memory-swap=250m and using --memory-swappiness=0. You could use test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java and CheckOperatingSystemMXBean.java as a model. Again a cgroupsv1 specific test.

@xpbob
Copy link
Contributor Author

xpbob commented May 12, 2022

Thanks for review.
@jerboaa
Added test in TestMemoryWithCgroupV1

Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Consider a better name for the test :)

.shouldContain("Memory & Swap Limit: " + expectedLimit);
}

private static void testOperatingSystemMXBeanAwareness(String memoryAllocation, String swapAllocation,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a more telling name for this. Perhaps this? testOSBeanSwappinessMemory.

@openjdk
Copy link

openjdk bot commented May 12, 2022

@xpbob 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:

8284950: CgroupV1 detection code should consider memory.swappiness

Reviewed-by: sgehwolf, iklam

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 414 new commits pushed to the master branch:

  • e2448ce: 8286791: CLONE - ProblemList compiler/c2/irTests/TestSkeletonPredicates.java in -Xcomp mode
  • 357f990: 8286428: AlgorithmId should understand PBES2
  • f4f1ddd: 8284194: Allow empty subject fields in keytool
  • dc94621: 8286782: Exclude vmTestbase/gc/gctests/WeakReference/weak006/weak006.java
  • 0e4bece: 8286705: GCC 12 reports use-after-free potential bugs
  • 63bd3b7: 8286773: cleanup @returns in sun.security classes
  • af24d2d: 8286771: workaround implemented for JDK-8282607 is incomplete
  • 80cf9f3: 8286594: (zipfs) Mention paths with dot elements in ZipException and cleanups
  • 29c4b8e: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
  • 9eb15c9: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
  • ... and 404 more: https://git.openjdk.java.net/jdk/compare/21ea740e1da48054ee46efda493d0812a35d786e...master

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, @iklam) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 12, 2022
@xpbob
Copy link
Contributor Author

xpbob commented May 12, 2022

Thanks for review.
@jerboaa
The expression is clearer after the change

Copy link
Contributor

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@xpbob
Copy link
Contributor Author

xpbob commented May 16, 2022

@iklam , are you also fine with the latest change? Thanks.

Copy link
Member

@iklam iklam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xpbob
Copy link
Contributor Author

xpbob commented May 16, 2022

Thanks @jerboaa and @iklam . :)

@xpbob
Copy link
Contributor Author

xpbob commented May 16, 2022

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label May 16, 2022
@openjdk
Copy link

openjdk bot commented May 16, 2022

@xpbob
Your change (at version 584488f) is now ready to be sponsored by a Committer.

@DamonFool
Copy link
Member

/sponsor

@openjdk
Copy link

openjdk bot commented May 16, 2022

Going to push as commit 46d208f.
Since your change was applied there have been 414 commits pushed to the master branch:

  • e2448ce: 8286791: CLONE - ProblemList compiler/c2/irTests/TestSkeletonPredicates.java in -Xcomp mode
  • 357f990: 8286428: AlgorithmId should understand PBES2
  • f4f1ddd: 8284194: Allow empty subject fields in keytool
  • dc94621: 8286782: Exclude vmTestbase/gc/gctests/WeakReference/weak006/weak006.java
  • 0e4bece: 8286705: GCC 12 reports use-after-free potential bugs
  • 63bd3b7: 8286773: cleanup @returns in sun.security classes
  • af24d2d: 8286771: workaround implemented for JDK-8282607 is incomplete
  • 80cf9f3: 8286594: (zipfs) Mention paths with dot elements in ZipException and cleanups
  • 29c4b8e: 8286444: javac errors after JDK-8251329 are not helpful enough to find root cause
  • 9eb15c9: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
  • ... and 404 more: https://git.openjdk.java.net/jdk/compare/21ea740e1da48054ee46efda493d0812a35d786e...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 16, 2022
@openjdk openjdk bot closed this May 16, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels May 16, 2022
@openjdk
Copy link

openjdk bot commented May 16, 2022

@DamonFool @xpbob Pushed as commit 46d208f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants