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

feat: add swapOnlyUsage in MemoryStats #4010

Merged
merged 1 commit into from
Oct 25, 2023
Merged

feat: add swapOnlyUsage in MemoryStats #4010

merged 1 commit into from
Oct 25, 2023

Conversation

HeRaNO
Copy link
Contributor

@HeRaNO HeRaNO commented Sep 6, 2023

implement #4000

@lifubang
Copy link
Member

lifubang commented Sep 9, 2023

Thanks for your contribution. Please use git rebase main to catch up with the latest update, instead of merge main.

@HeRaNO
Copy link
Contributor Author

HeRaNO commented Sep 9, 2023

Thanks for your contribution. Please use git rebase main to catch up with the latest update, instead of merge main.

done

@lifubang
Copy link
Member

lifubang commented Sep 9, 2023

And could you see rootStatsFromMeminfo in

memoryUsage, err := getMemoryDataV2(dirPath, "")
if err != nil {
if errors.Is(err, unix.ENOENT) && dirPath == UnifiedMountpoint {
// The root cgroup does not have memory.{current,max}
// so emulate those using data from /proc/meminfo and
// /sys/fs/cgroup/memory.stat
return rootStatsFromMeminfo(stats)
}
return err
}

  1. Do you know whether .pead exist in UnifiedMountpoint or not?
  2. If not, could you know how to get max_usage_in_bytes from /proc/meminfo?

@HeRaNO
Copy link
Contributor Author

HeRaNO commented Sep 9, 2023

And could you see rootStatsFromMeminfo in

memoryUsage, err := getMemoryDataV2(dirPath, "")
if err != nil {
if errors.Is(err, unix.ENOENT) && dirPath == UnifiedMountpoint {
// The root cgroup does not have memory.{current,max}
// so emulate those using data from /proc/meminfo and
// /sys/fs/cgroup/memory.stat
return rootStatsFromMeminfo(stats)
}
return err
}

  1. Do you know whether .peak exist in UnifiedMountpoint or not?
  2. If not, could you know how to get max_usage_in_bytes from /proc/meminfo?
  1. No.
  2. It seems that /proc/meminfo doesn't export any usage of memory watermark, so It's hard to get max_usage_in_bytes.

@HeRaNO HeRaNO requested a review from lifubang September 9, 2023 03:48
@AkihiroSuda
Copy link
Member

Can we have a test?

@HeRaNO
Copy link
Contributor Author

HeRaNO commented Sep 9, 2023

Can we have a test?

Sure, added.

@HeRaNO HeRaNO requested a review from lifubang October 6, 2023 12:47
@HeRaNO HeRaNO requested a review from lifubang October 9, 2023 02:15
@HeRaNO
Copy link
Contributor Author

HeRaNO commented Oct 13, 2023

@haircommander @lifubang @AkihiroSuda PTAL, thx

@AkihiroSuda
Copy link
Member

Please squash commits

@AkihiroSuda AkihiroSuda requested review from kolyshkin and haircommander and removed request for haircommander October 13, 2023 01:54
@haircommander
Copy link
Contributor

LGTM

@HeRaNO
Copy link
Contributor Author

HeRaNO commented Oct 14, 2023

@kolyshkin PTAL, thx

Copy link
Member

@lifubang lifubang left a comment

Choose a reason for hiding this comment

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

It seems that we have accepted the ‘swapOnlyUsage’ approach, so please add some descriptions in ‘libcontainer/README.md’ to make each memory stat field more clear to the users.

@HeRaNO HeRaNO changed the title feat: use memory.peak and memory.swap.peak in cgroups v2 feat: add swapOnlyUsage in MemoryStats Oct 15, 2023
@HeRaNO
Copy link
Contributor Author

HeRaNO commented Oct 15, 2023

It seems that we have accepted the ‘swapOnlyUsage’ approach, so please add some descriptions in ‘libcontainer/README.md’ to make each memory stat field more clear to the users.

I've added some explanation in CHANGELOG.md.

@HeRaNO HeRaNO requested a review from lifubang October 15, 2023 04:23
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Seems reasonable. LGTM

This field reports swap-only usage. For cgroupv1, `Usage` and `Failcnt`
are set by subtracting memory usage from memory+swap usage. For cgroupv2,
`Usage`, `Limit`, and `MaxUsage` are set. This commit also export `MaxUsage`
of memory under cgroupv2 mode, using `memory.peak` introduced in kernel 5.19.

Signed-off-by: Heran Yang <[email protected]>
@HeRaNO
Copy link
Contributor Author

HeRaNO commented Oct 25, 2023

All checks pass now. Ready to merge.

@lifubang lifubang merged commit edd00eb into opencontainers:main Oct 25, 2023
45 checks passed
@lifubang lifubang mentioned this pull request Jan 1, 2024
@cyphar cyphar mentioned this pull request Mar 14, 2024
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.

5 participants