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

Read system swappiness in omrsysinfo_get_memory_info() #7110

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

SajinaKandy
Copy link
Contributor

@SajinaKandy SajinaKandy commented Sep 7, 2023

Add tiny fixes for the work in #5237 which reads swappiness value and sets OMRPORT_MEMINFO_NOT_AVAILABLE if the value is not available.
The system wide swappiness value specified at /proc/sys/vm/swappiness for vm.swappiness is read.

Closes: #5237

@SajinaKandy
Copy link
Contributor Author

There were some changes made for retrieving memory stats differently to incorporate changes made in cgroup v2 here. However memory.swappiness is not available in cgroupv2. Discussions here.
We however are reading system wide swappiness value specified in vm.swappiness in /proc/sys/vm/swappiness so this should be good.

@babsingh
Copy link
Contributor

https://dev.azure.com/eclipse-omr/ea4519db-b27e-4d19-a971-f01491f803e3/_apis/build/builds/7957/logs/37

The failure in the PR build will need to be resolved.

2023-09-07T17:05:42.0110839Z 34: [----------] 39 tests from PortSysinfoTest
2023-09-07T17:05:42.0118198Z 34: originalSoftLimit=65536
2023-09-07T17:05:42.0149753Z 34: originalHardLimit=65536
2023-09-07T17:05:42.0150699Z 34: soft set to hard limit=65536
2023-09-07T17:05:42.0152952Z 34: 	/home/vsts/work/1/s/fvtest/porttest/si.cpp line 1308: omrsysinfo_testMemoryInfo 	Invalid memory usage statistics retrieved.
2023-09-07T17:05:42.0153644Z 34: 
2023-09-07T17:05:42.0155203Z 34: 			LastErrorNumber: 0
2023-09-07T17:05:42.0156070Z 34: 			LastErrorMessage: 
2023-09-07T17:05:42.0156265Z 34: 
2023-09-07T17:05:42.0158991Z 34: /home/vsts/work/1/s/fvtest/porttest/testHelpers.cpp:109: Failure
2023-09-07T17:05:42.0159911Z 34: Value of: 0 == numberFailedTestsInComponent
2023-09-07T17:05:42.0160426Z 34:   Actual: false
2023-09-07T17:05:42.0160855Z 34: Expected: true
2023-09-07T17:05:42.0161101Z 34: Test failed!
2023-09-07T17:05:42.0165818Z 34: [  FAILED  ] PortSysinfoTest.sysinfo_testMemoryInfo (1 ms)

https://dev.azure.com/eclipse-omr/ea4519db-b27e-4d19-a971-f01491f803e3/_apis/build/builds/7957/logs/12

Also, the issue in the line endings check will need to be resolved:

2023-09-07T16:54:22.9426028Z ###################################
2023-09-07T16:54:22.9562706Z port/unix/omrsysinfo.c:3511: trailing whitespace.

@SajinaKandy
Copy link
Contributor Author

@babsingh Thanks for reviewing. Can you please look at the fixed code.

@SajinaKandy SajinaKandy marked this pull request as ready for review October 5, 2023 17:30
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

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

LGTM

@babsingh
Copy link
Contributor

babsingh commented Oct 6, 2023

Started builds for a sanity check.

jenkins build all

@babsingh
Copy link
Contributor

babsingh commented Oct 6, 2023

Commit message guidelines: https://github.com/eclipse/omr/blob/master/CONTRIBUTING.md#commit-guidelines

  • Title doesn't match the PR description.
  • The body should be wrapped at 72 characters, where reasonable.
  • PR description is missing key details such as Closes: #5237.

Revision:

Read system swappiness in omrsysinfo_get_memory_info()

The code read swappiness value on Linux and sets
OMRPORT_MEMINFO_NOT_AVAILABLE if the value is not available.
The system wide swappiness value specified at
/proc/sys/vm/swappiness for vm.swappiness is read.

Closes: #5237

Signed-off-by: SajinaKandy <[email protected]>

include_core/omrport.h Outdated Show resolved Hide resolved
port/common/omrport.tdf Outdated Show resolved Hide resolved
port/unix/omrsysinfo.c Outdated Show resolved Hide resolved
port/unix/omrsysinfo.c Outdated Show resolved Hide resolved
port/unix/omrsysinfo.c Show resolved Hide resolved
@babsingh
Copy link
Contributor

