cgroup v2: Don't require CPU controller for CPU accounting in 4.15+ #10507

cdown · 2018-10-24T16:47:05Z

systemd only uses functions that are as of Linux 4.15+ provided
externally to the CPU controller (currently usage_usec), so if we have a
new enough kernel, we don't need to set CGROUP_MASK_CPU for
CPUAccounting=true as the CPU controller does not need to necessarily be
enabled in this case.

cdown · 2018-10-24T16:47:39Z

(This is partially based on #9665, but the author has gone AWOL.)

poettering · 2018-10-24T18:22:30Z

looks pretty ok.

One thing is missing though: in bus_cgroup_set_property() we should probably only request CGROUP_MASK_CPUACCT to be invalidated when bus_cgroup_set_boolean() is called when the CPUAccounting property is changed dynamically.

I'd also like to see a second commit in this PR which turns on global CPU accounting whenever need_cpu_controller_for_accounting() is false. i.e. in that case it's free, hence we should use it. THis would mean setting arg_default_cpu_accounting early on in main() somewhere. Would also need an update to the systemd-system.conf man page.

poettering · 2018-10-24T18:23:22Z

Also, the commit msg should somehow attribute the original author of PR #9665, i.e. @ghost for this.

poettering · 2018-10-24T18:25:10Z

The commit author of #9665 was "Ryutaroh Matsumoto".

cdown · 2018-10-24T18:31:54Z

The commit author of #9665 was "Ryutaroh Matsumoto".

Thanks, that helps. I'll make sure it's in the commit/PR message and look at the rest tomorrow.

cdown · 2018-10-25T13:09:46Z

Updated.

cdown · 2018-10-25T13:49:17Z

Tested live on my laptop in four configurations:

4.14 LTS, hybrid
4.18, hybrid
4.14 LTS, all_unified
4.18, all_unified

DefaultCPUAccounting value when looking at systemctl show looks correct on each

cdown · 2018-10-25T13:52:49Z

src/core/dbus-cgroup.c

-                return bus_cgroup_set_boolean(u, name, &c->cpu_accounting, CGROUP_MASK_CPUACCT|CGROUP_MASK_CPU, message, flags, error);
+        if (streq(name, "CPUAccounting")) {
+                CGroupMask mask = CGROUP_MASK_CPUACCT;
+                if (need_cpu_controller_for_accounting())


I'm not sure if this is correct, since this might not be local, right? If this isn't correct for that reason I'll just remove it and only set CGROUP_MASK_CPUACCT I suppose, unless you have any better ideas

why not use the cached version here?

poettering

some issues

src/core/cgroup.c

poettering · 2018-10-26T10:17:09Z

src/core/dbus-cgroup.c

-                return bus_cgroup_set_boolean(u, name, &c->cpu_accounting, CGROUP_MASK_CPUACCT|CGROUP_MASK_CPU, message, flags, error);
+        if (streq(name, "CPUAccounting")) {
+                CGroupMask mask = CGROUP_MASK_CPUACCT;
+                if (need_cpu_controller_for_accounting())


why not use the cached version here?

src/core/main.c

poettering · 2018-10-26T10:43:55Z

(and sorry for not realizing the above earlier)

cdown · 2018-10-26T15:10:02Z

Updated and tested the tristate does the right thing when manually set. Thanks for the suggestions! :-)

src/core/cgroup.c

poettering · 2018-10-26T16:27:48Z

src/core/cgroup.c

+}
+
+bool cpu_accounting_is_cheap(void) {
+        return !(get_cpu_accounting_mask() & CGROUP_MASK_CPU);


return get_cpu_accounting_mask() == 0;

It's cheap precisely when we need no controller for it at all...

poettering · 2018-10-26T16:28:49Z

src/core/cgroup.c

+        if (cgroup_context_has_cpu_weight(c) ||
            cgroup_context_has_cpu_shares(c) ||
            c->cpu_quota_per_sec_usec != USEC_INFINITY)
                mask |= CGROUP_MASK_CPUACCT | CGROUP_MASK_CPU;


actually, we might as well start being accurate here, and only set 'CGROUP_MASK_CPU` in this case...

poettering · 2018-10-26T16:31:59Z

src/core/cgroup.c


-                if ((u->cgroup_realized_mask & CGROUP_MASK_CPU) == 0)
+                if (!cpu_accounting_is_cheap() && (u->cgroup_realized_mask & CGROUP_MASK_CPU) == 0)
                        return -ENODATA;


let's move this check before the cg_all_unified() check above, in a generic fashion:

if (get_cpu_accounting_mask() & ~u->cgroup_realized_mask != 0) return -ENODATA;

The above checks whether any of the controllers we need are among the unrealized controllers. This kind of check is pretty nice as it works for all three cases the same way.

poettering · 2018-10-26T16:33:13Z

added some comments on the first patch. the second commit looks perfect!

cdown · 2018-10-26T17:39:23Z

Updated. As always, thanks for the tips :)

poettering

ok, the lack of emojis in that comment means it's not perfect, but I'll let it pass ;-)

