Various accountings are not implied by their controllers

[ghost]

The original manpage says Implies BBBAccounting=true many times but actually that accounting is not implied by the respective resource control in v239 with the unified cgroup hierarchy. This commit removes those false explanations and fixes #9647

[LionNatsu]

Hold on. @poettering I think we should revert this patch.

For example, CPUQuota does imply CPUAccounting in the source code of cgroup_context_get_mask(CGroupContext *c)

Here's the first place the statement comes from:
b2f8b02#diff-934893719bf9881c440da8cf911341ccR470 (in 2014)

It changed a bit three months later.
3a43da2#diff-934893719bf9881c440da8cf911341ccR450

They are still there now:

systemd/src/core/cgroup.c

Lines 1063 to 1091 in 4d7293f

	CGroupMask cgroup_context_get_mask(CGroupContext *c) {
	CGroupMask mask = 0;
	/* Figure out which controllers we need */
	if (c->cpu_accounting ||
	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;
	if (cgroup_context_has_io_config(c) || cgroup_context_has_blockio_config(c))
	mask |= CGROUP_MASK_IO | CGROUP_MASK_BLKIO;
	if (c->memory_accounting ||
	c->memory_limit != CGROUP_LIMIT_MAX ||
	cgroup_context_has_unified_memory_config(c))
	mask |= CGROUP_MASK_MEMORY;
	if (c->device_allow ||
	c->device_policy != CGROUP_AUTO)
	mask |= CGROUP_MASK_DEVICES;
	if (c->tasks_accounting ||
	c->tasks_max != CGROUP_LIMIT_MAX)
	mask |= CGROUP_MASK_PIDS;
	return mask;
	}

The issue #9647 does exist but only for using user instance.
So, the statement "Implies xxxAccounting=true" is true for the system instance.
We should not fix the problem by dropping the documented feature first.

[keszybz]

@ryutaroh-matsumoto can you revert the change and instead add "for the systemd instance" where necessary?

[ghost]

@keszybz Yes, I can. But please wait.

What I wrote at 9647 was that aaaAccounting (aaa=CPU etc) shown by systemctl show someunit is not enabled by, for example, CPUQuota=50%. aaaAccounting corresponds to, for example, cpu_accounting in src/core/cgroup.c. Those variables in src/core/cgroup.c are not changed to true by, for example, CPUQuota=50%. So the change in documentation is still correct, I believe. False values of ???_accounting in cgroup.c prevents systemctl status from printing the accounting information in the current programming logic of systemd. As such, from a view point of ordinary users, Accounting is not enabled (of course an expert can read cpu.stat file, but it does not seem enablement of accounting).

On the other hand, we probably agree that CPUQuota=50% turns on the CPU controller. But

• In the original and the latest systemd.resource-control, "enablement of a controller" is not explicitly discussed.

• Besides all of the above, CPUQuota, MemoryHigh etc enable their corresponding controllers. In such a case, making ???_accounting=true gives more information without additional cost. So I wonder if the original documentation intended enablement of aaa_accounting but a corresponding program logic has been missing in the cgroup.c.

So just updating the documentation as requested seems to make situation worse, that is, the difference between the documentation and the actual behavior becomes larger than now.

[LionNatsu]

the difference between the documentation and the actual behavior becomes larger than now.

Yes, that's true, but it may be not worse, it "exposed" the bugs that systemd has. The documentation and the specifications of design are what a project based on. If we remove it, other people may think we are planning to deprecate the feature, so users may think "they have to add "CPUAccounting=true" ASAP otherwise their program is going to break." and developers could go ahead to remove the feature entirely. However, we are going to fix it, I think it's not a #wontfix issue.

The documentation described how we want systemd to work. On the other side, regarding your research on cgroups components, the difference between the program logic in accounting resources and documentation is large. Let's think about "which side is the 'good' one". Considering those differences "bugs in the implementation of systemd" and fixing it are exactly the things we should do now in my opinion. :-)

[ghost]

I assume the unified cgroup controller. I think we should make clear distinction among the following three concepts in our discussion.

1. cgroup.controllers has cpu.
2. Independent of the controller, cpu.stat always exists in Linux 4.15 and newer kernels, which holds the CPU accounting information of a unit.
3. CPUAccounting shown in systemctl show, which is equivalent to cpu_accounting in src/core/cgroup.c

I have always meant 3 by "CPUAccounting" in this thread. On the other hand, @LionNatsu seems to have meant 1 in his/her first post at #9669 (comment) (sorry I cannot identify your gender).

We should first agree on the meanings of words, otherwise our precious time will be wasted by a confused conversation.

[ghost]

As a side note, in PR 9665 it is planned that

1. CPU accounting information is printed by systemctl status even if CPUAccounting=no and the CPU controller is disabled, on Linux 4.15 and newer.

2. CPUAccounting=yes does not enable cpu in cgroup.controllers on Linux 4.15 and newer.

This change obscures the operational meaning of CPUAccounting. What is meant by CPUAccounting...? I am not very sure about the right design, but I don't think I am at a position to discuss the high-level design of systemd.

[ghost]

So in this PR 9669, I just proposed to update the documentation to correctly reflect what are done by systemd, without discussing what is a correct design. I am not a core developer and I do not intend to propose a design which I think right/correct.

[LionNatsu]

Thank you for the quick reply.

On the other hand, @LionNatsu seems to have meant 1 in his/her first post at #9669 (comment) (sorry I cannot identify your gender).

To be honest, I mean 1 and 3. What happened to me is that I was trying to use CPUQuota (kernel 4.17, systemd 139, unified hierarchy) few days ago and it ran into all the same problems you mentioned (so I am here to figure out what is going on). CPUQuota in --user is now buggy. There are also issues on showing the CPUAccounting value too. (Btw, sorry for this confusing nickname, I am male.)

What is meant by CPUAccounting...?

So setting CPUAccounting cannot always work properly, getting CPUAccounting is also problematic. This is problem confusing me as a user.

I am not a core developer and I do not intend to propose a design which I think right/correct.

I understand (I am not a core developer too). But just in my opinion, specifying a CPUQuota without explicitly specifying CPUAccounting is convenient. Let's see what we can do about it?

[LionNatsu]

Checking #9665, I agree with you to separate "CPUAccounting" from other controller options. IMO, two PRs seems should be one.

I'm not sure if here is the right place to ask this question: can systemd add CPUController, MemoryController… to reflect the underlying mask status ?

[ghost]

Let's see what we can do about it?

I am willing to do that. But in order to consider/find a right/correct design, one needs to understand how all of legacy, hybrid and unified cgroup hierarchies are handled by systemd and related programs. Support for those three hierarchies simultaneously turns the programming logic of systemd into a chaos (I should use a more polite word). I knows only the unified cgroup hierarchy and related programming logic in systemd, and not motivated to dig into the legacy or the hybrid hierarchy, as they will fade out. Because I am reluctant to learn the legacy and the hybrid hierarchy, I will wait a responses from a core member.

But you are welcome to express your own opinions. I will just wait a core member's response (unless I am asked to answer).

[ghost]

@LionNatsu you said that CPUAccounting is implied by CPUQuota in a system (not user) service, but I cannot confirm what you wrote. Please see below. So I still believe that my merged commit updated the documentation correctly for the unified cgroup hierarchy. I wonder if what you wrote was for the hybrid or the legacy-only cgroup hierarchy.

# /etc/systemd/system/bashsleep.service
[Service]
ExecStart=/bin/bash -c "sleep 10000"
CPUQuota=50%
root@buster-wayland:~# systemctl start bashsleep.service
root@buster-wayland:~# systemctl show bashsleep.service | fgrep CPU
CPUUsageNSec=[not set]
CPUAccounting=no
CPUWeight=[not set]
StartupCPUWeight=[not set]
CPUShares=[not set]
StartupCPUShares=[not set]
CPUQuotaPerSecUSec=500ms
LimitCPU=infinity
LimitCPUSoft=infinity
CPUSchedulingPolicy=0
CPUSchedulingPriority=0
CPUSchedulingResetOnFork=no

@keszybz After @LionNatsu and I reached to a same recognition, I would like to ask if you still request 'revert the change and instead add "for the systemd instance" where necessary'.

[LionNatsu]

@ryutaroh-matsumoto I think that's because I misunderstood what "implies CPUAccounting" means. I only found the CPU accounting features "also" works when I turned on the CPUQuota.

It seems the switch (CPUAccounting) was turn on simultaneously, but the fact is cpu.stat features works regardless of the CPUAcounting is on or off, and CPUAccounting doesn't reflect the features (cpu.stat) can or cannot work, which has a enhancement PR has been filed at #9665.

In some ways, the PR stepped out the significant step to make the CPUAccounting more semantically distinguishing.

I remember that it's also the initial thoughts of the cgroup v2 design (according to documentation of itself) so we can let one daemon (systemd) to manage controllers, provide resource control features on demand and get rid of complicated dependencies between controllers. Please correct me if I am wrong.

Hence I am closing the revert request. Would you @keszybz kindly remove the #merged-but-needs-fixing tag for the PR if there is no other problems? Thanks again.