Stop CPU accounting to require CPU controller #9665

ghost · 2018-07-20T08:50:16Z

In the unified cgroup hierarchy, some CPU accounting information, usage_usec, user_usec, system_usec are always available in the cpu.stat file, while the CPU controller provides additional information nr_periods, nr_throttled, throttled_usec in the cpu.stat file. This behavior is defined in https://www.kernel.org/doc/Documentation/cgroup-v2.txt. On the other hand, systemd's unit_get_cpu_usage_raw() only uses usage_usec. Therefore, there is no need to enable the CPU controller in cgroup v2 when CPU accounting is required. This change prevents the CPU accounting from enabling the CPU controller when the unified cgroup is used, and meets the half of RFE 9650.

In the unified cgroup hierarchy, some CPU accounting information, usage_usec, user_usec, system_usec are always available, while the CPU controller provides additional information nr_periods, nr_throttled, throttled_usec in the cpu.stat file. This behavior is defined in https://www.kernel.org/doc/Documentation/cgroup-v2.txt . On the other hand, systemd's unit_get_cpu_usage_raw() only uses usage_usec. Therefore, there is no need to enable the CPU controller in cgroup v2 when CPU accounting is required. This change prevents the CPU accounting from enabling the CPU controller when the unified cgroup is used.

poettering · 2018-07-20T13:42:38Z

Hmm, older kernels with cgroupsv2 didn't expose the cpu.stat file unless the CPU controller was enabled. On those we should really continue to enable the controller...

@htejun can you comment on this? What's the state there? Is it correct that on newer cgroupsv2 cpu.stat is always there regardless if "cpu" is enabled or not? Is there any nice way how we can detect kernels where it is like that?

htejun · 2018-07-20T17:33:47Z

Yeah, cpu.stat is always there regardless of cpu controller on newer kernels (v4.15+). On older kernels, cpu.stat requires cpu controller to be enabled, which depending on workloads could lead to noticeable overhead (we observe ~4% point higher system cpu time on context switch heavy workloads with 3-4 level of nesting).

poettering · 2018-07-20T18:09:07Z

@htejun hmm, is there a nice way to know whether it is there without "cpu" being enabled? i.e. i need to know if i have to enable the "cpu" controller for a cgroup, but i'd like to know that ahead of time. i mean, i could create a cgroup two levels deep and check if the file is there without enabling the controller, and then i know, but i'd prefer being able to do that without creating cgroups... i could also to a kernel version check, but we usually prefer checking for features rather then kernel versions... any suggestion?

htejun · 2018-07-23T13:02:22Z

Hmm... unfortunately, I can't think of a way to test it without creating a cgroup; however, two level deep shouldn't be necessary. If cpu controller isn't enabled at the root, checking whether the first level children have cpu.stat should be enough.

keszybz · 2018-08-02T08:03:58Z

I think a version check is acceptable in this case. In the unlikely scenario that somebody backports this feature into an older kernel, we will unnecessarily enable the controller. But this is just a small performance degradation without other problems. And creating cgroups just to check this is complicated and ugly and much more risky.

Please rework this to keep current behaviour if the kernel is older than 4.15. Something like this:

assert_se(uname(&u) >= 0);
bool need_compat = str_verscmp(u.release, "4.15") < 0;

ghost · 2018-08-03T03:08:31Z

Thank you for examining this issue. I will revise PR, hopefully on Saturday.
Note added: I will wait for the conversation at #9669 to settle and to reach to an agreement (or fail to agree)

Added kernel version check in order to see if CPU_MASK has to be enabled when CPU accounting is requested.

ghost · 2018-08-03T20:15:05Z

I updated the commit to check the kernel version as instructed.

poettering

Sorry for the late review. I have been travelling for more than a month, and am still catching up with queued up reviews. Sorry!

poettering · 2018-10-05T18:39:20Z

src/core/cgroup.c

+         * CPU controller has to be enabled for cpu.stat to appear. Let's check it.
+         * Related discussion found at PR #9665. */
+        assert_se(uname(&u) >= 0);
+        need_cpumask_for_cpu_accounting = str_verscmp(u.release, "4.15") < 0;


hmm, doing this on each call might be needlessly expensive... could you turn the above two lines into a function of its own, that uses an internal static variable to cache the result? If you need inspiration how that function should look like, please look for the definition of urlify_enabled() in the sources (as one example), which does this correctly...

poettering · 2018-10-05T18:40:05Z

src/core/cgroup.c


+        if (c->cpu_accounting) {
+                mask |= CGROUP_MASK_CPUACCT;
+                if(need_cpumask_for_cpu_accounting)


coding style nitpick: please add a space between if and the opening (

poettering · 2018-10-05T18:43:12Z

src/core/cgroup.c

+        if (c->cpu_accounting) {
+                mask |= CGROUP_MASK_CPUACCT;
+                if(need_cpumask_for_cpu_accounting)
+                        mask |= CGROUP_MASK_CPU;


hmm, on cgroupsv1 we should still add CPUACCT and CPU into the mask here as before, after all we always mount both controllers together there.

poettering · 2018-10-05T18:44:03Z

src/core/cgroup.c


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


i think this test should stay, but be bypassed if the version check holds (which is cheap if it becomes its own function as suggested above)

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

Let's close this in favour of #10507.

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).

ghost mentioned this pull request Jul 20, 2018

CPUAccounting=yes unnecessarily enables the expensive CPU controller #9650

Closed

poettering added cgroups pid1 labels Jul 20, 2018

keszybz added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Aug 2, 2018

ghost mentioned this pull request Aug 3, 2018

Various accountings are not implied by their controllers #9669

Merged

cgroup.c: Added kernel version check

28be218

Added kernel version check in order to see if CPU_MASK has to be enabled when CPU accounting is requested.

poettering requested changes Oct 5, 2018

View reviewed changes

cdown mentioned this pull request Oct 24, 2018

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

Merged

poettering closed this Oct 24, 2018

polarathene mentioned this pull request Mar 6, 2023

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

Open


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

Uh oh!

Stop CPU accounting to require CPU controller #9665

Stop CPU accounting to require CPU controller #9665

Uh oh!

Conversation

ghost commented Jul 20, 2018

Uh oh!

poettering commented Jul 20, 2018

Uh oh!

htejun commented Jul 20, 2018

Uh oh!

poettering commented Jul 20, 2018

Uh oh!

htejun commented Jul 23, 2018

Uh oh!

keszybz commented Aug 2, 2018

Uh oh!

ghost commented Aug 3, 2018 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 3, 2018

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

poettering Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

poettering Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

poettering Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

poettering Oct 5, 2018

Choose a reason for hiding this comment

Uh oh!

poettering commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

ghost commented Aug 3, 2018 •

edited by ghost

Loading