babsingh commented Oct 6, 2023

The below description is a little confusing. Can we also add a comment on how to interpret the swappiness values and what decisions should be taken for specific values, for example, to improve performance?

From opencontainers/runtime-spec#1005,

Swappiness

Tejun: "It’s not very clear what swappiness encodes. A lot of it is compared to file-backed IOs, how [un]favorable IOs for anonymous memory are considering their inherently higher randomness. As such, it’s more a function of the underlying hardware than workloads. Also, the implementation wasn’t quite right either – iirc, the behavior would differ depending on who’s reclaiming."

I guess the only setting that makes sense here is disabling swap altogether and that setting can be better achieved by setting memory.swap.max.

The current OCI specification includes a memory.swappiness setting for this control. For backwards compatibility, we can look into possibly translating (or more specifically, reconcyling it with memory.swap) on the corner cases that try to disable swap for a specific container using this setting.

@SajinaKandy
Copy link
Contributor Author

The intention behind reading the value of swappiness was to see if the swap space is disabled using this parameter as discussed in the article you pointed out. In the current code we read J9MemoryInfo.totalSwap == 0 to confirm swap space availability however vm.swappiness=0 also disables swap so its important to check this value too, to determine swap space availability.

I have added extra comment to add few details about how the value read from this parameter can be interpreted but really this value defines a threshold when processes should be swapped out in favor of I/O caching. The default value of /proc/sys/vm/swappiness is 60 which means that applications and programs that have not done a lot lately can be swapped out. Higher values will provide more I/O cache and lower values will wait longer to swap out idle applications.

Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

@SajinaKandy Please squash the commits after addressing the current feedback; the PR will be good to merge.

port/unix/omrsysinfo.c Outdated Show resolved Hide resolved
port/unix/omrsysinfo.c Outdated Show resolved Hide resolved
port/unix_include/omrcgroup.h Outdated Show resolved Hide resolved
@SajinaKandy
Copy link
Contributor Author

The macOS failures could to be due to #6516 as the errors happen in sock_bind and sock_accept:

34: [----------] 19 tests from PortSockTest
34: /Users/runner/work/1/s/fvtest/porttest/omrsockTest.cpp:1209: Failure
34:       Expected: rc
34:       Which is: 1
34: To be equal to: 2
34: [  FAILED  ] PortSockTest.poll_functionality_basic (1002 ms)
34: /Users/runner/work/1/s/fvtest/porttest/omrsockTest.cpp:51: Failure
34:       Expected: privateOmrPortLibrary->sock_bind(privateOmrPortLibrary, *serverSocket, serverSockAddr)
34:       Which is: -506
34: To be equal to: 0
34: /Users/runner/work/1/s/fvtest/porttest/omrsockTest.cpp:1259: Failure
34:       Expected: privateOmrPortLibrary->sock_accept(privateOmrPortLibrary, serverSocket, &connectedServerSockAddr, &connectedServerSocket)
34:       Which is: -20
34: To be equal to: 0
34: [  FAILED  ] PortSockTest.poll_functionality_many_sockets (0 ms)

@babsingh
Copy link
Contributor

@SajinaKandy Code changes LGTM. I still see two commits: https://github.com/eclipse/omr/pull/7110/commits. Can you squash them into a single commit?

The code read swappiness value on Linux and sets
OMRPORT_MEMINFO_NOT_AVAILABLE if the value is not available.
The system wide swappiness value specified at
/proc/sys/vm/swappiness for vm.swappiness is read.

Closes: eclipse#5237

Signed-off-by: SajinaKandy <[email protected]>
@babsingh
Copy link
Contributor

jenkins build all

@babsingh
Copy link
Contributor

babsingh commented Oct 11, 2023

The macOS failures could to be due to #6516 as the errors happen in sock_bind and sock_accept:

Correct, macOS failures are known issues (#6516); I will ignore them.

@SajinaKandy
Copy link
Contributor Author

Same failures again on macOS ( #6516 )

@babsingh babsingh merged commit 2a7cf70 into eclipse:master Oct 11, 2023
16 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants