Locate and read cgroup files for cgroup v2 #6432

EricYangIBM · 2022-03-24T14:36:01Z

Add isCgroupV2Available to detect if cgroup v2 is available on the system.
Split readCgroupFile into populateCgroupEntryListV1 and populateCgroupEntryListV2,
where the latter is added for cgroup v2 to fetch enabled subsystems from
$MOUNT_POINT/cgroupName/cgroup.controllers.
Update getCgroupMemoryLimit (and its helpers) for cgroup v2: read the correct
controller files at MOUNT_POINT/cgroupName/ and account for the possible "max"
values in these files.
Add PPG_sysinfoControlFlags global to cache if cgroup v1 or v2 is available,
or if the process is running in a container.

Issue: #1281
Signed-off-by: Eric Yang eric.yang@ibm.com

babsingh · 2022-03-24T15:04:03Z

In this PR, I see changes from #6422. All changes are in one commit. First, it won't rebase automatically; there will be conflicts. Second, the new changes cannot be distinguished from older ones. To avoid these issues, you need to add a new commit (with only new changes) on top of the commit in #6422.

babsingh · 2022-03-24T15:52:00Z

Some feedback

readCgroupFile is invoked only once to populate PPG_cgroupEntryList from omrsysinfo_cgroup_is_system_available. Currently, it is explicitly tailored for cgroup v1.
Instead of modifying readCgroupFile to work with both cgroup v1 and v2, we should add a new function, a cgroup v2 variant for readCgroupFile.
This will keep v1 and v2 logic separated. Thus, reducing the chances to break the existing v1 code.

