libct/cg/fs2: use file + anon + swap for usage #3933

alexeldeib · 2023-07-10T13:51:46Z

This aligns v2 usage calculations more closely with v1. Current node-level reporting for v1 vs v2 on the same machine under similar load may differ by several hundred megabytes.

mem_cgroup_usage in the kernel counts
NR_FILE_PAGES + NR_ANON_MAPPED + nr_swap_pages (if swap enabled) ¹.

Using total - free results in higher "usage" numbers. This is likely due to various types of reclaimable memory technically counted as in use (e.g. inactive anon).

See also kubernetes/kubernetes#118916 for more context

https://github.com/torvalds/linux/blob/06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5/mm/memcontrol.c#L3673-L3680 ↩

alexeldeib · 2023-07-10T13:54:44Z

libcontainer/cgroups/fs2/memory.go

@@ -78,7 +78,7 @@ func setMemory(dirPath string, r *configs.Resources) error {
 	return nil
 }

-func statMemory(dirPath string, stats *cgroups.Stats) error {
+func statMemory(rootCgroupPath, dirPath string, stats *cgroups.Stats) error {


I don't love this, but it made unit testing simpler and keeps all exposed APIs unchanged.

Welcome better testing suggestions, especially some way to diff v1/v2 (either mock or e2e on real machines?)

We could make UnifiedMountpoint a variable that you can override in tests. We do this for some other stuff in the cgroup tests. The only issue is that it is publicly exposed and we probably don't want to expose allowing this to be changed by libcontainer users...

would you prefer that?

I think this signature is awkward, but could probably be better resolved by more deliberate refactoring separate from this issue, while avoiding an exported/mutable var like you mentioned.

I was digging into the FS calls to see if I could drop in a mock, but quickly realized that wouldn't be so simple 😄

It seems the only point of adding this extra argument is to make the function call statsFromMeminfo. If that's the case, we can test statsFromMeminfo directly in the unit tests.

ah good call. I left a test for pod cgroup as an "anti-test" on the root cgroup, and added a simple test for v2 root, although I hoped to avoid mocking swap calculations.

alexeldeib · 2023-07-10T16:41:28Z

libcontainer/cgroups/fs2/memory.go

+			// cgroup v1 `usage_in_bytes` reports memory usage as rss (NR_ANON_MAPPED) + cache (NR_FILE_PAGES).
+			// cgroup v2 reports the same values as anon and file.
+			// sum those to report the same value as `usage_in_bytes` in v1 for consistency.
+			stats.MemoryStats.Usage.Usage = stats.MemoryStats.Stats["anon"] + stats.MemoryStats.Stats["file"]


potentially we should add swapcached here for consistency with v1 as well...

I don't think we should emulate the what cgroupv1 gives you for the cgroupv2 (and only for the root case too) -- in general in OCI we just give you what the kernel provides. I suspect emulating cgroupv2 more correctly (which is what you seem to be doing here? I need to look into how the kernel does the accounting...).

We have to do cgroupv1 -> cgroupv2 mapping for writing to cgroupfs (because otherwise old configs wouldn't work on newer systems) but faking cgroupv1 stats output for cgroupv2 seems particularly odd to me.

@kolyshkin wdyt?

hmm. my original intention was to match usage_in_bytes which I think is NR_FILE_PAGES + NR_ANON_MAPPED (+ NR_SWAPCACHE) for the root cgroup. I'm not sure what the equivalent of memory.current would be for the root cgroup in v2 if not something like the v1 estimate? I admit I don't know specifics of how the per-cg usage counter charges.

I guess part of the problem is "usage" is poorly defined vs. specific zones/memory types? part of my read was that runc -> cadvisor -> k8s basically aligned on "usage" == NR_FILE_PAGES + NR_ANON_MAPPED, and "working set" == "usage" - "inactive_file". which has issues, but is at least consistent in v1 + v2 short of renamed statistics.

I can definitely say total - free hasn't been received well, but welcome alternatives 😄

@kolyshkin any thoughts on this one?

As described in https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/cgroup-v1/memory.rst?plain=1#L625-L633 , I think maybe we should add swapcached here.
So if we want to merge this, we should make a decision for this question.

I agree with adding swapcached here. One reservation I had -- total - free for swap also does not equal swapcached for the root, but if you look at specific slices, memory.swap.current for a given cgroup doesn't equal memory.stat.swapcached anyway. I suppose it makes sense -- swap area can be used or not free in different ways (?), part of the swap space may not be writable (not the case for my demo however).

we also deliberately add memory + swap usage to match v1 later -

// As cgroup v1 reports SwapUsage values as mem+swap combined, // while in cgroup v2 swap values do not include memory, // report combined mem+swap for v1 compatibility. swapUsage.Usage += memoryUsage.Usage if swapUsage.Limit != math.MaxUint64 { swapUsage.Limit += memoryUsage.Limit } stats.MemoryStats.SwapUsage = swapUsage

should potentially make the same adjustment

added swapcached, but what do you think about the swap adjustment?

I think we should add memory usage for consistency, e.g. you can see consumers expect it today https://github.com/google/cadvisor/blob/8164b38067246b36c773204f154604e2a1c962dc/container/libcontainer/handler.go#L808

but i'm not sure if we should stick with total - free for swap usage, use root swapcached, or something else?

should potentially make the same adjustment

I think so, maybe this is a bug before.
When we fall though to use statsFromMeminfo to calc node's memory data, we return in here, so report combined mem+swap for v1 compatibility is missing.
Please see:
https://github.com/opencontainers/runc/blob/main/libcontainer/cgroups/fs2/memory.go#L105-L126

@kolyshkin Am I right?

think that might be better?

notably, I think we want nr_swap_pages from the kernel, but we only get proxies to that -
https://github.com/torvalds/linux/blob/5d0c230f1de8c7515b6567d9afba1f196fb4e2f4/mm/memcontrol.c#L3677-L3680
https://github.com/torvalds/linux/blob/5d0c230f1de8c7515b6567d9afba1f196fb4e2f4/mm/swapfile.c#L3257-L3258

wonder if we have the same "overcounting" for swap or not

mrunalp · 2023-07-11T17:08:38Z

@kolyshkin @cyphar ptal

AkihiroSuda · 2023-07-16T12:39:51Z

This is likely to break compatibility, so I'm not sure we can merge this.

AkihiroSuda

#3933 (comment)

alexeldeib · 2023-07-16T13:29:22Z

@AkihiroSuda could you elaborate on the problem?

Current code causes major pain as a behavioral change to downstream consumers for v1 vs v2. I would argue compatibility has already been broken, and this restores previous behavior.

Can you clarify what you mean? Who breaks here with this change and in what way?

AkihiroSuda · 2023-07-16T13:38:27Z

@AkihiroSuda could you elaborate on the problem?

Current code causes major pain as a behavioral change to downstream consumers for v1 vs v2. I would argue compatibility has already been broken, and this restores previous behavior.

Can you clarify what you mean? Who breaks here with this change and in what way?

There might be third party programs that are already depending on the existing output for v2.

alexeldeib · 2023-07-16T13:42:42Z

I can understand that this is a change to v2 consumers. I am not sure I see how it leads to breakage. You’d simply see a lower number for usage? The contract is unchanged.

Alternative changes up the stack would lead to runc producing different stats for identical memory usage under v1 and v2, but cadvisor and Kubernetes would fudge it using the same calculation as this PR.

It seems to me “more correct” to fix the actual meaning of usage. But if there is concern about this approach, then I would start the discussion at cadvisor level or potentially purely in k/k.

The same argument you’re suggesting applies to cadvisor and k/k too, but at least Kubernetes folks seem to agree this is a bug for v1/v2. So I hope folks at some layer are willing to adapt and recognize the calculation is wildly different.

Ps: what layer would you suggest the change occurs at? Or do you disagree there’s even an issue?

alexeldeib · 2023-07-16T14:25:34Z

Reframing. I can understand that changing these numbers with no API change has an effect on users. That’s exactly the scenario I hope to “fix” and the one which runc “broke” with the existing calculation. What I don’t understand, is what then runc’s contract actually is. Is it implementation defined by “whatever we did first”? That leaves little room for bug fixes. Is it formally defined — “usage is rss + cache”? Then this PR makes sense and repairs an existing API violation, regardless of existing behavior. I don’t see a formal definition of this API. Cadvisor and Kubernetes strike me as rather large 3p users as you put it. I would be curious where runc puts the balance of practical usage and behavioral consistency against “bug compatibility” and what you think this "correct" here ignoring potential compat breakage (and what that means we should do here?). There is plenty of opportunity up the stack for fudging. However IMO it’s extremely misleading machines with comparable usage will report differently whether they are v1 or v2. If runc doesn’t consider than a bug, okay I suppose, but I’d like to understand clearly for future reference when we can or cannot make corrective behavioral changes like this and what runc’s actual guarantees are. It’s a bit disappointing to me something like this would differ from v1 since day 1 and may remain baked in — future 3p users would have to know about this and fudge their logic too if they care about getting the ”correct” data, otherwise they will perceive they lose memory when they upgrade to v2.

…

On Sun, Jul 16, 2023 at 9:38 AM Akihiro Suda ***@***.***> wrote: @AkihiroSuda <https://github.com/AkihiroSuda> could you elaborate on the problem? Current code causes major pain as a behavioral change to downstream consumers for v1 vs v2. I would argue compatibility has *already* been broken, and this restores previous behavior. Can you clarify what you mean? Who breaks here with this change and in what way? There might be third party programs that are already depending on the existing output for v2. — Reply to this email directly, view it on GitHub <#3933 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABT4LWLCWMUCJPNQUKX4HNLXQPVF5ANCNFSM6AAAAAA2ESQEVI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

mrunalp · 2023-07-25T16:45:30Z

I can understand that this is a change to v2 consumers. I am not sure I see how it leads to breakage. You’d simply see a lower number for usage? The contract is unchanged.

I agree. I am not seeing how this would be breaking if we are presenting a more accurate view of usage in line with v1.

AkihiroSuda · 2023-07-25T16:52:26Z

I'm ok to merge this, but if something breaks we may have to revert this and maybe re-add it as another field like "Usage2".

#3933 (comment)

AkihiroSuda · 2023-07-25T16:53:58Z

Could you update https://github.com/opencontainers/runc/blob/main/CHANGELOG.md to explain the change?

alexeldeib · 2023-07-26T15:22:59Z

Could you update https://github.com/opencontainers/runc/blob/main/CHANGELOG.md to explain the change?

Wasn't sure, if I should mark this as "unreleased" or create a new unreleased 1.1.9 tag? marked it as "unreleased - change"

CHANGELOG.md

kolyshkin · 2023-07-28T20:58:24Z

I agree with the general direction of this PR; thank you for your contribution @alexeldeib.

Just a few notes:

The logic of getting the root cgroup stats should better be contained in statsFromMeminfo, not spread between the two as it is now.
Maybe instead adding an extra argument to statMemory for the sake of unit test, we can test statsFromMeminfo directly? This will make the commit less invasive.

While at it, it might make sense to rename statsFromMeminfo to e.g. rootStatsFromMeminfo so it is more clear it is only used for root cgroup.

kolyshkin · 2023-08-01T00:38:14Z

@alexeldeib needs a rebase

alexeldeib · 2023-08-02T13:06:06Z

I think that might be better?

swap confused me a bit, but I think I get it. I do not think we should count swapcached.

v1 has usage_in_bytes from mem_cgroup_read_u64 with MEMFILE_PRIVATE(_MEM, RES_USAGE)
https://github.com/torvalds/linux/blob/5d0c230f1de8c7515b6567d9afba1f196fb4e2f4/mm/memcontrol.c#L5016-L5020

In this case we use memcg->memory not memcg->memsw (which is v1 only? vs memcg->swap for v2 only?)
https://github.com/torvalds/linux/blob/5d0c230f1de8c7515b6567d9afba1f196fb4e2f4/mm/memcontrol.c#L3705-L3706

which would mean we actually call mem_cgroup_usage with swap = false
https://github.com/torvalds/linux/blob/5d0c230f1de8c7515b6567d9afba1f196fb4e2f4/mm/memcontrol.c#L3722-L3724

which makes sense semantically for us anyway - if usage were already usage with swap, then summing swap + usage for swap would be silly.

v2 has current with memory_current_read which seems to just read memcg->memory
https://github.com/torvalds/linux/blob/5d0c230f1de8c7515b6567d9afba1f196fb4e2f4/mm/memcontrol.c#L6714-L6715

so v2 is consistent at least there

This aligns v2 usage calculations more closely with v1. Current node-level reporting for v1 vs v2 on the same machine under similar load may differ by ~250-750Mi. Also return usage as combined swap + memory usage, aligned with v1 and non-root v2 cgroups. `mem_cgroup_usage` in the kernel counts NR_FILE_PAGES + NR_ANON_MAPPED + `nr_swap_pages` (if swap enabled) [^0]. Using total - free results in higher "usage" numbers. This is likely due to various types of reclaimable memory technically counted as in use (e.g. inactive anon). See also kubernetes/kubernetes#118916 for more context [^0]: https://github.com/torvalds/linux/blob/06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5/mm/memcontrol.c#L3673-L3680 Signed-off-by: Alexander Eldeib <alexeldeib@gmail.com>

kolyshkin

LGTM

addressed

kolyshkin · 2023-08-03T05:30:09Z

@lifubang it looks like your comments were addressed, so I dismissed your old "request changes" review.

linxiulei · 2023-10-19T14:29:50Z

I think this may have brought in inconsistency issue for cgroupv2. AIUI, there is memory account difference by design between cgroup v1 and cgroup v2 (please correct me if I'm wrong):

v1 accounts file + anon + swap
v2 accounts file + anon + swap + in-kernel data

So it's WAI that v2 shows higher memory usage for non-root cgroups, which should be applied to the root as well. But this PR made the root account memory usage as v1, which appears inconsistency to me.

bobbypage · 2023-11-28T20:19:54Z

cc @kolyshkin @haircommander @mrunalp it would be great to get your thoughts on #3933 (comment).

I'm a little concerned that after this change we are no longer properly accounting kernel memory in cgroupv2 as @linxiulei mentioned.

ace-cohere · 2023-11-30T03:45:19Z

it's WAI that v2 shows higher memory usage for non-root cgroups, which should be applied to the root as well.

what would the right math be in your mind? do you have a source for the specific v2 non-root calculation? I'd browsed but didn't find anything that would indicate this inconsistency (not at all a kernel expert, had to learn some things to send this PR).

the original issue was total - free was not reflective of what we called "usage" in k8s-land. you can see several examples of the cgroup and proc info stats in the linked k/k issue to get a sense what the right numbers should be and whether it's a pure calculation or OS issue.

It's not clear to me what the right math would be if we need to add something other than rss + cache. I tried a few approaches but the numbers didn't quite add up (see the comments in k/k). there's active debate over what should be counted, but in practice this PR seemed to do the right thing when I tested it with some load. Have you found a counterexample?

is there a specific type of memory you suspect is not properly accounted? inactive_anon/file were where I'd looked previously, not sure what you're thinking.

linxiulei · 2023-11-30T11:39:11Z

I assume "what we called usage in k8s-land" is what we collect from memory.current in v2, which I believe is more than rss + cache. One specific example is TCP memory (see https://lpc.events/event/16/contributions/1212/attachments/1079/2052/LPC%202022%20-%20TCP%20memory%20isolation.pdf). The kernel doc suggests the kmem is accumulated into memory.current (main counter).

I don't think memory.current accounts all types of kernel memory such as memory page tables, device drivers etc. But I do believe a visible amount of memory is not accounted in rss + cache, which is essentially userspace memory. Other example I could think of, but without solid evidence, is dcache. Maybe ithis](https://android.googlesource.com/kernel/msm/+/android-msm-3.9-usb-and-mmc-hacks/Documentation/cgroups/memory.txt#298) is a good list of kernel memory types we miss despite the doc is for v1

haircommander · 2024-06-07T16:06:46Z

I think the key here is to emulate what was previously done in cgroupv1. I think the initial investigation done by @alexeldeib is correct: if we look at the calculate of memory usage, we can see v1 uses memory.usage_in_bytes, even for the root cgroup. In the root cgroup case, we can see this is built from anon + file, and so the best approximation for cgroupv1 usage_in_bytes of the root cgroup on v2 is to just use file + anon

alexeldeib commented Jul 10, 2023

View reviewed changes

alexeldeib force-pushed the ace/v2root branch 3 times, most recently from 80b6fb0 to ff695e9 Compare July 10, 2023 14:11

alexeldeib commented Jul 10, 2023

View reviewed changes

alexeldeib force-pushed the ace/v2root branch from ff695e9 to 3e1d2cc Compare July 11, 2023 14:50

alexeldeib force-pushed the ace/v2root branch from 3e1d2cc to f7da113 Compare July 14, 2023 16:37

AkihiroSuda previously requested changes Jul 16, 2023

View reviewed changes

This was referenced Jul 16, 2023

Node memory usage on cgroupv2 reported higher than cgroupv1 kubernetes/kubernetes#118916

Open

fix: align node memory usage for cgroup v1 and v2 kubernetes/kubernetes#119356

Closed

AkihiroSuda added the impact/changelog label Jul 25, 2023

alexeldeib force-pushed the ace/v2root branch from f7da113 to cd20efa Compare July 26, 2023 15:23

AkihiroSuda reviewed Jul 26, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

alexeldeib force-pushed the ace/v2root branch 2 times, most recently from f1b45c7 to e92b917 Compare July 26, 2023 15:41

AkihiroSuda approved these changes Jul 26, 2023

View reviewed changes

AkihiroSuda requested review from cyphar and kolyshkin July 26, 2023 15:44

AkihiroSuda added the area/cgroupv2 label Jul 26, 2023

alexeldeib force-pushed the ace/v2root branch 3 times, most recently from b86d3f9 to 92f1e3a Compare August 2, 2023 11:38

alexeldeib changed the title ~~libct/cg/fs2: use file + anon for usage~~ libct/cg/fs2: use file + anon + swap for usage Aug 2, 2023

alexeldeib force-pushed the ace/v2root branch 4 times, most recently from 19f95b1 to 884a7aa Compare August 2, 2023 12:55

lifubang added the status/4-merge label Aug 2, 2023

alexeldeib force-pushed the ace/v2root branch from 884a7aa to 7d2becd Compare August 2, 2023 13:18

kolyshkin approved these changes Aug 3, 2023

View reviewed changes

kolyshkin merged commit 2da710c into opencontainers:main Aug 3, 2023
36 checks passed

kolyshkin mentioned this pull request Aug 9, 2023

[1.1] libct/cg/fs2: use file + anon + swap for usage #3977

Merged

kolyshkin added the backport/1.1-done A PR in main branch which has been backported to release-1.1 label Aug 9, 2023

kolyshkin mentioned this pull request Aug 10, 2023

runc 1.1.9 -- "There is a crack in everything. That's how the light gets in." #3981

Merged

haircommander mentioned this pull request Aug 28, 2023

v0.47: vendor: bump runc to 1.1.9 google/cadvisor#3383

Merged

cyphar mentioned this pull request Mar 14, 2024

release 1.2.0-rc.1 #4221

Merged

naemono mentioned this pull request May 22, 2024

Bump Kibana memory only for 8.1.x in e2e tests. elastic/cloud-on-k8s#7836

Merged

harche mentioned this pull request Jun 3, 2024

Move cgroup v1 support into maintenance mode kubernetes/enhancements#4572

Merged

libct/cg/fs2: use file + anon + swap for usage #3933

libct/cg/fs2: use file + anon + swap for usage #3933

Conversation

alexeldeib commented Jul 10, 2023 • edited Loading

Footnotes

alexeldeib Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

alexeldeib Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp commented Jul 11, 2023

AkihiroSuda commented Jul 16, 2023

AkihiroSuda left a comment

Choose a reason for hiding this comment

alexeldeib commented Jul 16, 2023 • edited Loading

AkihiroSuda commented Jul 16, 2023

alexeldeib commented Jul 16, 2023 • edited Loading

alexeldeib commented Jul 16, 2023 via email • edited Loading

mrunalp commented Jul 25, 2023 • edited Loading

AkihiroSuda commented Jul 25, 2023 • edited Loading

AkihiroSuda commented Jul 25, 2023

alexeldeib commented Jul 26, 2023

kolyshkin commented Jul 28, 2023

kolyshkin commented Aug 1, 2023

alexeldeib commented Aug 2, 2023 • edited Loading

kolyshkin left a comment

Choose a reason for hiding this comment

kolyshkin commented Aug 3, 2023

linxiulei commented Oct 19, 2023

bobbypage commented Nov 28, 2023

ace-cohere commented Nov 30, 2023

linxiulei commented Nov 30, 2023

haircommander commented Jun 7, 2024

alexeldeib commented Jul 10, 2023 •

edited

Loading

alexeldeib Jul 10, 2023 •

edited

Loading

cyphar Jul 12, 2023 •

edited

Loading

alexeldeib Jul 12, 2023 •

edited

Loading

alexeldeib commented Jul 16, 2023 •

edited

Loading

alexeldeib commented Jul 16, 2023 •

edited

Loading

alexeldeib commented Jul 16, 2023 via email •

edited

Loading

mrunalp commented Jul 25, 2023 •

edited

Loading

AkihiroSuda commented Jul 25, 2023 •

edited

Loading

alexeldeib commented Aug 2, 2023 •

edited

Loading