[DO NOT MERGE] cgroup v1: optimize code to avoid parsing mountinfo if possible #2438

kolyshkin · 2020-05-29T00:33:10Z

This set of patches brings down a number of times we parse /proc/self/mountinfo during container start+stop:

with systemd: from 76 (49 after (*initProcess).start: rm second Apply #2446) to 6 times.
without systemd: from 116 (59 after (*initProcess).start: rm second Apply #2446) to 16.

(there is some improvement for other operations as well but I haven't measured those)

Ideally I'd like to bring it down to less than 5 per runc run, but this is already a super good improvement.

Update: this is being splitted into multiple smaller PRs to facilitate easier reviewing. The following list is just for my convenience to make sure I haven't forgot anything.

And here is the list of PRs this one was split into:

cgroups/v1: optimize path usage #2494 cgroups/v1: optimize path usage
libct/cgroups/fs: simple optimization #2495 libct/cgroups/fs: simple optimization
libct/cgroups/fs: simplify/speedup freezer code #2496 libct/cgroups/fs: simplify/speedup freezer code
libct/cgroups/fs: rm getCgroupRoot #2497 libct/cgroups/fs: rm getCgroupRoot (or the alternative: libct/cgroups/GetCgroupRoot: make it faster #2507)
libct/cgroups/fs: code cleanups #2498 libct/cgroups/fs: code cleanups
cgroupv1/FindCgroupMountpoint: add a fast path #2499 cgroupv1/FindCgroupMountpoint: add a fast path
libct/cgroups/fs: rework Apply() #2500 libct/cgroups/fs: rework Apply()

kolyshkin · 2020-05-29T03:48:42Z

I haven't done any measurements yet but this should be a relatively big win for cgroup v1 container start performance.

kolyshkin · 2020-05-29T19:15:53Z

Before this patch, runc run parses mountinfo 116 times. With this commit, the number goes down to 62. Which is still a lot.

kolyshkin · 2020-05-30T21:59:22Z

I'm still working on that, so let's make this a draft.

kolyshkin · 2020-06-09T20:55:44Z

I think this one is ready but it depends on #2411 which is still not merged.

In case cgroupPath is under the default cgroup prefix, let's try to guess the mount point by adding the subsystem name to the default prefix, and resolving the resulting path in case it's a symlink. In most cases, given the default cgroup setup, this trick should result in returning the same result faster, and avoiding /proc/self/mountinfo parsing which is relatively slow and problematic. Be very careful with the default path, checking it is - a directory; - a mount point; - has cgroup fstype. If something is not right, fall back to parsing mountinfo. While at it, remove the obsoleted comment about mountinfo parsing. The comment belongs to findCgroupMountpointAndRootFromReader(), but rather than moving it there, let's just remove it, since it does not add any value in understanding the current code. Signed-off-by: Kir Kolyshkin <[email protected]>

When paths are already set, we only need to place the PID into proper cgroups, and we already know all the cgroups path. The fs function d.path(), and the systemd v1 function getSubsystemPath() are both parsing /proc/self/mountinfo, and the only reason they are used here is to check whether the subsystem is available. Use a much simpler check instead. Frankly, I am not sure why the check is needed at all. Maybe we just need to drop it altogether. Also, for fs, since d is no longer used in this code path, moves its initialization to after it. Signed-off-by: Kir Kolyshkin <[email protected]>

We call joinCgroups() from Apply, and in there we iterate through the list of subsystems, calling getSubsystemPath() for each. This is expensive, since every getSubsystemPath() involves parsing mountinfo. At the end of Apply(), we iterate through the list of subsystems to fill the m.paths, again calling getSubsystemPath() for every subsystem. As a result, we parse mountinfo about 20 times here. Let's find the paths first and reuse m.paths in joinCgroups(). Signed-off-by: Kir Kolyshkin <[email protected]>

In all these cases, getSubsystemPath() was already called, and its result stored in m.paths map. It makes no sense to not reuse it. Signed-off-by: Kir Kolyshkin <[email protected]>

It does not add any value. Signed-off-by: Kir Kolyshkin <[email protected]>

In here, defer looks like an overkill, since the code is very simple and we already have an error path. Signed-off-by: Kir Kolyshkin <[email protected]>

Instead of iterating over m.paths, iterate over subsystems and look up the path for each. This is faster since a map lookup is faster than iterating over the names in Get. A quick benchmark shows that the new way is 2.5x faster than the old one. Note though that this is not done to make things faster, as savings are negligible, but to make things simpler by removing some code. Signed-off-by: Kir Kolyshkin <[email protected]>

All the test cases are doing the same checks, only input differs, so we can unify those using a test data table. While at it: - use t.Fatalf where it makes sense (no further checks are possible); - remove the "XXX" comments as we won't get rid of cgroup Name/Parent. PS I tried using t.Parallel() as well but it did not result in any noticeable speedup, so I dropped it for simplicity. Signed-off-by: Kir Kolyshkin <[email protected]>

I think this was added during the dark ages where cgroupfs can be mounted to /cgroup, /dev/cgroup, or some other path. Now it have finally settled on /sys/fs/cgroup, so let's remove the guessing and use the standard path. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2020-06-17T19:09:06Z

libcontainer/cgroups/fs/fs.go

 	m.paths = make(map[string]string)
 	if c.Paths != nil {
 		for name, path := range c.Paths {
-			_, err := d.path(name)
+			cgMap, err := cgroups.ParseCgroupFile("/proc/self/cgroup")


This is a check to see whether the subsystem is available. I am not quite sure in which scenario such a check is required. Maybe we need to just drop it.

kolyshkin · 2020-06-17T19:09:44Z

libcontainer/cgroups/systemd/v1.go

-				}
-				return err
+			if _, ok := cgMap[name]; ok {
+				paths[name] = path


same as above

kolyshkin · 2020-06-17T19:12:36Z

libcontainer/cgroups/systemd/v1.go

+			// Even if it's `not found` error, we'll return err
+			// because devices cgroup is hard requirement for
+			// container security.
+			if s.Name() == "devices" {


this was moved here from joinCgroups() (see below) so we bail out as early as in the old code in case of error.

kolyshkin · 2020-06-17T19:15:33Z

libcontainer/cgroups/systemd/v1.go

@@ -233,48 +240,25 @@ func (m *legacyManager) Path(subsys string) string {
 	return m.paths[subsys]
 }

-func join(c *configs.Cgroup, subsystem string, pid int) (string, error) {


the body of this function (modulo getSubsystemPath) was inlined into joinCgroups (see below)

kolyshkin · 2020-06-19T19:22:20Z

@AkihiroSuda @mrunalp @cyphar @crosbymichael PTAL

kolyshkin · 2020-07-03T02:15:24Z

OK, apparently this one is way too big to review comfortably. Let me split it in a few smaller and (hopefully) more digestible PRs.

kolyshkin · 2020-07-03T22:01:07Z

OK, apparently this one is way too big to review comfortably. Let me split it in a few smaller and (hopefully) more digestible PRs.

Split into a few smaller PRs; the description of this one is updated to list them all.

h-vetinari · 2020-08-21T07:53:14Z

Seems that only #2494 is left (and therefore now equivalent to this PR, but more up-to-date?)

kolyshkin · 2020-08-25T00:40:28Z

Seems that only #2494 is left (and therefore now equivalent to this PR, but more up-to-date?)

@h-vetinari this PR was split into 7 different ones, of which 5 are now merged, 6th got rewritten and merged, and yes you're right, the only one left is #2494. Once merged I'll close this one (which I ended up using to track the merge progress).

kolyshkin · 2020-09-23T00:54:27Z

Everything's merged now, closing

kolyshkin force-pushed the v1-less-mountinfo branch 2 times, most recently from f6fe9ba to 102659c Compare May 29, 2020 02:07

kolyshkin added the area/cgroupv1 label May 29, 2020

kolyshkin force-pushed the v1-less-mountinfo branch from 102659c to 407e631 Compare May 29, 2020 19:13

kolyshkin force-pushed the v1-less-mountinfo branch from b5cc7a3 to 3106475 Compare May 30, 2020 21:58

kolyshkin marked this pull request as draft May 30, 2020 21:58

kolyshkin force-pushed the v1-less-mountinfo branch 4 times, most recently from f810dac to 286035a Compare June 1, 2020 00:08

kolyshkin mentioned this pull request Jun 3, 2020

WIP/RFC: libcontainer/cgroups: add mountinfo cache #2265

Closed

kolyshkin force-pushed the v1-less-mountinfo branch 10 times, most recently from 3f555f0 to a455619 Compare June 9, 2020 19:59

kolyshkin added 5 commits June 17, 2020 08:07

cgroupv1/systemd: (re)use m.paths

55c1ce2

In all these cases, getSubsystemPath() was already called, and its result stored in m.paths map. It makes no sense to not reuse it. Signed-off-by: Kir Kolyshkin <[email protected]>

cgroups/fs: rm getSubsystems

db45ce7

It does not add any value. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin added 4 commits June 17, 2020 09:05

cgroups/fs/Freeze: simplify

cffb553

In here, defer looks like an overkill, since the code is very simple and we already have an error path. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the v1-less-mountinfo branch from a455619 to 5aef675 Compare June 17, 2020 18:43

kolyshkin marked this pull request as ready for review June 17, 2020 19:06

kolyshkin changed the title ~~cgroupv1/FindCgroupMountpoint: add a fast path~~ cgroup v1: optimize code to avoid parsing mountinfo if possible Jun 17, 2020

kolyshkin commented Jun 17, 2020

View reviewed changes

libcontainer/cgroups/systemd/v1.go

}

return err

if _, ok := cgMap[name]; ok {

paths[name] = path

Copy link

Contributor Author

kolyshkin Jun 17, 2020

Choose a reason for hiding this comment

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

same as above

kolyshkin commented Jun 17, 2020

View reviewed changes

This was referenced Jul 3, 2020

cgroups/v1: optimize path usage #2494

Merged

libct/cgroups/fs: simple optimization #2495

Merged

This comment has been minimized.

Sign in to view

kolyshkin mentioned this pull request Jul 3, 2020

libct/cgroups/fs: simplify/speedup freezer code #2496

Merged

This comment has been minimized.

Sign in to view

kolyshkin mentioned this pull request Jul 3, 2020

libct/cgroups/fs: rm getCgroupRoot #2497

Closed

This comment has been minimized.

Sign in to view

kolyshkin changed the title ~~cgroup v1: optimize code to avoid parsing mountinfo if possible~~ [DO NOT MERGE] cgroup v1: optimize code to avoid parsing mountinfo if possible Jul 3, 2020

kolyshkin marked this pull request as draft July 3, 2020 21:24

This was referenced Jul 3, 2020

libct/cgroups/fs: code cleanups #2498

Merged

cgroupv1/FindCgroupMountpoint: add a fast path #2499

Merged

libct/cgroups/fs: rework Apply() #2500

Merged

kolyshkin closed this Sep 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT MERGE] cgroup v1: optimize code to avoid parsing mountinfo if possible #2438

[DO NOT MERGE] cgroup v1: optimize code to avoid parsing mountinfo if possible #2438

kolyshkin commented May 29, 2020 •

edited

Loading

kolyshkin commented May 29, 2020

kolyshkin commented May 29, 2020

kolyshkin commented May 30, 2020

kolyshkin commented Jun 9, 2020

kolyshkin Jun 17, 2020

kolyshkin Jun 17, 2020

kolyshkin Jun 17, 2020

kolyshkin Jun 17, 2020 •

edited

Loading

kolyshkin commented Jun 19, 2020

kolyshkin commented Jul 3, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kolyshkin commented Jul 3, 2020

h-vetinari commented Aug 21, 2020

kolyshkin commented Aug 25, 2020

kolyshkin commented Sep 23, 2020

[DO NOT MERGE] cgroup v1: optimize code to avoid parsing mountinfo if possible #2438

[DO NOT MERGE] cgroup v1: optimize code to avoid parsing mountinfo if possible #2438

Conversation

kolyshkin commented May 29, 2020 • edited Loading

kolyshkin commented May 29, 2020

kolyshkin commented May 29, 2020

kolyshkin commented May 30, 2020

kolyshkin commented Jun 9, 2020

kolyshkin Jun 17, 2020

Choose a reason for hiding this comment

kolyshkin Jun 17, 2020

Choose a reason for hiding this comment

kolyshkin Jun 17, 2020

Choose a reason for hiding this comment

kolyshkin Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

kolyshkin commented Jun 19, 2020

kolyshkin commented Jul 3, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kolyshkin commented Jul 3, 2020

h-vetinari commented Aug 21, 2020

kolyshkin commented Aug 25, 2020

kolyshkin commented Sep 23, 2020

kolyshkin commented May 29, 2020 •

edited

Loading

kolyshkin Jun 17, 2020 •

edited

Loading

kolyshkin commented Jul 3, 2020 •

edited

Loading