readCgroupFile -> populateCgroupV1EntryList { ... Old cgroup v1 logic

populateCgroupV2EntryList() { ... New cgroup v2 logic

omrsysinfo_cgroup_is_system_available() {
     ...
     if (isCgroupV1Available) : populateCgroupV1EntryList()
     else if (isCgroupV2Available) : populateCgroupV2EntryList()
     else Trace point to indicate PPG_cgroupEntryList was not populated

EricYangIBM · 2022-03-25T16:45:34Z

With these changes the heap size for cgroups v2 matches that of v1 for a 4GB docker

root@63d105473c38:~/hostdir/openj9-openjdk-jdk8# build/linux-x86_64-normal-server-release/images/j2sdk-image/bin/java -XshowSettings:vm -XX:+OriginalJDK8HeapSizeCompatibilityMode -version
MM_GCExtensions::computeDefaultMaxHeapForJava usablePhysicalMemory: 4294967296
Subsystems enabled
memlimit set
memoryMax: 3221225472
VM settings:
    Max. Heap Size (Estimated): 3.00G

babsingh · 2022-03-28T12:48:52Z

@pshipton This should fix ibmruntimes/ci.docker#124 which is linked to eclipse-openj9/openj9#14190.

Also, can you confirm if the below trace point rules still apply?

New trace points should be defined at the end of the file.
Old trace points should not be modified or removed.
Instead, a modified version of the old trace point should be defined at the end of the file, and the usage of the old trace point should be stopped.

EricYangIBM · 2022-03-28T13:10:21Z

Forgot about the rules for adding tracepoints. But according to https://github.com/eclipse-openj9/openj9/blob/master/doc/diagnostics/AddingTracepoints.md my modifications of old trace points is fine since they don't modify the signature of the format specifiers.

pshipton · 2022-03-28T13:57:27Z

The older tracepoint names can't be modified. The original names need to be preserved so the latest build can still process older tracepoint files. It's ok to change some text (without changing signatures), as long as the the new text still makes sense in the context of processing older tracepoint files.

If a tracepoint is no longer used, you can add the Obsolete keyword to it.

EricYangIBM · 2022-03-28T14:20:36Z

I thought that the formatter looks at the ordering of the tracepoints and not the name so the position of a tracepoint in the file determines which old and new tracepoints are matched. If this isn't the case, then maybe we should mention that names can't be changed in the documentation.

pshipton · 2022-03-28T14:28:26Z

Maybe @keithc-ca knows for sure?

pshipton · 2022-03-28T14:29:24Z

The doc at https://github.com/eclipse-openj9/openj9/blob/master/doc/diagnostics/AddingTracepoints.md
shows
2. If a tracepoint's signature changes, obsolete it and create a new one. By signature we mean the types, order and total number of format specifiers in the Template. Changes to the Tracepoint Type, Overhead, Level and NoEnv parameters, and cosmetic changes to the text in the Template can be made without adding a new tracepoint.

keithc-ca · 2022-03-28T14:45:36Z

@EricYangIBM is correct: the name of the tracepoint is not relevant, just it's position within the *.tdf file that matters.
On the other hand, changing the name of a tracepoint will make understanding their history more difficult: I'm not sure it's warranted here. If we want to have the tracepoint names match the function name, either don't rename the function or add new tracepoints (and mark the existing ones as obsolete). There is a small (but IMO negligible) cost to having obsolete tracepoints, I don't think that cost justifies the potential for confusion down the road.

babsingh

first review pass ... please notify once the all the comments have been addressed ... then, will do a second review pass.

port/unix/omrsysinfo.c

port/common/omrport.tdf

port/unix/omrsysinfo.c

babsingh · 2022-04-11T20:48:22Z

jenkins build all

babsingh · 2022-04-12T14:09:05Z

Downstream (OpenJ9) testing should be good based upon the builds listed in #6432 (comment).

@EricYangIBM Can you locally verify and confirm if ibmruntimes/ci.docker#124 is fixed with the latest changes?

EricYangIBM · 2022-04-12T14:33:54Z

These changes seem to fix the docker issue (-XX:+OriginalJDK8HeapSizeCompatibilityMode is default, but heap size is the same with or without it):

root@285a1ceaa59d:~/hostdir/openj9-openjdk-jdk8# build/linux-x86_64-normal-server-release/images/j2sdk-image/bin/java -XshowSettings:vm -XX:+OriginalJDK8HeapSizeCompatibilityMode -version
VM settings:
    Max. Heap Size (Estimated): 3.00G
    Ergonomics Machine Class: server
    Using VM: Eclipse OpenJ9 VM

babsingh

lgtm. @keithc-ca @0xdaryl, for final review and merge.

EricYangIBM · 2022-04-19T14:29:34Z

Is this ready to merge?

babsingh · 2022-04-19T15:14:03Z

Is this ready to merge?

Yes, lgtm. Waiting for @keithc-ca and @0xdaryl to approve.

keithc-ca · 2022-04-19T15:33:39Z

port/unix/omrsysinfo.c

+	requiredSize = portLibrary->str_printf(portLibrary, NULL, 0, "/proc/%d/cgroup", pid);
+	Assert_PRT_true(requiredSize <= PATH_MAX);
+	portLibrary->str_printf(portLibrary, cgroupFilePath, sizeof(cgroupFilePath), "/proc/%d/cgroup", pid);


This could be improved (in a future pull request): there's no reason to call str_printf() twice if we are going to insist the result fit the the buffer we already have.
The assertion (that would go away) should use sizeof(cgroupFilePath) instead of PATH_MAX.

tajila · 2022-04-19T16:05:11Z

@0xdaryl Please review and merge these changes

In eclipse-omr#6432, OMR port library started throwing an error if isRunningInContainer failed. isRunningInContainer can fail if /proc is mounted with the hidepid=2 setting on Linux (eclipse-omr#7021). This prevents a JVM user to start. Before eclipse-omr#6432, no error was returned if isRunningInContainer failed; a user was completely unaware of this failure; this behaviour can lead to performance issues if the process is running in a container; but no functional issues will be seen. The new behaviour will not thrown an error if isRunningInContainer fails, but will issue a warning message to highlight the potential performance impact. Currently, isRunningInContainer is run from omrsysinfo_startup. Neither the trace engine nor NLS messages are enabled at this point. If there is an error in isRunningInContainer, no tracepoint or NLS message will work inside isRunningInContainer. Invocation of isRunningInContainer is delayed to first-use. In OpenJ9, the first-use still happens before the trace engine is initialized, but it happens after the NLS messages are enabled. A new NLS message has been added in eclipse-openj9/openj9#17560, which will show up as a warning when isRunningInContainer fails and highlight the potential performance impact. The result of isRunningInContainer is cached and updated via an atomic operation to enforce data consistency. The caching helps to improve performance when isRunningInContainer is repeatedly invoked. Four new states are introduced for PPG_isRunningInContainer to support the new changes: - OMRPORT_RUNNING_IN_CONTAINER_UNINITIALIZED: evaluate the result of isRunningInContainer. - OMRPORT_RUNNING_IN_CONTAINER_TRUE: inside a container. - OMRPORT_RUNNING_IN_CONTAINER_FALSE: not in a container. - OMRPORT_RUNNING_IN_CONTAINER_ERROR: an error was encountered while evaluating isRunningInContainer. Related: eclipse-omr#7021 Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

In eclipse-omr#6432, OMR port library started throwing an error if isRunningInContainer failed. isRunningInContainer can fail if /proc is mounted with the hidepid=2 setting on Linux (eclipse-omr#7021). This prevents a JVM user to start. Before eclipse-omr#6432, no error was returned if isRunningInContainer failed; a user was completely unaware of this failure; this behaviour can lead to performance issues if the process is running in a container; but no functional issues will be seen. The new behaviour will not throw an error if isRunningInContainer fails, but will issue a warning message to highlight the potential performance impact. Currently, isRunningInContainer is run from omrsysinfo_startup. Neither the trace engine nor NLS messages are enabled at this point. If there is an error in isRunningInContainer, no tracepoint or NLS message will work inside isRunningInContainer. Invocation of isRunningInContainer is delayed to first-use. In OpenJ9, the first-use still happens before the trace engine is initialized, but it happens after the NLS messages are enabled. A new NLS message has been added in eclipse-openj9/openj9#17560, which will show up as a warning when isRunningInContainer fails and highlight the potential performance impact. The result of isRunningInContainer is cached and updated via an atomic operation to enforce data consistency. The caching helps to improve performance when isRunningInContainer is repeatedly invoked. Four new states are introduced for PPG_isRunningInContainer to support the new changes: - OMRPORT_RUNNING_IN_CONTAINER_UNINITIALIZED: evaluate the result of isRunningInContainer. - OMRPORT_RUNNING_IN_CONTAINER_TRUE: inside a container. - OMRPORT_RUNNING_IN_CONTAINER_FALSE: not in a container. - OMRPORT_RUNNING_IN_CONTAINER_ERROR: an error is encountered while evaluating isRunningInContainer. Related: eclipse-omr#7021 Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>

github-actions bot added the comp:port label Mar 24, 2022

babsingh self-assigned this Mar 24, 2022

EricYangIBM force-pushed the cgroup2 branch from 741b69a to 4bdeadc Compare March 24, 2022 15:36

EricYangIBM force-pushed the cgroup2 branch 2 times, most recently from 1332c4e to 3e27238 Compare March 25, 2022 16:42

EricYangIBM marked this pull request as ready for review March 25, 2022 16:48

EricYangIBM requested review from charliegracie, youngar, rwy7 and mstoodle as code owners March 25, 2022 16:48

EricYangIBM force-pushed the cgroup2 branch from 667d49d to a7e184c Compare March 28, 2022 14:53

babsingh requested changes Mar 28, 2022

View reviewed changes

keithc-ca reviewed Mar 28, 2022

View reviewed changes

port/unix/omrsysinfo.c Outdated Show resolved Hide resolved

EricYangIBM force-pushed the cgroup2 branch from 0639726 to 7ffcc24 Compare March 29, 2022 16:29

keithc-ca reviewed Mar 29, 2022

View reviewed changes

port/unix/omrsysinfo.c Outdated Show resolved Hide resolved

EricYangIBM force-pushed the cgroup2 branch from e86ce98 to 243d240 Compare March 29, 2022 18:15

babsingh reviewed Mar 29, 2022

View reviewed changes

port/common/omrport.tdf Outdated Show resolved Hide resolved

babsingh reviewed Mar 29, 2022

View reviewed changes

port/unix/omrsysinfo.c Outdated Show resolved Hide resolved

babsingh reviewed Mar 29, 2022

View reviewed changes

port/unix/omrsysinfo.c Outdated Show resolved Hide resolved

EricYangIBM force-pushed the cgroup2 branch from 2a716cd to 48e6619 Compare March 30, 2022 13:01

babsingh approved these changes Apr 12, 2022

View reviewed changes

babsingh mentioned this pull request Apr 16, 2022

Collect cgroup v2 memory stats #6465

Merged

keithc-ca reviewed Apr 19, 2022

View reviewed changes

keithc-ca approved these changes Apr 19, 2022

View reviewed changes

babsingh mentioned this pull request Apr 19, 2022

Expose limits imposed by cgroups via port library APIs #1281

Open

6 tasks

0xdaryl self-assigned this Apr 19, 2022

0xdaryl approved these changes Apr 19, 2022

View reviewed changes

0xdaryl merged commit 04d7395 into eclipse-omr:master Apr 19, 2022

EricYangIBM deleted the cgroup2 branch April 20, 2022 12:56

babsingh mentioned this pull request Apr 25, 2022

Update omrsysinfo_get_number_CPUs_by_type for cgroup v2 #6479

Merged

babsingh mentioned this pull request May 17, 2022

Memory management is not correct when using cgroup v2 eclipse-openj9/openj9#14190

Closed

babsingh mentioned this pull request Jun 8, 2023

Ignore errors, delay and cache the result of isRunningInContainer #7029

Merged

babsingh mentioned this pull request Jun 20, 2023

JVM fails to start when /proc is mounted with hidepid=2 eclipse-openj9/openj9-docs#1113

Closed

tbitonti mentioned this pull request Oct 31, 2023

CpuInfo needs to handle "cgroups v2" filesystem structure OpenLiberty/open-liberty#24588

Open

Locate and read cgroup files for cgroup v2 #6432

Locate and read cgroup files for cgroup v2 #6432

Uh oh!

Conversation

EricYangIBM commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babsingh commented Mar 24, 2022

Uh oh!

babsingh commented Mar 24, 2022

Uh oh!

EricYangIBM commented Mar 25, 2022

Uh oh!

babsingh commented Mar 28, 2022

Uh oh!

EricYangIBM commented Mar 28, 2022

Uh oh!

pshipton commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EricYangIBM commented Mar 28, 2022

Uh oh!

pshipton commented Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pshipton commented Mar 28, 2022

Uh oh!

keithc-ca commented Mar 28, 2022

Uh oh!

babsingh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

babsingh commented Apr 11, 2022

Uh oh!

babsingh commented Apr 12, 2022

Uh oh!

EricYangIBM commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babsingh left a comment

Choose a reason for hiding this comment

Uh oh!

EricYangIBM commented Apr 19, 2022

Uh oh!

babsingh commented Apr 19, 2022

Uh oh!

keithc-ca Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

tajila commented Apr 19, 2022

Uh oh!

Uh oh!

EricYangIBM commented Mar 24, 2022 •

edited

Loading

pshipton commented Mar 28, 2022 •

edited

Loading

pshipton commented Mar 28, 2022 •

edited

Loading

EricYangIBM commented Apr 12, 2022 •

edited

Loading