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

Fix swap handling for cgroups v2 #617

Merged
merged 3 commits into from
Mar 27, 2024
Merged

Conversation

alexhudspith
Copy link

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

src/proc_fuse.c Outdated Show resolved Hide resolved
@mihalicyn
Copy link
Member

Hi @alexhudspith

Amazing work! Sorry about such a big delay with review.

Couldn't you force push it to trigger CI tests?

@alexhudspith
Copy link
Author

alexhudspith commented Mar 21, 2024

Hi @alexhudspith

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);
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

@mihalicyn
Copy link
Member

Thanks! Should I redo the last commit on my branch to force push? Not sure how the CI trigger works.

easiest thing you can do to trigger CI is:

git commit --amend
# change nothing
git push -f

@alexhudspith
Copy link
Author

easiest thing you can do to trigger CI is: ...

Thanks both. Force pushed with no changes.

@alexhudspith alexhudspith reopened this Mar 21, 2024
@stgraber stgraber added this to the lxcfs-6.0.0 milestone Mar 26, 2024
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]>
@mihalicyn mihalicyn self-assigned this Mar 27, 2024
@stgraber stgraber merged commit 532c61f into lxc:main Mar 27, 2024
10 checks passed
mihalicyn added a commit to mihalicyn/lxcfs that referenced this pull request Mar 27, 2024
See:
lxc#617 (comment)

Suggested-by: Signed-off-by: Christian Brauner (Microsoft) <[email protected]>
Signed-off-by: Alexander Mikhalitsyn <[email protected]>
mihalicyn added a commit to mihalicyn/lxcfs that referenced this pull request Mar 27, 2024
See:
lxc#617 (comment)

Suggested-by: Signed-off-by: Christian Brauner (Microsoft) <[email protected]>
Signed-off-by: Alexander Mikhalitsyn <[email protected]>
mihalicyn added a commit to mihalicyn/lxcfs that referenced this pull request Mar 27, 2024
See:
lxc#617 (comment)

Suggested-by: Christian Brauner (Microsoft) <[email protected]>
Signed-off-by: Alexander Mikhalitsyn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Swap not working with cgroup2
4 participants