core/cgroup: various fixes for accounting by YHNdnzj · Pull Request #33258 · systemd/systemd

YHNdnzj · 2024-06-09T13:49:21Z

Fixes #33149
Replaces #33145
Closes #33145

YHNdnzj · 2024-06-10T16:44:55Z

cc @michaelolbrich @HeRaNO Could you please give this PR a test?

HeRaNO · 2024-06-11T02:33:18Z

cc @michaelolbrich @HeRaNO Could you please give this PR a test?

I'm so sorry that recently I won't have much time working on this. I will give it a test when I have time, maybe within next two weeks.

src/core/cgroup.c

rpigott

👍 works for me

HeRaNO · 2024-06-15T14:21:10Z

I'm working on testing today but I didn't manage to run a testing qemu with mkosi (24~devel). The terminal quits unexpectedly quite often (on Ubuntu 24.04) and wastes me a lot of time. Please ignore me if you're sure that the issue is solved. I'm trying hard to start the testing qemu. T T

michaelolbrich · 2024-06-15T18:39:20Z

Works as expected now.

daandemeyer · 2024-06-15T19:43:13Z

I'm working on testing today but I didn't manage to run a testing qemu with mkosi (24~devel). The terminal quits unexpectedly quite often (on Ubuntu 24.04) and wastes me a lot of time. Please ignore me if you're sure that the issue is solved. I'm trying hard to start the testing qemu. T T

Can you open an issue on the mkosi repository with what you are seeing?

Note that apparmor is broken on ubuntu and will need to be disabled to run mkosi with a swtpm as is done in the systemd repo.

HeRaNO · 2024-06-16T05:40:10Z

I'm working on testing today but I didn't manage to run a testing qemu with mkosi (24~devel). The terminal quits unexpectedly quite often (on Ubuntu 24.04) and wastes me a lot of time. Please ignore me if you're sure that the issue is solved. I'm trying hard to start the testing qemu. T T

Can you open an issue on the mkosi repository with what you are seeing?

Note that apparmor is broken on ubuntu and will need to be disabled to run mkosi with a swtpm as is done in the systemd repo.