excellent work, thanks!

poettering · 2018-10-26T17:50:44Z

src/core/cgroup.c

+         *   ║ Unified       │ nothing             │ CGROUP_MASK_CPU     ║
+         *   ╟───────────────┼─────────────────────┼─────────────────────╢
+         *   ║ Hybrid/Legacy │ CGROUP_MASK_CPUACCT │ CGROUP_MASK_CPUACCT ║
+         *   ╚═══════════════╧═════════════════════╧═════════════════════╝


beautiful! but why no emojis? ;-)

evverx · 2018-10-30T20:27:58Z

This pull request introduces 2 alerts when merging 729f8e2 into fee04d7 - view on LGTM.com

new alerts:

2 for FIXME comment

Comment posted by LGTM.com

martinpitt · 2018-10-30T20:29:32Z

@cdown: It sounds like your earlier test patch is obsolete with this?

cdown · 2018-10-31T01:10:05Z

This is ready for final review now. Two FIXMEs are added pending #10580 being cleaned up, but it doesn't make it worse than it is without this PR, since before we unconditionally used both cpuacct and cpu together anyway :-)

@cdown: It sounds like your earlier test patch is obsolete with this?

Yeah, no need for changes in debian downstream. We'll follow up on the JoinControllers shenanigans in #10580.

cdown · 2018-10-31T15:20:39Z

Just a friendly poke, this is ready for review :-)

Nothing has changed since you accepted it other than the FIXMEs being added for the JoinController bug (and thus always using both cpu and cpuacct controllers on legacy hierarchy together). CI passes.

evverx · 2018-11-02T17:00:07Z

This pull request introduces 2 alerts when merging b38ee91 into 3da1da8 - view on LGTM.com

new alerts:

2 for FIXME comment

Comment posted by LGTM.com

axionl · 2018-11-06T15:29:05Z

Has network error broken the checks?

https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-bionic-pitti-systemd-semaphore/bionic/amd64/s/systemd-upstream/20181102_162920_01e16@/log.gz

poettering · 2018-11-10T13:59:18Z

Heya, sorry for not reporting back quickly on this. We did some internal surveying inside of RH to figure out if any customers actually used the joincontroller config thingy. Turns out they do not. So I'll prepare a patch now removing the ability to configure which controllers are mounted together, and we simply hardcode that cpu + cpuacct are mounted together.

cdown · 2018-11-10T16:47:34Z

Thanks! Once you've done that, I'll rebase on top of it and go back to the FIXMEless version.

