core: introduce MemorySwapMax#3659
core: introduce MemorySwapMax#3659poettering merged 1 commit intosystemd:masterfrom walyong:memory_swap_limit_v02
Conversation
|
This PR is new version of #2171. |
src/core/cgroup.c
Outdated
| log_cgroup_compat(u, "Applying MemoryLimit %" PRIu64 " as MemoryMax", max); | ||
|
|
||
| swap_max = c->memory_swap_limit; | ||
|
|
There was a problem hiding this comment.
This translation doesn't make sense to me. memsw of cgroup v1 and swap of cgroup v2 have very different semantics and I don't think it'd be a good idea to translate the two back and forth this way. Given that systemd omits other aspects of memcg on cgroup v1 and that memsw is difficult to wrap one's head around, I'd recommend just implementing swap max for cgroup v2.
There was a problem hiding this comment.
I'd followed the max = c->memory_limit; for swap_max. But I'm agreeing to your opinion. Because the unified cgroup swap is operating different with legacy cgroup as menthioned at:
The combined memory+swap accounting and limiting is replaced by real
control over swap space.
https://www.kernel.org/doc/Documentation/cgroup-v2.txt
So memory_limit and memory_swap_limit have to be used only for legacy cgroup. And memory_max and memory_swap_max have to be used only for unified cgroup.
I will rework this soon.
Thanks you for comments.
There was a problem hiding this comment.
I am not sure we should add new options just for the legacy hierarchy. I think we should add these new settings only for the new hierarchy, and accept that the legacy stays the way it is...
There was a problem hiding this comment.
systemd has already two memory control options.
MemoryMax=bytes is for "memory.max" unified hierarchy control group attribute. MemoryLimit=bytes is for "memory.limit_in_bytes" legacy control group attribute.
Now, I'd like to add two memory options, MemorySwapMax= and MemorySwapLimit=. Do you want to add only MemorySwapMax= option?
Many of systems are still using legacy cgroup. So, to support legacy, I think both options are needed. Further, there are difference in memory operations between legacy and unified. At legacy, memory.memsw.limit_in_bytes is accounting and limiting memory+swap. But, at unified, memory.swap.max handles only swap.
So, I think, systemd needs both options to make a pair of corresponding memory control option.
There was a problem hiding this comment.
I think the memsw semantics is a bit too confusing especially given that it goes away with v2 and am not too sure how useful memsw limit is. So, I'd much prefer just adding the knob for v2. Thanks.
There was a problem hiding this comment.
Yes, the idea would be to only add memorySwapMax=, but leave MemorySwapLimit= out.
|
Patch looks pretty OK code-wise, but I think it would be best to follow @htejun's advice and only expose the cgroupsv2 version of this for now. Any chance you could rework the patch accordingly? |
poettering
added
cgroups
reviewed/needs-rework 🔨
|
(And sorry for taking so long to review this!) |
walyong
changed the title
|
Sorry for my late rework. I removed codes for MemorySwapLimit=. So this PR includes MemorySwapMax=only. (i.e. only for unified cgroup) According to .github/CONTRIBUTING.md:
But I can not remove |
| if (cgroup_context_has_unified_memory_config(c)) { | ||
| max = c->memory_max; | ||
| else { | ||
| swap_max = c->memory_swap_max; |
There was a problem hiding this comment.
Just a heads-up: this isn't kerneldoc. On GitHub, you mention a user named "max" with that @-notation :)
|
It prolly needs documentation updates too? |
|
looks pretty good. Misses documentation though. Could you add a short description of this to the man page, please? (don't mind the label thing: apparently only admins of the project can add/remove them. That's why I wrote "try" in that text...) |
Similar to MemoryMax=, MemorySwapMax= limits swap usage. This controls controls "memory.swap.max" attribute in unified cgroup.
4 participants
Similar to MemoryMax=, MemorySwapMax= limits swap usage. This controls
controls "memory.swap.max" attribute in unified cgroup.