Hi. I mainly follow the guide on Hacking on systemd. But I'm afraid I can't provide useful information for you because the terminal just disappeared while running the steps (also I'm not clear on what step the terminal disappeared. Thing I can only do is open another terminal and rerun the build).

Here are my environment and steps:

OS: Ubuntu 24.04 (6.8.0-35-generic) on VirtualBox 7.0.18 (nested VT-x/AMD-V on)
Step:
- Workaround for bwrap issue
- Install reprepro (sudo apt install reprepro). It will fail on building image stage without the package installed.
- Clone the mkosi and systemd repo, and setup mkosi.
- cd into systemd and generate key (sudo mkosi genkey)
- Create mkosi.local.conf and add the following configure:
```
[Output]
Format=disk
[Host]
QemuFirmware=uefi
```
- Run sudo mkosi -f qemu and wait. Because newuidmap and newgidmap will fail to execute without sudo.

I'm afraid the problem is with me because I didn't disable Apparmor (only doing workarounds). Please point them out if I've made any other mistakes. Thanks.

YHNdnzj · 2024-06-19T17:20:13Z

@poettering Could you please take a look? Currently the accounting output for systemd-run is completely non-functional.

intelfx · 2024-06-19T18:54:38Z

It would seem that systemd-run v256, even with this merged, no longer prints IP traffic stats even when IPAccounting=yes by default. Is this expected?

intelfx · 2024-06-19T18:59:23Z

src/core/cgroup.c

        if (r < 0)
-                return log_unit_debug_errno(u, r, "Failed to determine whether cgroup %s is empty, ignoring: %m", empty_to_root(crt->cgroup_path));
-
+                log_unit_debug_errno(u, r, "Failed to determine whether cgroup %s is empty: %m", empty_to_root(crt->cgroup_path));


I liked the , ignoring suffix. In general I like that systemd tries to always tell what is its reaction to any error condition that's happening; might make sense to keep it or there's something I'm missing?

But here the error isn't ignored, as we return it anyway. Also I doubt that ", ignoring" here really tells you anything?

Yes, it's an unfortunate bleed-through of semantics from the function's (only?) caller. It is moderately useful, though, as it tells me that as a result of this error, an action will be skipped, the system will continue to function, but with some cruft potentially left over.

I have never encountered this exact error, but similar error messages in the past were quite helpful. I'm mainly administering "pets", not "cattle" though, so I might be slightly biased.

YHNdnzj · 2024-06-19T19:13:25Z

It would seem that systemd-run v256, even with this merged, no longer prints IP traffic stats even when IPAccounting=yes by default. Is this expected?

Indeed. Will update the patch,

YHNdnzj · 2024-06-20T21:19:09Z

@intelfx Could you test IP accounting again?

intelfx · 2024-06-22T11:09:18Z

@YHNdnzj yup, can confirm it works for me, thanks!

keszybz

LGTM. The code is complicated… so it's hard to say that all the changes are correct, but just looking at the patches, it looks all good, and the reports from testing are also good.

src/core/bpf-firewall.c

…r freezer The same check is used everywhere else.

No functional change, just deduplicate default values in cgroup_runtime_free() and remove pointless call in unit_free() (at the time it's called the CGRuntime has been destroyed already).

Since the introduction of CGroupRuntime, there's no need to call *_reset_accounting in unit_new(), hence make those static. While at it, refrain from hardcoding default values in cgroup_runtime_new(), but call the corresponding funcs. This also corrects the default value of io_accounting_base. Fixes systemd#33482

If cgroup is already gone, i.e. CGRuntime.cgroup_path is NULL, do not return -ENODATA prematurely, but check for cached values first. For systemd#33149

Fixes systemd#33149 Replaces systemd#33145

Currently, unit_setup_cgroup_runtime() is called in various _coldplug() functions if the unit is not inactive. That seems unnecessary though, and kinda defeats the purpose of CGroupRuntime. If we need to fork off a process for the unit or got something during deserialization, the CGroupRuntime would be automatically set up by unit_prepare_exec() / cgroup_runtime_deserialize_one(). Otherwise it would mean the cgroup doesn't exist and we don't need to allocate that in the first place. Plus, note that socket units might also carry a cgroup with ExecStartPre=/ExecStartPost=/... Hence the existing code is really inconsistent.

daandemeyer · 2024-07-19T13:45:48Z

I backported the 2nd, 4th, 6th, 7th, 8th and 10th commits

YHNdnzj added the pid1 label Jun 9, 2024

YHNdnzj added this to the v257 milestone Jun 9, 2024

github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Jun 9, 2024

YHNdnzj requested a review from poettering June 9, 2024 13:50

This comment was marked as off-topic.

Sign in to view

YHNdnzj force-pushed the cg-runtime-accounting branch 2 times, most recently from 2750516 to 41a562b Compare June 9, 2024 13:57

YHNdnzj added the needs-stable-backport label Jun 12, 2024

michaelolbrich reviewed Jun 13, 2024

View reviewed changes

src/core/cgroup.c Show resolved Hide resolved

src/core/cgroup.c Show resolved Hide resolved

YHNdnzj added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jun 13, 2024

YHNdnzj mentioned this pull request Jun 14, 2024

systemd-run resource accounting missing since v256 #33348

Closed

rpigott approved these changes Jun 14, 2024

View reviewed changes

YHNdnzj force-pushed the cg-runtime-accounting branch from 41a562b to 23795ea Compare June 15, 2024 11:12

github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jun 15, 2024

YHNdnzj force-pushed the cg-runtime-accounting branch from 23795ea to 8db3cb4 Compare June 15, 2024 11:15

YHNdnzj mentioned this pull request Jun 18, 2024

tmpfiles: make --purge more restrictive, and various other tweaks #33383

Merged

intelfx reviewed Jun 19, 2024

View reviewed changes

YHNdnzj force-pushed the cg-runtime-accounting branch from 8db3cb4 to 052ba23 Compare June 19, 2024 19:17

YHNdnzj mentioned this pull request Jun 25, 2024

IOAccounting values not reported correctly in systemctl status #33482

Closed

YHNdnzj linked an issue Jun 25, 2024 that may be closed by this pull request

IOAccounting values not reported correctly in systemctl status #33482

Closed

keszybz approved these changes Jun 28, 2024

View reviewed changes

src/core/bpf-firewall.c Show resolved Hide resolved

keszybz added good-to-merge/with-minor-suggestions and removed please-review PR is ready for (re-)review by a maintainer labels Jun 28, 2024

YHNdnzj added 12 commits June 28, 2024 15:38

core/cgroup: correct macro alignment

38f3b73

core/cgroup: use > 0 comparison rather than == 1

8153be9

core/cgroup: check CGroupRuntime.cgroup_path rather than _realized fo…

c97c79a

…r freezer The same check is used everywhere else.

core/cgroup: make unit_has_host_root_cgroup take const Unit*

4442aef

core/cgroup: call bpf_firewall_close in cgroup_runtime_free

5a8c2c9

No functional change, just deduplicate default values in cgroup_runtime_free() and remove pointless call in unit_free() (at the time it's called the CGRuntime has been destroyed already).

core/cgroup: check root cgroup earlier for unit_get_memory_accounting

d3d0353

core/cgroup: actually make use of the cached accounting values

3849d1f

If cgroup is already gone, i.e. CGRuntime.cgroup_path is NULL, do not return -ENODATA prematurely, but check for cached values first. For systemd#33149

core/cgroup: drop spurious ", ignoring" for unit_cgroup_is_empty

4c1fc52

core: do not drop CGroupRuntime when unit stops, but only on GC

4918f14

Fixes systemd#33149 Replaces systemd#33145

core: cast ignored retval of unit_realize_cgroup to void

8d178f7

YHNdnzj force-pushed the cg-runtime-accounting branch from 052ba23 to f26b2ec Compare June 28, 2024 13:44

YHNdnzj added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Jun 28, 2024

YHNdnzj merged commit 7fee3dd into systemd:main Jun 29, 2024

YHNdnzj deleted the cg-runtime-accounting branch June 29, 2024 14:11

github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jun 29, 2024

philiptaron mentioned this pull request Jul 13, 2024

systemd: 255.9 -> 256.2 NixOS/nixpkgs#307068

Merged

13 tasks

daandemeyer removed the needs-stable-backport label Jul 19, 2024

yuwata mentioned this pull request Aug 16, 2024

systemd-run has inconsistent behaviour on memory accounting #33191

Closed

waldheinz mentioned this pull request Nov 11, 2024

systemd leaks file descriptors NixOS/nixpkgs#355205

Open

yuwata mentioned this pull request Jul 27, 2025

[v256] core: fix freeze regression #38365

Merged

Uh oh!

Comments

Conversation

YHNdnzj commented Jun 9, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

YHNdnzj commented Jun 10, 2024

Uh oh!

HeRaNO commented Jun 11, 2024

Uh oh!

Uh oh!

Uh oh!

rpigott left a comment

Choose a reason for hiding this comment

Uh oh!

HeRaNO commented Jun 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelolbrich commented Jun 15, 2024

Uh oh!

daandemeyer commented Jun 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HeRaNO commented Jun 16, 2024

Uh oh!

YHNdnzj commented Jun 19, 2024

Uh oh!

intelfx commented Jun 19, 2024

Uh oh!

intelfx Jun 19, 2024

Choose a reason for hiding this comment

Uh oh!

YHNdnzj Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

intelfx Jun 19, 2024

Choose a reason for hiding this comment

Uh oh!

YHNdnzj commented Jun 19, 2024

Uh oh!

YHNdnzj commented Jun 20, 2024

Uh oh!

intelfx commented Jun 22, 2024

Uh oh!

keszybz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daandemeyer commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

YHNdnzj commented Jun 9, 2024 •

edited by github-actions bot

Loading

HeRaNO commented Jun 15, 2024 •

edited

Loading

daandemeyer commented Jun 15, 2024 •

edited

Loading

YHNdnzj Jun 19, 2024 •

edited

Loading

daandemeyer commented Jul 19, 2024 •

edited

Loading