This removes the ability to configure which cgroup controllers to mount together. Instead, we'll now hardcode that "cpu" and "cpuacct" are mounted together as well as "net_cls" and "net_prio". The concept of mounting controllers together has no future as it does not exist to cgroupsv2. Moreover, the current logic is systematically broken, as revealed by the discussions in systemd#10507. Also, we surveyed Red Hat customers and couldn't find a single user of the concept (which isn't particularly surprising, as it is broken...) This reduced the (already way too complex) cgroup handling for us, since we now know whenever we make a change to a cgroup for one controller to which other controllers it applies.

… mask according to cpu/cpuacct joint mounting Note that for cgroup_context_get_mask() this doesn't actually change much, but it does prepare the ground for systemd#10507 later on.

poettering · 2018-11-15T20:25:55Z

Heya, sorry for the delay. I have now prepared #10785 which removes the JoinControllers= setting. Would be thankful for a review, and then eventuall a rebase of your work on top of it.

poettering · 2018-11-15T20:26:12Z

Oh, and I noticed that cgtop should probably be updated by your patch too

This removes the ability to configure which cgroup controllers to mount together. Instead, we'll now hardcode that "cpu" and "cpuacct" are mounted together as well as "net_cls" and "net_prio". The concept of mounting controllers together has no future as it does not exist to cgroupsv2. Moreover, the current logic is systematically broken, as revealed by the discussions in systemd#10507. Also, we surveyed Red Hat customers and couldn't find a single user of the concept (which isn't particularly surprising, as it is broken...) This reduced the (already way too complex) cgroup handling for us, since we now know whenever we make a change to a cgroup for one controller to which other controllers it applies.

… mask according to cpu/cpuacct joint mounting Note that for cgroup_context_get_mask() this doesn't actually change much, but it does prepare the ground for systemd#10507 later on.

systemd only uses functions that are as of Linux 4.15+ provided externally to the CPU controller (currently usage_usec), so if we have a new enough kernel, we don't need to set CGROUP_MASK_CPU for CPUAccounting=true as the CPU controller does not need to necessarily be enabled in this case. Part of this patch is modelled on an earlier patch by Ryutaroh Matsumoto (see PR systemd#9665).

If CPU accounting is cheap, no controller necessarily needs to be enabled here for us to be able to read statistics.

We now don't enable the CPU controller just for CPU accounting if we are on 4.15+ and using pure unified hierarchy, as this is provided externally to the CPU controller. This makes CPUAccounting=yes essentially free, so enabling it by default when it's cheap seems like a good idea.

cdown · 2018-11-18T22:26:41Z

@poettering arm64 failure is unrelated, this is good to go.

poettering · 2018-11-19T09:57:55Z

Looks excellent! Thanks!

Summary: Rebase two fixes onto the version merged upstream: systemd/systemd#10507 systemd/systemd#10567 and backport a few more: systemd/systemd#10411 systemd/systemd#10493 systemd/systemd#10757 systemd/systemd#10876 These are almost all cgroup2 related. Reviewed By: cdown Differential Revision: D13351498 fbshipit-source-id: 87c8428d48dbb0eb2ae7d34f7381fff88f83872f

poettering mentioned this pull request Oct 24, 2018

Stop CPU accounting to require CPU controller #9665

Closed

poettering added cgroups pid1 reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Oct 24, 2018

cdown mentioned this pull request Oct 24, 2018

parse: Add CategoricalBool and parser #10496

Closed

cdown force-pushed the cpu_acct branch from 98d2ba7 to db43bed Compare October 25, 2018 13:09

cdown commented Oct 25, 2018

View reviewed changes

poettering requested changes Oct 26, 2018

View reviewed changes

cdown force-pushed the cpu_acct branch from db43bed to 4b5c49d Compare October 26, 2018 15:09

cdown force-pushed the cpu_acct branch 2 times, most recently from 7097b64 to 6ab76ab Compare October 26, 2018 15:26

poettering reviewed Oct 26, 2018

View reviewed changes

src/core/cgroup.c Outdated Show resolved Hide resolved

poettering reviewed Oct 26, 2018

View reviewed changes

cdown force-pushed the cpu_acct branch from 6ab76ab to 4a93b04 Compare October 26, 2018 17:38

cdown force-pushed the cpu_acct branch from 4a93b04 to 5e06cdb Compare October 26, 2018 17:42

poettering approved these changes Oct 26, 2018

View reviewed changes

poettering added 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 Oct 26, 2018

cdown force-pushed the cpu_acct branch 2 times, most recently from 7f90bd4 to 729f8e2 Compare October 30, 2018 19:54

cdown force-pushed the cpu_acct branch from 729f8e2 to b38ee91 Compare November 2, 2018 16:27

poettering mentioned this pull request Nov 15, 2018

remove JoinControllers= setting #10785

Merged

cdown force-pushed the cpu_acct branch 2 times, most recently from 477e903 to 2c6c5a7 Compare November 17, 2018 12:13

cdown added 3 commits November 18, 2018 12:21

cgtop: Still try to get CPU statistics if controller-free

94ddb08

If CPU accounting is cheap, no controller necessarily needs to be enabled here for us to be able to read statistics.

cdown force-pushed the cpu_acct branch from 2c6c5a7 to a88c5b8 Compare November 18, 2018 12:22

poettering merged commit 2b38a8e into systemd:master Nov 19, 2018

polarathene mentioned this pull request Mar 6, 2023

Review / revisit systemd unit files docker/for-linux#73

Open

jay-mckay mentioned this pull request Aug 28, 2024

Support cgroup v2 CPU stats when controller not enabled containerd/cgroups#347

Closed

Uh oh!

cgroup v2: Don't require CPU controller for CPU accounting in 4.15+ #10507

cgroup v2: Don't require CPU controller for CPU accounting in 4.15+ #10507

Uh oh!

Conversation

cdown commented Oct 24, 2018

Uh oh!

cdown commented Oct 24, 2018

Uh oh!

poettering commented Oct 24, 2018

Uh oh!

poettering commented Oct 24, 2018

Uh oh!

poettering commented Oct 24, 2018

Uh oh!

cdown commented Oct 24, 2018

Uh oh!

cdown commented Oct 25, 2018

Uh oh!

cdown commented Oct 25, 2018

Uh oh!

cdown Oct 25, 2018 • 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!

poettering Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

poettering Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

poettering commented Oct 26, 2018

Uh oh!

cdown commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

poettering Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

poettering Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

poettering Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

poettering commented Oct 26, 2018

Uh oh!

cdown commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

poettering Oct 26, 2018

Choose a reason for hiding this comment

Uh oh!

evverx commented Oct 30, 2018

Uh oh!

martinpitt commented Oct 30, 2018

Uh oh!

cdown commented Oct 31, 2018

Uh oh!

cdown commented Oct 31, 2018

Uh oh!

evverx commented Nov 2, 2018

Uh oh!

axionl commented Nov 6, 2018

Uh oh!

poettering commented Nov 10, 2018

Uh oh!

cdown commented Nov 10, 2018

Uh oh!

poettering commented Nov 15, 2018

Uh oh!

cdown Oct 25, 2018 •

edited

Loading

cdown commented Oct 26, 2018 •

edited

Loading

cdown commented Oct 26, 2018 •

edited

Loading