Nothing Special   »   [go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

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

runc/libcontainer/cgroups/fs2/memory.go

Lines 105 to 114 in b137e99

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

runc/libcontainer/cgroups/fs2/memory.go

Lines 105 to 114 in b137e99

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.

@lifubang lifubang mentioned this pull request Sep 27, 2023
Closed
@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

lifubang
lifubang reviewed Oct 15, 2023
View reviewed changes
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
kolyshkin
kolyshkin approved these changes Oct 23, 2023
View reviewed changes
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

@HeRaNO
libct/cg: add swapOnlyUsage in MemoryStats
104b8dc 
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 <heran55@126.com>
@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
@harche harche mentioned this pull request Dec 4, 2023
Merged
@lifubang lifubang mentioned this pull request Dec 16, 2023
Closed
@lifubang lifubang mentioned this pull request Jan 1, 2024
Merged
@cyphar cyphar mentioned this pull request Mar 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kolyshkin kolyshkin kolyshkin approved these changes

@lifubang lifubang lifubang approved these changes

@AkihiroSuda AkihiroSuda Awaiting requested review from AkihiroSuda

@haircommander haircommander Awaiting requested review from haircommander

Assignees
No one assigned
Labels
impact/changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HeRaNO @lifubang @AkihiroSuda @haircommander @kolyshkin