-
Notifications
You must be signed in to change notification settings - Fork 252
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
Fix swap handling for cgroups v2 #617
Conversation
Amazing work! Sorry about such a big delay with review. Couldn't you force push it to trigger CI tests? |
Thanks! Should I redo the last commit on my branch to force push? Not sure how the CI trigger works. |
* upwards in the hierarchy in case memory accounting is disabled via | ||
* cgroup.subtree_control for the given cgroup itself. | ||
*/ | ||
int ret = cgroup_walkup_to_root(ops->cgroup2_root_fd, h->fd, cgroup_rel, file, &junk_value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I briefly looked at my old helper again and while walking upwards we should verify that each component we're looking at is actually on a cgroup2 filesystem. If not, we should fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a separate patch from this series.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll prepare fix for this as a separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
easiest thing you can do to trigger CI is:
|
f7c5892
to
bf3dafc
Compare
Thanks both. Force pushed with no changes. |
Signed-off-by: Alex Hudspith <[email protected]>
On cgroups v2, there are no swap current/max files at the cgroup root, so can_use_swap must look lower in the hierarchy to determine if swap accounting is enabled. To also account for memory accounting being turned off at some level, walk the hierarchy upwards from lxcfs' own cgroup. Signed-off-by: Alex Hudspith <[email protected]> [ added check cgroup pointer is not NULL in lxcfs_init() ] Signed-off-by: Alexander Mikhalitsyn <[email protected]>
Since memory.swap.max = 0 is valid under v2, limits of 0 must not be treated differently. Instead, use UINT64_MAX as the default limit. This aligns with cgroups v1 behaviour anyway since 'limit_in_bytes' files contain a large number for unspecified limits (2^63). Resolves: lxc#534 Signed-off-by: Alex Hudspith <[email protected]>
bf3dafc
to
a6c309b
Compare
See: lxc#617 (comment) Suggested-by: Signed-off-by: Christian Brauner (Microsoft) <[email protected]> Signed-off-by: Alexander Mikhalitsyn <[email protected]>
See: lxc#617 (comment) Suggested-by: Signed-off-by: Christian Brauner (Microsoft) <[email protected]> Signed-off-by: Alexander Mikhalitsyn <[email protected]>
See: lxc#617 (comment) Suggested-by: Christian Brauner (Microsoft) <[email protected]> Signed-off-by: Alexander Mikhalitsyn <[email protected]>
This should resolve the issues with swap reporting with cgroups v2. It consists of 3 commits which are logically separated.
First PR here, feedback welcome 😃
proc_fuse: Fix get_swap_info typo swtotal == 0 -> *swtotal == 0
proc: Fix swap handling for cgroups v2 (can_use_swap)
On cgroups v2, there are no swap current/max files at the cgroup root, so
can_use_swap must look lower in the hierarchy to determine if swap accounting
is enabled. To also account for memory accounting being turned off at some
level, walk the hierarchy upwards from lxcfs' own cgroup.
proc: Fix swap handling for cgroups v2 (zero limits)
Since memory.swap.max = 0 is valid under v2, limits of 0 must not be
treated differently. Instead, use UINT64_MAX as the default limit. This aligns
with cgroups v1 behaviour anyway since 'limit_in_bytes' files contain a large
number for unspecified limits (2^63).
Resolves: #534